Closed Bug 1575097 Opened 3 months ago Closed 3 months ago

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

Categories

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

defect

Tracking

(firefox70 fixed)

RESOLVED FIXED
Firefox 70
Tracking Status
firefox70 --- fixed

People

(Reporter: bradwerth, Assigned: bradwerth)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [rdm-mvp])

Attachments

(7 files, 1 obsolete file)

Attached file 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.

Priority: P1 → P3
Whiteboard: [rdm-mvp] → [rdm-reserve]

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?

Flags: needinfo?(botond)

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.

(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.

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.

Status: NEW → ASSIGNED
Priority: P3 → P1
Whiteboard: [rdm-reserve] → [rdm-mvp]

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.)

Flags: needinfo?(botond)

(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.

Summary: RDM: window.alert changes the value of screen.width when meta viewport is enabled → RDM: screen.width and window.outerWidth should be the width of simulated device

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?

(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.

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

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?

Flags: needinfo?(ehsan)

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.)

Flags: needinfo?(ehsan)
Attachment #9086852 - Attachment description: Bug 1575097 Part 1: Make nsScreen::GetRDMScreenSize accept a size instead of a rect. → Bug 1575097 Part 2: Make nsScreen::GetRDMScreenSize accept a CSSIntSize instead of an nsRect.
Attachment #9086854 - Attachment description: Bug 1575097 Part 2: Make nsScreen::GetRDMScreenSize return unscaled browser sizes. → Bug 1575097 Part 3: Make nsScreen::GetRDMScreenSize get the size from the window.
Attachment #9086855 - Attachment is obsolete: true
Blocks: 1576256
Blocks: 1576298
Pushed by bwerth@mozilla.com:
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
See Also: → 1576542
Duplicate of this bug: 1124731
You need to log in before you can comment on or make changes to this bug.