Use layout viewport size (i.e. not ICB, the size expanded by minimums-scale) for window.innerHeight
Categories
(Core :: Layout, defect, P3)
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.
Reporter | ||
Updated•5 years ago
|
Updated•5 years ago
|
Comment 1•5 years ago
|
||
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
Updated•5 years ago
|
Comment 2•5 years ago
|
||
(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
.
Comment 3•5 years ago
|
||
(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=278204951Possibly 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, likedocument.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
Comment 4•5 years ago
|
||
(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.
Updated•5 years ago
|
Comment 5•5 years ago
|
||
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.
Comment 6•5 years ago
|
||
FWIW here is a reasonable try result;
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f77ac89e97e267972e03fc3560ab5d04ed289587
Updated•4 years ago
|
Updated•4 years ago
|
Updated•4 years ago
|
Updated•2 years ago
|
Description
•