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

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[Brad Werth [:bradwerth]]

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.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

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

Comment 2

[Botond Ballo [:botond] [away until Jun 24]]

(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

[Hiroyuki Ikezoe (:hiro)]

(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

Comment 4

[Hiroyuki Ikezoe (:hiro)]

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

Comment 5

[Hiroyuki Ikezoe (:hiro)]

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

Comment 6

[Hiroyuki Ikezoe (:hiro)]

FWIW here is a reasonable try result;
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f77ac89e97e267972e03fc3560ab5d04ed289587

Comment 7

[Markus Stange [:mstange]]

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

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.

Hiro, do you remember why fixing this test failure requires innerWidth/innerHeight to be non-writable? I can't find a test named test_innerWidthHeight_script.html in today's mozilla-central, so I wonder whether we can try this again without fixing the dependencies of bug 1595962.

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Though I wasn't aware of the fact that the test file has been removed in bug 1837953, the test file is actually a test case ensure those attributes are writable.

So yeah, you are right, now it's time to fix this bug.

Comment 9

[Botond Ballo [:botond] [away until Jun 24]]

Raising priority and marking as webcompat:platform-bug due to this causing bug 1944725.

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1598487 - Use document.documentElement.clientHeight rather than window.innerHeight in helper_doubletap_zoom_hscrollable2.html. r?tnikkel

The document in question gets scaled down on mobile so with the spec
compliant window.innerHeight (comming in the next commit), the value
would be much bigger than it's supposed to be.

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1598487 - Make window.inner{Height,Width} return the proper value on for documents where the minimum-scale is applied. r?#layout-reviewers!,botond!

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1598487 - Add `scrollbar-width:none` to avoid scrollbar area affecting window.innerWidth. r?emilio

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1598487 - Make window.inner{Height,Width} return the proper value on for documents where the minimum-scale is applied. r?#layout-reviewers!,botond!

Comment 14

[Hiroyuki Ikezoe (:hiro)]

Attachment: A test case to confirm window.innerWidth flushes layout

For future reference, I used this test to tell whether Chrome flushes layout on window.innerWidth call. Actually Chrome flushes layout.

Comment 15

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1598487 - Use documentElement.client{Width,Height} in testViewportZoomWidthAndHeight. r?botond,bradwerth

And specify <!DOCTYPE html> in relevant test cases to use standars mode.

The proper window.inner{Width,Height} values depend on the initial zoom
value, rather documentElement.client{Width,Height} are independent from
the zoom value.

Note that documentElement.clientHeight on quirks mode is diffrent from
the one on standards mode. E.g. if the document is zoomed by 0.5x, then
documentElement.clientHeight on quirks mode is twice the value of the
one on standards mode. I've confirmed the behavior is same in Chrome's
RDM.

Comment 16

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1598487 - Use documentElement.client{Width,Height} in PanZoomControllerTest.kt r?botond

And specify <!DOCTYPE html> in scroll.html.

Comment 17

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1598487 - Specify a meta viewport tag to avoid scaling down the content. r?#layout-reviewers

There are some wpts had no meta viewport tag and have at least either
one of window.innerWidth or window.innerHeight call. We hadn't flushed
any pending layout changes on either window.innerWidth or
window.innerHeight call, but with flushing layout these tests start
failing since the rendering results are not what each test is supposed
to be, i.e. the content is scaled down. In other words these tests had
been accidentally passing on mobile environments due to missing meta
viewport tags. With the proper meta viewport tag, some wpts start
passing on mobile environments.

Comment 18

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1598487 - Add a "minimum-scale=1" meta viewport to both of the reference and the test case for test_clip_box_partially_visible. r?#webdriver-reviewers

The test enlarges the target element width to window.innerWidth+100px,
thus without minimum-scale=1 the document gets scaled down further to
fit the enlarged element into the view. Thus the element becomes fully
visible, is not partially visible what the the test supposed to be.

Comment 19

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1598487 - Add a "minimum-scale=1" meta viewport into get_actions_origin_page(). r?#webdriver-reviewers

Similar to the previous change but for
test_element_in_view_center_point_partly_visible using
get_actions_origin_page().

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/e69ee818f97d
https://hg.mozilla.org/integration/autoland/rev/b4ed747248f7
Use document.documentElement.clientHeight rather than window.innerHeight in helper_doubletap_zoom_hscrollable2.html. r=tnikkel
https://github.com/mozilla-firefox/firefox/commit/539615e70271
https://hg.mozilla.org/integration/autoland/rev/92e75caacf49
Add `scrollbar-width:none` to avoid scrollbar area affecting window.innerWidth. r=emilio
https://github.com/mozilla-firefox/firefox/commit/b86d915a0758
https://hg.mozilla.org/integration/autoland/rev/900cad41b5f8
Use documentElement.client{Width,Height} in testViewportZoomWidthAndHeight. r=botond,bradwerth,devtools-reviewers,ochameau
https://github.com/mozilla-firefox/firefox/commit/ca7531474e57
https://hg.mozilla.org/integration/autoland/rev/ba26979b6ef4
Use documentElement.client{Width,Height} in PanZoomControllerTest.kt r=botond,geckoview-reviewers
https://github.com/mozilla-firefox/firefox/commit/86c53ef80682
https://hg.mozilla.org/integration/autoland/rev/9cc1e26808dd
Specify a meta viewport tag to avoid scaling down the content. r=layout-reviewers,dshin
https://github.com/mozilla-firefox/firefox/commit/426333bc1b1b
https://hg.mozilla.org/integration/autoland/rev/b8cd45b2083b
Add a "minimum-scale=1" meta viewport to both of the reference and the test case for test_clip_box_partially_visible. r=webdriver-reviewers,jdescottes
https://github.com/mozilla-firefox/firefox/commit/4ef0efd4c389
https://hg.mozilla.org/integration/autoland/rev/7865be80143d
Add a "minimum-scale=1" meta viewport into get_actions_origin_page(). r=webdriver-reviewers,jdescottes
https://github.com/mozilla-firefox/firefox/commit/8ebbc62f8cb9
https://hg.mozilla.org/integration/autoland/rev/867103d05e78
Make window.inner{Height,Width} return the proper value on for documents where the minimum-scale is applied. r=botond,layout-reviewers,emilio

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/b3d0851558ef
https://hg.mozilla.org/integration/autoland/rev/66943e58c0b6
Revert "Bug 1598487 - Make window.inner{Height,Width} return the proper value on for documents where the minimum-scale is applied. r=botond,layout-reviewers,emilio" for causing mochitest failures on test_group_double_tap_zoom-2.html

Comment 22

[Cristina Horotan [:chorotan]]

Backed out for causing mochitest failures on test_group_double_tap_zoom-2.html

Backout link

Push with failures

Failure log mochitest

Comment 23

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 24

[Hiroyuki Ikezoe (:hiro)]

Dumped window.innerHeight and document.documentElement.clientHeight on a mac in a try run. The values are;

window.innerHeight: 864
document.documentElement.clientHeight: 849

It looks to me that our Mac on CIs enables "showing always scrollbars options".

With this small difference helper_doubletap_zoom_hscrollbar2.html fails on Mac.

I did add html {scrollbar-width: none} in D262531 to avoid this difference.

Comment 25

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/54892 for changes under testing/web-platform/tests

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/59b70c6312fc
https://hg.mozilla.org/mozilla-central/rev/3245f080dd68
Use document.documentElement.clientHeight rather than window.innerHeight in helper_doubletap_zoom_hscrollable2.html. r=tnikkel
https://github.com/mozilla-firefox/firefox/commit/46420825a9c7
https://hg.mozilla.org/mozilla-central/rev/d884c11dedf1
Add `scrollbar-width:none` to avoid scrollbar area affecting window.innerWidth. r=emilio
https://github.com/mozilla-firefox/firefox/commit/29dcbff43e3a
https://hg.mozilla.org/mozilla-central/rev/936685abe38e
Use documentElement.client{Width,Height} in testViewportZoomWidthAndHeight. r=botond,bradwerth,devtools-reviewers,ochameau
https://github.com/mozilla-firefox/firefox/commit/927b06b18ee8
https://hg.mozilla.org/mozilla-central/rev/06e4e355e044
Use documentElement.client{Width,Height} in PanZoomControllerTest.kt r=botond,geckoview-reviewers
https://github.com/mozilla-firefox/firefox/commit/6dd1aa0edc92
https://hg.mozilla.org/mozilla-central/rev/6fb777dd5dde
Specify a meta viewport tag to avoid scaling down the content. r=layout-reviewers,dshin
https://github.com/mozilla-firefox/firefox/commit/3af910c57aa8
https://hg.mozilla.org/mozilla-central/rev/8677fea330b9
Add a "minimum-scale=1" meta viewport to both of the reference and the test case for test_clip_box_partially_visible. r=webdriver-reviewers,jdescottes
https://github.com/mozilla-firefox/firefox/commit/99eb4877d2a4
https://hg.mozilla.org/mozilla-central/rev/d7c3586418d7
Add a "minimum-scale=1" meta viewport into get_actions_origin_page(). r=webdriver-reviewers,jdescottes
https://github.com/mozilla-firefox/firefox/commit/302639e59995
https://hg.mozilla.org/mozilla-central/rev/253563bc6f5a
Make window.inner{Height,Width} return the proper value on for documents where the minimum-scale is applied. r=botond,layout-reviewers,emilio

Comment 27

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/3245f080dd68
https://hg.mozilla.org/mozilla-central/rev/d884c11dedf1
https://hg.mozilla.org/mozilla-central/rev/936685abe38e
https://hg.mozilla.org/mozilla-central/rev/06e4e355e044
https://hg.mozilla.org/mozilla-central/rev/6fb777dd5dde
https://hg.mozilla.org/mozilla-central/rev/8677fea330b9
https://hg.mozilla.org/mozilla-central/rev/d7c3586418d7
https://hg.mozilla.org/mozilla-central/rev/253563bc6f5a

Comment 28

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot