1575097 - RDM: screen.width and window.outerWidth should be the width of simulated device

Status: RESOLVED FIXED

(DevTools :: Responsive Design Mode, defect, P1)

Description

[Brad Werth [:bradwerth]]

Attachment: screen-width.html

Steps to Reproduce

1. In about:config, set devtools.responsive.metaViewport.enabled to true.
2. Open attached testcase. Dismiss the alert dialog that appears.
3. Switch to Responsive Design Mode.
4. Ensure that Touch Simulation is turned on.
5. Reload.

Expected results: the two screen width values (before and after the alert dialog) have the same value, which is equal to the width of the simulated device.

Actual results: the second screen width value is 980, regardless of the width of the simulated device.

Comment 1

[Brad Werth [:bradwerth]]

The differing values before and after the window.alert are really about inline script running before the browser has applied meta viewport scaling and called setInitialViewport. When the viewport is resized to its default of 980 CSS pixels wide, that affects the screen.width value because of https://searchfox.org/mozilla-central/rev/03853a6e87c4a9405fce1de49e5d03b9e7a7a274/dom/base/nsGlobalWindowOuter.cpp#3429.

Chrome doesn't seem to care about the scaling of the visual viewport, and instead returns the (simulated) screen size. Reading of the spec at https://drafts.csswg.org/cssom-view/#dom-window-innerwidth seems to say that the Firefox behavior is correct, but this will be a web compatibility issue if we keep this behavior.

I'm in favor of matching Chrome behavior here. Botond, any objection?

Comment 2

[Brad Werth [:bradwerth]]

I realize I need to test this on GeckoView to see what we report there. Whatever we report on GeckoView we need to report in RDM, regardless of whether or not it matches Chrome behavior.

Comment 3

[Brad Werth [:bradwerth]]

