1663000 - Return reasonable INPUT_RESULT value depending on content's height/event handlig results

Status: RESOLVED FIXED

(GeckoView :: IME, defect, P2)

Description

[Petru-Mugurel Lingurar [:petru]]

Following the discussions from Fenix - https://github.com/mozilla-mobile/fenix/issues/13797

There is an issue where pages have a height bigger than (window - toolbar) but smaller than window (ending underneath the toolbar) and so the page will not be considered as scrollble -> the dynamic toolbar won't be animated away to allow viewing / interacting with the very bottom of the page.

 
(Botond Ballo (:botond) from https://github.com/mozilla-mobile/fenix/issues/13797#issuecomment-685974024)

My suggestion for how to handle this case, as mentioned, is to size the viewport to the (window - toolbar) height. The resulting behaviour would depend on the page:

• If the page has a fixed content height (e.g. a GitHub issues list with a fixed number of issues to display) that just happens to fall into this small range, then the page would become a bit scrollable, like you said.
• If the page has content height == viewport height, then the content height would shrink. This covers cases like the ublock settings page (where an iframe's viewport takes up the page viewport; the iframe would remain scrollable with the smaller viewport, as desired), as well as cases like a mapping or drawing application where the canvas size would simply be reduced by the height of the toolbar.

 
For a minimal testcase - https://codepen.io/mattok/full/XWdeQZY

Comment 1

[Hiroyuki Ikezoe (:hiro)]

What I observed Chrome's behavior is; Chrome hides the toolbar when the iframe's scroll position reaches to the end. That's what Botond told me on the GeckoView Matrix channel. I think it's worth a shot!

Comment 2

[Hiroyuki Ikezoe (:hiro)]

I am going to re-use this bug to make our INPUT_RESULT more reasonable.

I tested various cases on Chrome, so far behaviors what I've observed on Chrome;

a) preventDefault() always prevents moving the toolbar
Decent canvas like applications use it with height: 100% with
overflow:hidden on the root
b) Listening touchstart event doesn't seem to prevent moving the toolbar
c) Without (touchstart) event handlers moving the toolbar depends on whether
the toolbar convers some of contents or not
d) If all ancestor scrollable elements reach to the bottom edge of each scroll
position, and if there is room being pushed out of the screen by the top
dynamic toolbar, then the toolbar starts moving
One caveat in this case is once after the dynamic toolbar has been completely
hidden (and left the touching finger), the toolbar will never re-appear until
the scroll potion reached to the top of the iframe, this behavior is quite
irritating.

What we need to replicate the behaviors are (I can think of);

Nothing we need for a)

For b)
b-1) Send the intrinsic composition size to the compositor
b-2) Add a function to tell whether the root composition size is taller than
the intrinsic size so that we can tell whether the dynamic toolbar needs to
move
b-3) InputBlockCallback needs to return tri-state;

1. The event is handled by a non-root APZC
2. The event is handled by the root APZC
   (NOTE: this is including 100vh case, i.e. the root APZC is not scrollable,
   but the dynamic toolbar needs to move)
3. The event is NOT handled at all (i.e. there is no scrollable content)

For c)
Most cases work fine, the only one case doesn't work is "non-scrollable contents
covered by the dynamic toolbar". We need b-1 and b-2 at least. I wonder whether
we probably can simply add the b-2 function check in addition to along with this
ArePointerEventsConsumable check as if it's consumed in the root APZC (so
that it can properly return either nsEventStatus_eConsumeDoDefault or
nsEventStatus_eConsumeNoDefault I think)

For d)

1. Add a function to tell this situation
2. With event handlers, return the event handled by the root APZC in
   the same place in InputQueue::ProcessQueue() for b-3).
3. Without event handlers return as if the event handled by the root APZC in the
   place for c).

Here is a try result with these changes (though it looks like there is a race condition that one of tests added with these changes fails on non E10S)

Also there are still things need to be discussed about approaches how to fix it, for example the place where the intrinsic composition size is stored, but it can be addressed during reviews, so I will post the changes for reviews now.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1663000 - Change 33vh value to 33% in touch.html and rename touchEventForResult to touchEventForResultWithStaticToolbar. r?#geckoview-reviewers!,botond!

