web-platform-tests / wpt

Test suites for Web platform specs — including WHATWG, W3C, and others
https://web-platform-tests.org/
Other
4.86k stars 3.04k forks source link

Support changing the DPI in tests #7140

Open wpt-issue-mover opened 7 years ago

wpt-issue-mover commented 7 years ago

Originally posted as https://github.com/w3c/wptrunner/issues/240 by @gsnedders on 21 Mar 2017, 17:50 UTC:

Split out from https://github.com/w3c/wptrunner/issues/166

[edit: see https://github.com/web-platform-tests/wpt/issues/7140#issuecomment-464757730 for the current status]

progers commented 5 years ago

Is this issue for adding support for changing DPI in web platform tests?

I just ran into this when trying to write a WPT test for a chromium bug. Our rendering is different with different DPI values and we had a bug that was only present at non-1.0 DPIs (https://crbug.com/925676). I wanted to add a WPT test for this but was not able to because we don't have an API for that yet. Blink's test runner has SetBackingScaleFactor(double) which is roughly the API we'd need to add. In blink we have 45 tests that use SetBackingScaleFactor.

gsnedders commented 5 years ago

Is this issue for adding support for changing DPI in web platform tests?

Yes. Essentially we have undocumented support in wptrunner for this for Servo only, and given it's something that is generally useful it's been a low-priority to-do item for the long time.

I just ran into this when trying to write a WPT test for a chromium bug. Our rendering is different with different DPI values and we had a bug that was only present at non-1.0 DPIs (https://crbug.com/925676). I wanted to add a WPT test for this but was not able to because we don't have an API for that yet. Blink's test runner has SetBackingScaleFactor(double) which is roughly the API we'd need to add. In blink we have 45 tests that use SetBackingScaleFactor.

The current support we have relies on <meta name=device-pixel-ratio contents=[double]>. Given some browsers need restarted to actually pay attention to that changing, we need something statically readable, as that is. I wouldn't be surprised if we put a wpt- prefix on the name or something.

progers commented 5 years ago

In the review of my chromium bug fix, a reviewer pointed out that chromium has two ways to write DPI web tests: the testRunner.SetBackingScaleFactor API and a chromium test feature called "virtual test suites". Virtual test suites are where certain test directories re-run with different flags such as gpu acceleration enabled/disabled, directwrite fonts enabled/disabled, threaded compositing enabled/disabled, various scale factors, etc. To avoid running all tests many times, virtual suites are limited to certain directories (see: VirtualTestSuites). These approaches are chromium-specific though, so we need a way to write upstreamed WPT tests.

I would support either of these approaches in WPT tests: special directories that are declared to depend on DPI, or an explicit DPI flag used in the test itself. I looked through chromium's existing tests that use SetBackingScaleFactor and it would not be an issue to switch these to use a static, declarative API such as <meta name=wpt-device-pixel-ratio contents=[double]> (in other words, the existing tests are not testing changing DPIs).

Image srcset is an example of a web feature that needs DPI control to test. I didn't find existing srcset WPT tests that cover srcset resolutions (there are some DPR header tests but I didn't see any that test the rendering of 2x images). (@yoavweiss , do you know if this is tested and I just missed it?) I think we need DPI control to write WPT srcset tests.

Do you mind if we change the title of this issue to "Add DPI support to WPT tests"?

yoavweiss commented 5 years ago

On Tue, Feb 19, 2019, 16:48 Philip Rogers notifications@github.com wrote:

In the review of my chromium bug fix, a reviewer pointed out that chromium has two ways to write DPI web tests: the testRunner.SetBackingScaleFactor https://cs.chromium.org/chromium/src/content/shell/test_runner/test_runner.cc?type=cs&l=222 API and a chromium test feature called "virtual test suites". Virtual test suites are where certain test directories re-run with different flags such as gpu acceleration enabled/disabled, directwrite fonts enabled/disabled, threaded compositing enabled/disabled, various scale factors, etc. To avoid running all tests many times, virtual suites are limited to certain directories (see: VirtualTestSuites https://cs.chromium.org/chromium/src/third_party/blink/web_tests/VirtualTestSuites). These approaches are chromium-specific though, so we need a way to write upstreamed WPT tests.

I would support either of these approaches in WPT tests: special directories that are declared to depend on DPI, or an explicit DPI flag used in the test itself. I looked through chromium's existing tests that use SetBackingScaleFactor https://cs.chromium.org/chromium/src/content/shell/test_runner/test_runner.cc?type=cs&l=222 and it would not be an issue to switch these to use a static, declarative API such as (in other words, the existing tests are not testing changing DPIs).

Image srcset is an example of a web feature that needs DPI control to test. I didn't find existing srcset WPT tests that cover srcset resolutions (there are some DPR header tests but I didn't see any that test the rendering of 2x images). (@yoavweiss https://github.com/yoavweiss , do you know if this is tested and I just missed it?) I think we need DPI control to write WPT srcset tests.

I believe we tested those with internal layout tests. Since the actual selection algorithm is not strictly defined, that seems OKish. (E.g. the browser can intervene and request a lower resolution image) It would still be better to have baseline WPTs that make sure the non-intervention case is working as expected.

Regardless of srcset, being able to set DPR for wpt tests seems valuable (e.g. for client hint, for the devicePixelRatio interface, etc)

Do you mind if we change the title of this issue to "Add DPI support to WPT tests"?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/web-platform-tests/wpt/issues/7140#issuecomment-465186715, or mute the thread https://github.com/notifications/unsubscribe-auth/AAv_C2MGY2IPbW3C2YUMSDy7-yhW80jeks5vPBzlgaJpZM4PJKOU .

noamr commented 4 years ago

I've added tests for image-set, and encountered the same need.... src selection in image-set can only be effectively tested when diffrent DPRs are allowed

noamr commented 4 years ago

I'm currently writing tests that truly need this: https://github.com/web-platform-tests/wpt/pull/23789, otherwise they don't test a lot of the functionality.

I tried to fix this myself but got lost in the complexities of the python infrastructure... any help with this would be appreciated.

stephenmcgruer commented 4 years ago

So today, Edge (the Chromium-base one), Chrome, and Safari have their reftest-support in tools/wptrunner/wptrunner/executors/executorwebdriver.py, and Firefox has it in tools/wptrunner/wptrunner/executors/executormarionette.py.

As you can see, we currently assert that dpi is None, because only Servo supports changing the dpi (check out executorservo.py). I believe they are able to support this because they always shutdown and re-launch their browser after every test, whereas the others re-use a browser instance between tests where possible for speed.

Options for adding dpi support that I can think of:

  1. Detect when dpi is non-default, and restart the browser with the appropriate flag in such cases. (E.g. for Chromium there's --force-device-scale-factor which I think is equivalent to changing dpi?). This is a bit weird as its a violation of abstraction for most browsers; normally the higher-level testrunner.py (tools/wptrunner/wptrunner/testrunner.py) takes care of the browser lifecycle. (Servo appears to be an exception to this, which is interesting).
  2. Add a WebDriver or other web-exposed API for setting the DPI. This is a longer path given it requires getting browsers to agree on this being useful and feasible, but its much less hacky than restarting browsers and passing special flags. (And works on any browser that adheres to the standard).

cc @jgraham @Hexcles who may have thoughts (also gsnedders, but they're already on this issue)

jgraham commented 4 years ago

There are a couple of hooks here to help. settings is for things that require a full browser restart because of a change in the test setup e.g. [1]. If the settings dictionary changes from one test to the next the runner will arrange for a full restart, passing in the settings dictionary [2].

If the pref can be changed at runtime without a restart, it can form part of the test "environment" [3], which is basically a mechanism to do some stateful things before a test, but only if necessary.

So if this can be done in terms of prefs, I think it's pretty easy to do in wptrunner. In gecko it can be controlled using the layout.css.devPixelsPerPx pref, and that can be set at runtime. That means for gecko at least this can form part of the test environment.

I independently think that this makes sense as a WebDriver feature, and would certainly support a PR against that spec to make it an option. That may be needed to support WebKit (I have no idea how settings in WebKit works, so it may also not be needed).

[1] https://searchfox.org/mozilla-central/source/testing/web-platform/tests/tools/wptrunner/wptrunner/browsers/firefox.py#725 [2] https://searchfox.org/mozilla-central/source/testing/web-platform/tests/tools/wptrunner/wptrunner/testrunner.py#222 [3] https://searchfox.org/mozilla-central/source/testing/web-platform/tests/tools/wptrunner/wptrunner/executors/base.py#285

noamr commented 4 years ago

There are a couple of hooks here to help. settings is for things that require a full browser restart because of a change in the test setup e.g. [1]. If the settings dictionary changes from one test to the next the runner will arrange for a full restart, passing in the settings dictionary [2].

If the pref can be changed at runtime without a restart, it can form part of the test "environment" [3], which is basically a mechanism to do some stateful things before a test, but only if necessary.

So if this can be done in terms of prefs, I think it's pretty easy to do in wptrunner. In gecko it can be controlled using the layout.css.devPixelsPerPx pref, and that can be set at runtime. That means for gecko at least this can form part of the test environment.

I independently think that this makes sense as a WebDriver feature, and would certainly support a PR against that spec to make it an option. That may be needed to support WebKit (I have no idea how settings in WebKit works, so it may also not be needed).

WebKit internal tests can modify the dpr before loading a page using Javascript (setBackingScaleFactor). It's used by internal srcset tests.

noamr commented 4 years ago

I think the following issue in WebDriver is about this: https://github.com/w3c/webdriver/issues/1378

stephenmcgruer commented 4 years ago

WebKit internal tests can modify the dpr before loading a page using Javascript (setBackingScaleFactor). It's used by internal srcset tests.

I suspect that feature is likely not exposed in main Safari though, I presume? (Similar to Chromium's internals testing API, which only exists in our content_shell not chrome) As such it would present the problem that runners using Safari (e.g. what we do here in wpt) would never pass those tests.

noamr commented 4 years ago

WebKit internal tests can modify the dpr before loading a page using Javascript (setBackingScaleFactor). It's used by internal srcset tests.

I suspect that feature is likely not exposed in main Safari though, I presume? (Similar to Chromium's internals testing API, which only exists in our content_shell not chrome) As such it would present the problem that runners using Safari (e.g. what we do here in wpt) would never pass those tests.

When wpt is imported to webkit it uses the internal testing API rather than what's exposed to Safari. I'm not sure what's exposed in Safari, I don't know the Safari WebDriver code. But in any case, I think WebDriver is the way to go, something simple-ish like setting scale-factor for a WebDriver context that would affect subsequent navigations in a window.