1514429 - window.innerWidth/innerHeight should return the dimensions of the layout viewport

Status: RESOLVED FIXED

(Core :: Layout, enhancement, P3)

Description

[Botond Ballo [:botond]]

window.inner{Width,Height} currently return the dimensions of the visual viewport. (Background of layout vs. visual viewport terminology: [1].)

This has been changed a couple of times aready (bug 602580, bug 919437), but I believe we'll need to change it again to align with Blink, which has settled on reporting the layout viewport dimensions [2].

Their justification for that choice is that we have the VisualViewport API [3] to report the visual viewport dimensions; I think that's reasonable.

We probably want to wait until shipping the Visual Viewport API (bug 1512813) before making this change.

[1] https://github.com/bokand/bokand.github.io/blob/master/web_viewports_explainer.md
[2] https://docs.google.com/spreadsheets/d/11DfDDa-s1hePVwBn3PZidlPJZ9ahhkJ44dyuMiOQtrc/edit#gid=0
[3] https://github.com/WICG/visual-viewport

Comment 1

[Brad Werth [:bradwerth] | Out until Feb 2025]

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

window.inner{Width,Height} currently return the dimensions of the visual
viewport.

I've been looking at this. The getters return visual viewport (nsIPresShell::GetVisualViewportSize) only when the visual viewport is overridden, and return the layout viewport (nsPresContext::GetVisibleArea) otherwise. Setters behave similarly. That means that most of the time we're using the layout viewport. Visual viewport override occurs when Responsive Design Mode is enabled.

We probably want to wait until shipping the Visual Viewport API (bug
1512813) before making this change.

I'd like to fix this right away because it is a blocker for Responsive Design Mode bugs like Bug 1504659. Given that RDM already has bugs regarding the visual viewport, is there a problem with making the change now to make innerWidth and innerHeight ALWAYS use the layout viewport?

Comment 2

[Botond Ballo [:botond]]

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

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

window.inner{Width,Height} currently return the dimensions of the visual
viewport.

I've been looking at this. The getters return visual viewport (nsIPresShell::GetVisualViewportSize) only when the visual viewport is overridden, and return the layout viewport (nsPresContext::GetVisibleArea) otherwise. Setters behave similarly. That means that most of the time we're using the layout viewport. Visual viewport override occurs when Responsive Design Mode is enabled.

If nsIPresShell::IsVisualViewportSizeSet() returns false, the visual and layout viewports have the same size. So, it is the case that currently inner{Width,Height} always evaluates to the visual viewport size. The only reason we have a condition here [1] is that when nsIPresShell::IsVisualViewportSizeSet() returns false, no one has set nsIPresShell::mVisualViewportSize, so we'd be trying to use an empty size if we called nsIPresShell::GetVisualViewportSize() in that case.

We probably want to wait until shipping the Visual Viewport API (bug
1512813) before making this change.

I'd like to fix this right away because it is a blocker for Responsive Design Mode bugs like Bug 1504659. Given that RDM already has bugs regarding the visual viewport, is there a problem with making the change now to make innerWidth and innerHeight ALWAYS use the layout viewport?

If we make this change before shipping the Visual Viewport API, we would be depriving page authors of a way to obtain the visual viewport size.

If you give some more details about how this is causing a problem for RDM, I may be able to suggest an alternate solution.

[1] https://searchfox.org/mozilla-central/rev/76fe4bb385348d3f45bbebcf69ba8c7283dfcec7/dom/base/nsGlobalWindowOuter.cpp#3049

Comment 3

[Brad Werth [:bradwerth] | Out until Feb 2025]

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

If nsIPresShell::IsVisualViewportSizeSet() returns false, the visual and layout viewports have the same size. So, it is the case that currently inner{Width,Height} always evaluates to the visual viewport size. The only reason we have a condition here [1] is that when nsIPresShell::IsVisualViewportSizeSet() returns false, no one has set nsIPresShell::mVisualViewportSize, so we'd be trying to use an empty size if we called nsIPresShell::GetVisualViewportSize() in that case.

I had assumed that a meta viewport tag would change the layout viewport size without affecting the visual viewport, and without setting nsIPresShell::IsVisualViewportSizeSet(), leading to a mismatch. If that is not the case, and IsVisualViewportSizeSet() is equivalent to "visual viewport may differ from layout viewport", then I agree there is no problem here.

If you give some more details about how this is causing a problem for RDM, I may be able to suggest an alternate solution.