(In reply to Brad Werth [:bradwerth] from comment #2)

I realize I need to test this on GeckoView to see what we report there. Whatever we report on GeckoView we need to report in RDM, regardless of whether or not it matches Chrome behavior.

GeckoView returns the device resolution, not the viewport size, so that's another reason to make this change.

Comment 4

[Brad Werth [:bradwerth]]

Attachment: Bug 1575097 Part 2: Make nsScreen::GetRDMScreenSize accept a CSSIntSize instead of an nsRect.

Comment 5

[Brad Werth [:bradwerth]]

Attachment: Bug 1575097 Part 3: Make nsScreen::GetRDMScreenSize get the size from the window.

Depends on D42743

Comment 6

[Brad Werth [:bradwerth]]

Attachment: Bug 1575097 Part 3: Make nsGlobalWindowOuter::GetOuterSize return unscaled browser sizes for RDM documents.

Depends on D42745

Comment 7

[Brad Werth [:bradwerth]]

Attachment: Bug 1575097 Part 5: Update test to check outerWidth and screen.width and device-width media queries with meta viewport on.

Depends on D42746

Comment 8

[Brad Werth [:bradwerth]]

This is really hard! Further testing shows that innerWidth should also be equal to the device resolution, and media queries against device width need to use device resolution values. More work is needed, but I'm putting up the current version of the patches.

Given that at least 4 things need to change (window.innerWidth, window.outerWidth, screen.width, and media query device-width) it's likely that I should find a narrower approach that can affect all of them at once instead of the piecemeal methods being used in the current patches.

Comment 9

[Botond Ballo [:botond]]

Changing window.innerWidth to match Chrome's behaviour and report the layout viewport size, is tracked in bug 1514429.

The main blocker for that was shipping the Visual Viewport API, which we have done (on Android, which is the only place where the distinction is currently relevant).

So, if making that change fixes this issue as well, we may just want to go ahead and do it. (Note, though, that there will probably be a bunch of internal uses of innerWidth that will need to be updated.)

Comment 10

[Brad Werth [:bradwerth]]

(In reply to Botond Ballo [:botond] from comment #9)

So, if making that change fixes this issue as well, we may just want to go ahead and do it. (Note, though, that there will probably be a bunch of internal uses of innerWidth that will need to be updated.)

It doesn't solve the problem, unfortunately. I posted a patch for Bug 1514429. For this bug, I'll keep going on the path laid out by these patches and see if I can determine the mechanism by which meta viewport is forcing the use of the visual viewport values instead of the CSS viewport values.

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Though I might be misunderstanding this problem, it seems to me that the poped-up alert window should be just rendered inside the RDM content window instead of the browser window?

Comment 12

[Brad Werth [:bradwerth]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #11)

Though I might be misunderstanding this problem, it seems to me that the poped-up alert window should be just rendered inside the RDM content window instead of the browser window?

Could be. We haven't figured out the right way to do alerts in RDM. I re-titled the bug because the window alert isn't the most interesting part of this bug. The issue that initially caused me to file the bug is that reading the viewport in an inline script doesn't have access to the final viewport post reflow. The window.alert was just ensuring that the second inline script ran after the call to setInitialViewport. There may be another bug that needs to be addressed there, but I need to retest with a build that includes your fix for Bug 1553689.

Comment 13

[Brad Werth [:bradwerth]]

Attachment: Bug 1575097 Part 4: Make nsMediaFeatures::GetDeviceSize return unscaled browser sizes for RDM documents.

Depends on D42746

Comment 14

[Brad Werth [:bradwerth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bbf6931bb8a99a5cc995381bcc62c7238e4905ee

Comment 15

[Brad Werth [:bradwerth]]

Ehsan, in this patch stack, I'm duplicating logic for retrieving the unscaled device size of the simulated Responsive Design Mode device. The changes are currently being made in three functions: nsScreen::GetRDMScreenSize, nsGlobalWindowOuter::GetOuterSize, and nsMediaFeatures::GetDeviceSize. It would be cleaner to consolidate this logic into one place. One natural place would be as a new function Document::GetRDMDeviceSize. Would you support that? If not, would you please propose an alternative you would support?

Comment 16

[(no longer active)]

Hi Brad,

I assume these functions are similar to nsGlobalWindowOuter::GetOuterSize() as it is today, please correct me if my understanding is wrong.

The problem with putting the shared code in Document is that there may not always be a Document for a given outer window. In fact that function already handles cases where the outer window doesn't have a doc. So I doubt Document is the exact right place for sharing the code... It appears that this is really the property of a "browsing context" in the HTML spec terminology (that is, we're computing the size of our browsing context), so perhaps the new BrowsingContext class would be a better place for the GetRDMDeviceSize() function to live? If not, how about nsGlobalWindowOuter itself?

(What I'm thinking is to put the shared code somewhere with an object that is always accessible, and I think one of those two places would fit the bill. Let me know what you think.)

Comment 17

[Brad Werth [:bradwerth]]

Attachment: Bug 1575097 Part 1: Add a method nsGlobalWindowOuter::GetRDMDeviceSize and use it in GetOuterSize.

Comment 18

[Brad Werth [:bradwerth]]

Attachment: Bug 1575097 Part 6: Change RDM test helper function to check innerWidth instead of screen.width.

Depends on D42748

Comment 19

[Brad Werth [:bradwerth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a01d460a246cebd96f47d9380ef8ee688bc46882

Comment 20

[Brad Werth [:bradwerth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed6387c8098ba6bc403011577632f948892b56d7

Comment 21

[Brad Werth [:bradwerth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1b19cee1a3a02c00a5cc70e6819767c9c2ff49f8

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7e2f3bd041de
Part 1: Add a method nsGlobalWindowOuter::GetRDMDeviceSize and use it in GetOuterSize. r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/6125b2b41908
Part 2: Make nsScreen::GetRDMScreenSize accept a CSSIntSize instead of an nsRect. r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/651580f22a7c
Part 3: Make nsScreen::GetRDMScreenSize get the size from the window. r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/79b515c90d54
Part 4: Make nsMediaFeatures::GetDeviceSize return unscaled browser sizes for RDM documents. r=emilio
https://hg.mozilla.org/integration/autoland/rev/47099c4e6346
Part 5: Update test to check outerWidth and screen.width and device-width media queries with meta viewport on. r=gl
https://hg.mozilla.org/integration/autoland/rev/4c5c38750336
Part 6: Change RDM test helper function to check innerWidth instead of screen.width. r=gl

Comment 23

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/7e2f3bd041de
https://hg.mozilla.org/mozilla-central/rev/6125b2b41908
https://hg.mozilla.org/mozilla-central/rev/651580f22a7c
https://hg.mozilla.org/mozilla-central/rev/79b515c90d54
https://hg.mozilla.org/mozilla-central/rev/47099c4e6346
https://hg.mozilla.org/mozilla-central/rev/4c5c38750336