touchEventForResult is the only one test using touch.html and it doesn't use
setDynamicToolbarMaxHeight, which means the test is supposed to work with
static toolbar, so with this change it gets clearer what the test does.
(and we are going to add a bunch of tests with dynamic toolbar)

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1663000 - Send the composition size without the dynamic toolbar height to the compositor. r?botond

Also add AsyncPanZoomController::IsRootCompositionHeightIsGreaterThanInsrintic
to tell whether the root APZC is not scrollable but there is room covered by
the dynamic toolbar so that we can tell GeckoView to move the dynamic toolbar
in such cases.

Depends on D93913

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1663000 - Return UNHANDLED even if there's no event handler which uses preventDefault() and the root content is not covered by the dynamic toolbar. r?botond

This change has a couple of tests but they are for scrollable/non-scrollable
contents, they are not for non-scrollable contents but covered by the dynamic
toolbar cases, e.g. 100vh body element. Those cases will be introduced in the
subsequent change.

Depends on D93914

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1663000 - Add a bunch of tests including non-scrollable contents but covered by the dynamic toolbar. r?#geckoview-reviewers!,botond!

• The test cases cover with/without event handler, and with preventDefault in
  the event handler
• The non-scrollable contents but covered by the dynamic toolbar cases don't
  pass without the event handler now (the cases with the event handler have been
  fixed by the previous change)
• There is a test case which don't pass at all right now, that is a case
  (iframe_98vh_scrollable.html) that after scrolled at the bottom edge of an
  iframe

Depends on D93915

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1663000 - Handle the event as if it's consumed in the root APZC in InputQueue::ReceiveTouchInput() if there's contents covered by the dynamic toolbar.

Depends on D93916

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1663000 - Ensure the dynamic toolbar can be movied via scroll handoff from a subframe. r?botond

Depends on D93917

Comment 9

[Hiroyuki Ikezoe (:hiro)]

I've confirmed posted changes fix bug 1650644, bug 1662823 and bug 1649865. (there may be more I am not aware of).

Comment 10

[Kartikaya Gupta (email:[e-mail])]

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

I tested various cases on Chrome, so far behaviors what I've observed on Chrome;

a) preventDefault() always prevents moving the toolbar
Decent canvas like applications use it with height: 100% with
overflow:hidden on the root

Keep in mind that last time I checked Chrome still suffered from the spoofing bug, where web-content can prevent the browser toolbar from showing up and instead drawing it's own fake browser toolbar in its place. So we might not want to copy Chrome's behaviour exactly in this respect. That bug is something we've always tried hard to avoid.

(Edited to add: I didn't fully read and process your whole comment yet but just wanted to mention this since from point a it seemed like we would be exposing ourselves to this bug)

Comment 11

[Hiroyuki Ikezoe (:hiro)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #10)

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

I tested various cases on Chrome, so far behaviors what I've observed on Chrome;

a) preventDefault() always prevents moving the toolbar
Decent canvas like applications use it with height: 100% with
overflow:hidden on the root

Keep in mind that last time I checked Chrome still suffered from the spoofing bug, where web-content can prevent the browser toolbar from showing up and instead drawing it's own fake browser toolbar in its place. So we might not want to copy Chrome's behaviour exactly in this respect. That bug is something we've always tried hard to avoid.

That's super helpful info! Thanks! Though I have never thought about such spoofing, indeed Chrome and current Fenix can be spoofed. (I was about to upload a sample but wasn't).

I have no idea to avoid such spoofing right now unfortunately but will figure out.

Comment 12

[Hiroyuki Ikezoe (:hiro)]

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

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #10)

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

I tested various cases on Chrome, so far behaviors what I've observed on Chrome;

a) preventDefault() always prevents moving the toolbar
Decent canvas like applications use it with height: 100% with
overflow:hidden on the root

Keep in mind that last time I checked Chrome still suffered from the spoofing bug, where web-content can prevent the browser toolbar from showing up and instead drawing it's own fake browser toolbar in its place. So we might not want to copy Chrome's behaviour exactly in this respect. That bug is something we've always tried hard to avoid.

That's super helpful info! Thanks! Though I have never thought about such spoofing, indeed Chrome and current Fenix can be spoofed. (I was about to upload a sample but wasn't).

The current Fenix looks ok, the dynamic toolbar pops up even after the toolbar was completely hidden and and the content uses preventDefault(). And I've confirmed with those changes in this bug the behavior hasn't been changed.

