Open Bug 1598487 Opened 5 years ago Updated 2 years ago

Use layout viewport size (i.e. not ICB, the size expanded by minimums-scale) for window.innerHeight

Categories

(Core :: Layout, defect, P3)

defect

Tracking

()

People

(Reporter: bradwerth, Unassigned)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Bug 1514429 disabled this test. The comparisons between visualViewport, layout viewport (window.innerHeight), documentElement.scrollHeight, and documentElement.getBoundingClientRect all need to be audited to see if we still have some incorrect behavior.

Summary: Fix and re-enable → Fix and re-enable dom/base/test/test_viewport_metrics_on_landscape_content.html
Priority: -- → P3

I've realized the reason why the test fails. That's because we return nsPresContext::mVisibleArea in nsGlobalWindowOuter::GetInnerSize but apparently the layout viewport of the test case is expanded by 'minimum-scale=0.5' in meta viewport tag so we need to return the layout viewport instead of nsPresContext::mVisibleArea.

I did push a try with returning the layout viewport from the function, but unfortunately it causes a bunch of test failures. Probably I am missing something.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5e02d9dc6f7e5c46aaf7fbe48ca52d57a472df2f&selectedJob=278204951

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

I've realized the reason why the test fails. That's because we return nsPresContext::mVisibleArea in nsGlobalWindowOuter::GetInnerSize but apparently the layout viewport of the test case is expanded by 'minimum-scale=0.5' in meta viewport tag so we need to return the layout viewport instead of nsPresContext::mVisibleArea.

Ah, good point! I forgot about this when reviewing bug 1514429...

I did push a try with returning the layout viewport from the function, but unfortunately it causes a bunch of test failures. Probably I am missing something.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5e02d9dc6f7e5c46aaf7fbe48ca52d57a472df2f&selectedJob=278204951

Possibly there are internal uses of innerWidth/Height which are expecting the ICB size and not the layout viewport size, which may need to be modified to obtain the ICB size some other way, like document.scrollingElement.clientWidth/Height.

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

I did push a try with returning the layout viewport from the function, but unfortunately it causes a bunch of test failures. Probably I am missing something.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5e02d9dc6f7e5c46aaf7fbe48ca52d57a472df2f&selectedJob=278204951

Possibly there are internal uses of innerWidth/Height which are expecting the ICB size and not the layout viewport size, which may need to be modified to obtain the ICB size some other way, like document.scrollingElement.clientWidth/Height.

There probably are. A big mistake I missed is that PresShell::GetViewportSize() is only meaningful on root scrollable shell, presumably the function should return nsPresContext::mVisibleArea for fallback cases.

I did push another try now. We can probably see more reasonable results (failures) there.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=db6f6b06ff279b558a68b7bac52ccfea66f17e53

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

I did push another try now. We can probably see more reasonable results (failures) there.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=db6f6b06ff279b558a68b7bac52ccfea66f17e53

The change in this try was wrong again. It clobbers the layout viewport size...

Here is a try with more reasonable change (that I believe)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6126be183576a9cb9ecddb802927d39fa0a9684b

Still I am probably missing something. I will revisit this issue after bug 1523541.

Summary: Fix and re-enable dom/base/test/test_viewport_metrics_on_landscape_content.html → Use layout viewport size (i.e. not ICB, the size expanded by minimums-cale) for window.innerHeight

A big thing I missed is that we just need to use the layout viewport only if it's in the root content document.

Here is a new try, the result looks reasonable, most of failed tests need to be addressed by this window.innerHeight change. One exception is test_innerWidthHeight_script.html, we probably need to fix bug 1595962 first.

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

Depends on: 1601933
Depends on: 1595962
Summary: Use layout viewport size (i.e. not ICB, the size expanded by minimums-cale) for window.innerHeight → Use layout viewport size (i.e. not ICB, the size expanded by minimums-scale) for window.innerHeight
Severity: normal normal → S3 S3
You need to log in before you can comment on or make changes to this bug.