In Bug 1504659 I need the actual viewport width. The actual visible amount of the document. I see now that GetVisualViewportSize() returns both the viewable area and the area covered by any visible scrollbars. By subtracting away the area of the scrollbars, I should be able to get what I need. I'll needinfo you (on that bug) if I have trouble.

Comment 4

[Botond Ballo [:botond]]

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

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

If nsIPresShell::IsVisualViewportSizeSet() returns false, the visual and layout viewports have the same size. So, it is the case that currently inner{Width,Height} always evaluates to the visual viewport size. The only reason we have a condition here [1] is that when nsIPresShell::IsVisualViewportSizeSet() returns false, no one has set nsIPresShell::mVisualViewportSize, so we'd be trying to use an empty size if we called nsIPresShell::GetVisualViewportSize() in that case.

I had assumed that a meta viewport tag would change the layout viewport size without affecting the visual viewport,

Yep.

and without setting nsIPresShell::IsVisualViewportSizeSet(), leading to a mismatch. If that is not the case, and IsVisualViewportSizeSet() is equivalent to "visual viewport may differ from layout viewport", then I agree there is no problem here.

I believe the only scenario where we process a meta viewport tag but don't set a visual viewport size, is fixed by this patch. So, with that patch in place, there should be no problem.

If you give some more details about how this is causing a problem for RDM, I may be able to suggest an alternate solution.

In Bug 1504659 I need the actual viewport width. The actual visible amount of the document. I see now that GetVisualViewportSize() returns both the viewable area and the area covered by any visible scrollbars.

That surprises me. The visual viewport size is set here based on MobileViewportManager::GetCompositionSize(), which subtracts the scrollbar dimensions here.

Comment 5

[Brad Werth [:bradwerth] | Out until Feb 2025]

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

In Bug 1504659 I need the actual viewport width. The actual visible amount of the document. I see now that GetVisualViewportSize() returns both the viewable area and the area covered by any visible scrollbars.

That surprises me. The visual viewport size is set here based on MobileViewportManager::GetCompositionSize(), which subtracts the scrollbar dimensions here.

Yes, you're right. I was working from memory of what I believed to be the root cause when I started working on the bug. I just pared back my patches to the minimal fix (which you've already reviewed, thank you) plus test and things look good locally. I'm doing a try server run to see if it's landable. I honestly don't know what changed between my initial landing attempt and now, but it looks like it might go through.

Comment 6

[csheany]

You might not need them but I thought these links could be useful.

https://github.com/andreasbovens/understanding-viewport
http://andreasbovens.github.com/understanding-viewport/

Comment 7

[Botond Ballo [:botond]]