Comment 13

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/jobs?repo=try&revision=04e87ce4fe3d6651ca787e8ccec69301286c7dd9

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0a53caf5f3fd
Change 33vh value to 33% in touch.html and rename touchEventForResult to touchEventForResultWithStaticToolbar. r=botond,geckoview-reviewers,snorp
https://hg.mozilla.org/integration/autoland/rev/55c6846fe8e0
Send the composition size without the dynamic toolbar height to the compositor. r=botond
https://hg.mozilla.org/integration/autoland/rev/fbb2c1d1159d
Return UNHANDLED even if there's no event handler which uses preventDefault() and the root content is not covered by the dynamic toolbar. r=botond,geckoview-reviewers,snorp
https://hg.mozilla.org/integration/autoland/rev/f7f190aa7cb0
Add a bunch of tests including non-scrollable contents but covered by the dynamic toolbar. r=botond,geckoview-reviewers,snorp
https://hg.mozilla.org/integration/autoland/rev/3695289c1eb9
Handle the event as if it's consumed in the root APZC in InputQueue::ReceiveTouchInput() if there's contents covered by the dynamic toolbar. r=geckoview-reviewers,botond,snorp
https://hg.mozilla.org/integration/autoland/rev/dfc2063b5c58
Ensure the dynamic toolbar can be movied via scroll handoff from a subframe. r=botond,geckoview-reviewers,snorp

Comment 15

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/0a53caf5f3fd
https://hg.mozilla.org/mozilla-central/rev/55c6846fe8e0
https://hg.mozilla.org/mozilla-central/rev/fbb2c1d1159d
https://hg.mozilla.org/mozilla-central/rev/f7f190aa7cb0
https://hg.mozilla.org/mozilla-central/rev/3695289c1eb9
https://hg.mozilla.org/mozilla-central/rev/dfc2063b5c58

Comment 16

[lasdka88+mozbugs]

Attachment: Minimal flex box test case

