Open Bug 1514429 Opened 10 months ago Updated 9 days ago

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

Categories

(Core :: Layout, enhancement, P3)

enhancement

Tracking

()

Tracking Status
firefox66 --- affected

People

(Reporter: botond, Unassigned)

References

(Blocks 3 open bugs)

Details

(Keywords: dev-doc-needed, site-compat, Whiteboard: [geckoview:p3])

Attachments

(1 file)

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
Priority: -- → P3
See Also: → 1126346

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

Flags: needinfo?(botond)
Blocks: 1209426

(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

Flags: needinfo?(botond)

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

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

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

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

Blocks: 1523541
Summary: window.inner{Width,Height} should return the dimensions of the layout viewport → window.innerWidth/innerHeight should return the dimensions of the layout viewport
Whiteboard: [geckoview:p3]
You need to log in before you can comment on or make changes to this bug.