(In reply to csheany from comment #6)

You might not need them but I thought these links could be useful.

https://github.com/andreasbovens/understanding-viewport
http://andreasbovens.github.com/understanding-viewport/

That is a neat set of test pages, thanks!

Comment 8

[Brad Werth [:bradwerth] | Out until Feb 2025]

Attachment: Bug 1514429 Part 1: Make window inner size getters and setters exclusively use the CSS viewport.

Comment 9

[Brad Werth [:bradwerth] | Out until Feb 2025]

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

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Brad, what's the status on this bug? I did take a look at failures in the try in comment 9. The failure on

test_viewport_metrics_on_landscape_content.html is a case that bug 1508177 didn't handle properly. I am fine with replacing is in question with todo and filing a new bug for that. As for the failure on test_innersize_scrollport.html, I believe it's no longer valid, we can drop it entirely. As for browser_viewport_resizing_after_reload.js, I have no idea what's going on there since I can't run it locally due to bug 1595936, but you are more familiar with the code than I am, so you've already known what we should do?

Comment 12

[Brad Werth [:bradwerth] | Out until Feb 2025]

I'm rebasing my old patch and will provide a new test run shortly to see what needs to be fixed in code and in tests.

Comment 13

[Brad Werth [:bradwerth] | Out until Feb 2025]

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

Comment 14

[Brad Werth [:bradwerth] | Out until Feb 2025]

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

test_viewport_metrics_on_landscape_content.html is a case that bug 1508177 didn't handle properly.

I can't figure out the fix for this. The test conflates layout and visual viewports starting at const layoutViewportHeight = window.visualViewport.height; and though I could make the test pass, I don't know how the various comparisons it makes (scrollRect, document.clientBoundingRect) should be handled. I'll add a part to the patch that disables this test.

Comment 15

[Hiroyuki Ikezoe (:hiro)]

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

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

test_viewport_metrics_on_landscape_content.html is a case that bug 1508177 didn't handle properly.

I can't figure out the fix for this. The test conflates layout and visual viewports starting at const layoutViewportHeight = window.visualViewport.height; and though I could make the test pass, I don't know how the various comparisons it makes (scrollRect, document.clientBoundingRect) should be handled. I'll add a part to the patch that disables this test.

The failure happens on this line, we can just replace is with todo_is for now.

The content in the test case has minimum-scale=0.5 and a width: 200% element, but the root element height is height: 100%, so the content is shrunk to fit the 200% width to the screen, but nsPresContext.mVisibleArea (i.e. height:100%)is unchanged, so once we return the nsPresContext.mVisibleArea for window.innnerHeight, it's no longer correct, it should be size of the expanded size by the minimum-scale. And of course Chrome does returns the expanded size for window.innerHeight.

Brad, would you mind filing a bug for this issue and leave a comment in the todo_is?

Oops, a collision happened. I'd prefer to use todo_is instead of disabling whole test.

Comment 16

[Brad Werth [:bradwerth] | Out until Feb 2025]

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

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

I can't figure out the fix for this. The test conflates layout and visual viewports starting at const layoutViewportHeight = window.visualViewport.height;

I think this is a foundational issue that makes several of the comparisons fail.

The failure happens on this line, we can just replace is with todo_is for now.

That line doesn't fail and two others do. I'll make a version of the patch that changes the necessary lines to todo_is and notes the new Bug 1598487 as tracking the fix.

Comment 17

[Brad Werth [:bradwerth] | Out until Feb 2025]

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

As for browser_viewport_resizing_after_reload.js, I have no idea what's going on there

The fix for this test is a combination of fixing expectations in the test itself (easy) and fixing the RDM waitForViewportResizeTo function, which is not as easy for me. For that function "viewport" really means "device size". It looks like the internal state of the viewport React component is affected by scaling and is neither the visual viewport nor the layout viewport. I'll sort that out and then we should have a landable patch.

Comment 18

[Brad Werth [:bradwerth] | Out until Feb 2025]

Attachment: Bug 1514429 Part 2: Update a test that assumes innerWidth should change while zooming.

This test treats innerWidth as being related to the visual viewport,
which is no longer is. The test has no purpose now.

Depends on D42940

Comment 19

[Brad Werth [:bradwerth] | Out until Feb 2025]

Attachment: Bug 1514429 Part 3: Disable parts of a test of visual vs layout viewport, and mark it for needing a correctness audit.

This test attempts to relate the visual viewport, layout viewport, the
document scroll rect and the document bounding client rect. Some of the
logic here is no longer correct and needs revision. It's also possible
that there is a bug revealed in the visualviewport API code. I can't
find a simple, logical solution so I opened Bug 1598487 for the fix.

Depends on D54350

Comment 20

[Brad Werth [:bradwerth] | Out until Feb 2025]

Attachment: Bug 1514429 Part 4: Update tests and test functions that conflated visual viewport and layout viewport.

Responsive Design Mode sends internal messages for the content size and
viewport size. The viewport size used to be reflective of the visual
viewport. These changes make it use the layout viewport instead.

The viewport is also being used as a stand-in for RDM device size. That
needs more investigation since content size and viewport size should be
conceptually distinct. This need for clarity is less urgent because the
RDM feature is being rebuilt for Fission support, tracked by
Bug 1574885. If this issue becomes problematic before Bug 1574885 is
landed, then we'll need to open a new bug to resolve it.

Depends on D54351

Comment 21

[Brad Werth [:bradwerth] | Out until Feb 2025]

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

Comment 22

[Brad Werth [:bradwerth] | Out until Feb 2025]

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

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1e91e19089dd
Part 1: Make window inner size getters and setters exclusively use the CSS viewport. r=botond
https://hg.mozilla.org/integration/autoland/rev/9ba5efadaff0
Part 2: Update a test that assumes innerWidth should change while zooming. r=botond
https://hg.mozilla.org/integration/autoland/rev/33d9590eca92
Part 3: Disable parts of a test of visual vs layout viewport, and mark it for needing a correctness audit. r=hiro
https://hg.mozilla.org/integration/autoland/rev/bcdb4a6fbaf0
Part 4: Update tests and test functions that conflated visual viewport and layout viewport. r=mtigley

Comment 24

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/1e91e19089dd
https://hg.mozilla.org/mozilla-central/rev/9ba5efadaff0
https://hg.mozilla.org/mozilla-central/rev/33d9590eca92
https://hg.mozilla.org/mozilla-central/rev/bcdb4a6fbaf0

Comment 25

[Eric Shepherd [:sheppy]]

Documentation updated:

• Updated the pages about the Window properties innerWidth and innerHeight
• Firefox 73 for developers now mentions this change