Expected behaviour across all Firefox versions for this minimal flex box test case (which is a very minor variation on the test case provided here: https://github.com/mozilla-mobile/fenix/issues/13797#issuecomment-720049301):

• Header(green) always displayed at top of viewport
• Footer (green) always displayed at bottom of viewport
• Main (grey) region that is scrollable if required to show all (70) items in the list.
• Nothing (no other background colours, such as white) visible at any time, regardless of scroll.

Comment 17

[lasdka88+mozbugs]

I note that this is marked as fixed and closed.

This bug is marked as addressing https://github.com/mozilla-mobile/fenix/issues/13797#issuecomment-720049301 . I have attached (a minor variation on) a minimal test case from that issue: attachment 9187912 [details]. Considering the behaviour on the latest Firefox for Android, I can see that the behaviour for this test case has changed. However, it is far from what I could consider to be fixed. Behaviour is as expected (see the attachment description) on Firefox for desktop and in the responsive preview within the developer tools. Multiple major issues remain on Firefox for Android (nightly), however:

With the address bar at the bottom:

• Issue 1: When page first loads, the footer which should always be visible at the bottom of the visible viewport, is hidden under the address bar. This should never happen (unless, perhaps, only temporarily as part of a transition while the address bar moves out the way or returns).
• Issue 2: Scrolling down to the bottom of the list reveals the footer at the end of it, momentarily in the correct place at the bottom of the visible viewport. However, the address bar then disappears leaving white space where the address bar, and the content and footer, which should be fixed to the bottom of the visible viewport, suspended above the bottom of the page. The footer should always be positioned at the bottom of the viewport. If the viewport changes, because the address bar moves out the way and makes it bigger, the footer should move too. There should never be any white background visible underneath the footer.
• It is not possible to make the address bar come back without swiping down. However, swiping down does not behave correctly/as expected:
• Issue 3: Dragging down reloads the page instead of scrolling the central 'main' element. The page is reloaded leaving the very bottom of the list visible in the main region (i.e. list scrolled to the bottom), so it becomes impossible (without knowing to clear the page from Firefox's cache) to scroll back up to view the top of the list. Part of the page becomes effectively-permanently hidden from a normal user's perspective. It would ordinarily be expected that swiping down would scroll you back up the list towards the top of the list, and that only once no more scrolling is possible would a swipe down potentially trigger a page reload.

Behaviour with the address bar at the top is just as problematic, also with conflict/race-condition between reloading and scrolling when dragging your finger down the screen, white space left underneath the content etc.

Comment 18

[lasdka88+mozbugs]

Additional observation: With the address bar at the top, I've just managed to get myself into a position somehow whereby the address bar is hidden and it is not possible make it come back at all. The only thing I can make happen is reload the page with the list scrolled to the bottom.

Comment 19

[lasdka88+mozbugs]

For the record, the behaviour described in my last two comments relates to: Nightly 201114 17:01 (Build #2015775625), AC: 66.0.20201109143146, bb44b70ee1, GV: 84.0a1-20201109095222, AS: 63.0.0

Comment 20

[Botond Ballo [:botond] [away until Oct 19]]

lasdka, thank you for posting this additional testcase. For tracking purposes, could I ask you to post it in a new bug please?

The issue in comment 18 sounds particularly concerning. If you've found steps that reproduce it reliably, I think that merits its own bug report as well.

Comment 21

[Hiroyuki Ikezoe (:hiro)]

Yes, please file new bugs for the remaining issues, especially issue 3 sounds like one of issues Markus told me yesterday.

(In reply to lasdka88+mozbugs from comment #17)

• Issue 1: When page first loads, the footer which should always be visible at the bottom of the visible viewport, is hidden under the address bar. This should never happen (unless, perhaps, only temporarily as part of a transition while the address bar moves out the way or returns).

I can understand why you think it's a wrong behavior, but this is a behavior on all major mobile browsers. This document written by a Chrome developer is quite helpful to understand the behavior.

• Issue 2: Scrolling down to the bottom of the list reveals the footer at the end of it, momentarily in the correct place at the bottom of the visible viewport. However, the address bar then disappears leaving white space where the address bar, and the content and footer, which should be fixed to the bottom of the visible viewport, suspended above the bottom of the page. The footer should always be positioned at the bottom of the viewport. If the viewport changes, because the address bar moves out the way and makes it bigger, the footer should move too. There should never be any white background visible underneath the footer.

This is another variant of bug 1673517.

• It is not possible to make the address bar come back without swiping down. However, swiping down does not behave correctly/as expected:

I suppose this only happens when the toolbar is at top, right? Then I think https://github.com/mozilla-mobile/android-components/issues/7718 is the root cause.

• Issue 3: Dragging down reloads the page instead of scrolling the central 'main' element. The page is reloaded leaving the very bottom of the list visible in the main region (i.e. list scrolled to the bottom), so it becomes impossible (without knowing to clear the page from Firefox's cache) to scroll back up to view the top of the list. Part of the page becomes effectively-permanently hidden from a normal user's perspective. It would ordinarily be expected that swiping down would scroll you back up the list towards the top of the list, and that only once no more scrolling is possible would a swipe down potentially trigger a page reload.

I can't reproduce this, so I don't yet understand where the underlying issue is, whether it's in GeckoView or in Fenix, so it's worth filing a new bug for this issue.

Thanks!

Comment 22

[lasdka88+mozbugs]

Hiroyuki-san, in response to your comment on Issue 1, thank you for the reference to that document. Personally, I'm not interested in Firefox copying Chrome or other browsers just for the sake of it (although I do understand that consistency between browser can be a very good and helpful thing). While Chrome appear to have this implemented much better than Firefox at the moment (since I can't reproduce any of the other issues I reported above), I'm not sure that Chrome has got it right. Indeed, Chrome appear to acknowledge this in the document you cited but chose to do something different for compatibility with Firefox and Safari...

It might be conceptually nice to make this relative to the proposed-static ICB (i.e. the smallest possible viewable area) but both Safari and Firefox already use the opposite. Web authors' lives are difficult enought; in the name of compatibility, I propose we do the same thing as Safari and Firefox, and make vh units relative to the largest possible viewable area.

So, if cross-browser consistency is important, one possibility could be to coordinate a fix/change with both Chrome and Safari. Of importance to me here are:

• standards compliance (because that's what I refer to as a developer); and
• consistent behaviour between desktop and mobile versions of Firefox; and
• as a developer, meeting developers' reasonable(?) expectations.

Regarding standards compliance. The document you cited refers to a "vh viewport". I cannot find a reference to any such viewport in the standard. The latest draft of the CSS standard (and CSS2) says of vh units that (emphasis mine):

The viewport-percentage lengths are relative to the size of the initial containing block. When the height or width of the initial containing block is changed, they are scaled accordingly.

and for initial containing block it says (emphasis mine):

The containing block in which the root element lives is a rectangle called the initial containing block. For continuous media, it has the dimensions of the viewport and is anchored at the canvas origin ...

and for viewport it says (emphasis mine):

User agents for continuous media generally offer users a viewport (a window or other viewing area on the screen) through which users consult a document.

As such, as a developer I expect 100vh to represent 100% of the viewport through which the user is consulting (i.e. viewing - the clue in in the name I think) the document. 'Under the address bar' is not part of the visible viewport and nor is 'beneath the bottom of the page'. The address bar is different to a keyboard or tool to assist with form inputs that appears temporarily when interacting with a specific element on the page and then disappears again. It is intrinsic to the browser, and it is there when Firefox starts and it is there when the page loads. If Firefox or the user wishes to hide it, that is fine, but it is the hiding that is temporary, not the showing, and hiding it changes the visible viewport and my interpretation of the standard is that it ought to result in a corresponding change to the vh units. If, on the desktop version of Firefox, the page were to load with the bottom part of the page cut off, and it could only be revealed by first hiding the toolbar - which you could achieve by hiding it or scrolling to the very bottom for it to suddenly disappear and shift everything up to remove the cut off part, that would be extremely odd. In my opinion it's just as odd on Firefox for Android too. A possible exception could be made if the address bar hid itself immediately upon any interaction with the page (tap, scroll, etc.) - such that that was the 'default' view, and that the user had to do something special to make it come back again. However, that is not the case here: The user has to scroll right to the bottom of the page and only then does the address bar disappear to reveal the hidden part of the page.

Regarding consistent behaviour between desktop and mobile versions of Firefox: As a developer, if I have to run some javascript to detect whether the user is running the mobile or desktop version of Android and then use it to load different styles accordingly or to move elements into the correct position after the fact, then something is incredibly broken, and that appears to me to be the case here. On desktop Firefox, 100vh means the whole visible viewport. If I hide or show a toolbar, the viewport changes. How can I reproduce the desktop behaviour (elements permanently visible at the top and bottom of visible viewport with central scrollable region) on Android? In my opinion, the same behaviour is required on Firefox for Android too.

I should add that I've been waiting literally for YEARS (maybe even a decade) for some simple pure CSS way of achieving a holy grail layout. Along come viewport units and flex box and solved it all - and now it seems that's all being undone because viewport is being redefined to something that is not actually in view(!) on mobile devices only, and it is incredibly frustrating.

Regarding "It is not possible to make the address bar come back without swiping down. However, swiping down does not behave correctly/as expected", you are correct that I made a small mistake here: The address bar only fails to re-appear when it is positioned at the top; it does re-appear when it is positioned at the bottom. (Issue 3 is still correct though.)

Comment 23

[lasdka88+mozbugs]

I've created two new bugs: bug 1677413 and bug 1677412. I haven't separately reported the problem with whitespace at the bottom of the page, given that it appears to be captured by bug 1673517.

Comment 24

[Hiroyuki Ikezoe (:hiro)]

Welcome to the weird web world on mobile. :) I do understand your frustration but anyway, as Botond mentioned, commenting in an already-closed bug is not a good idea basically. If you want to change the vh unit behavior, you should open a spec issue in https://github.com/w3c/csswg-drafts instead.

Some additional info I know of;

1. There is an ongoing discussion to change the spec description about vh units and introduce a new CSS unit which is not including dynamic toolbar height, it's https://github.com/w3c/csswg-drafts/issues/4329
2. I've heard that the Chrome developer who wrote the document I posted argued Safari developers to change the behavior on Safari, but ended up changing Chrome's behavior instead
3. We've changed our behavior since the initial Fenix release because we've gotten a bunch of web compat reports (bug 1123938 is a tracker bug for the change)
4. As you can see in the tracker bug in 3), there remain a bunch of issues
5. The "pull-to-refresh" feature introduced another complexity (headache for me) we've tracked it in https://github.com/mozilla-mobile/fenix/issues/9766

Comment 25

[Chris Peterson [:cpeterson]]

Moving some input bugs to the new GeckoView::IME component.