1618582 - Dynamic toolbar should not hide when page is not scrollable

Status: RESOLVED FIXED

(GeckoView :: General, defect, P1)

Description

[[ex-Mozilla] Agi Sferro | :agi]

Attachment: testcase.html

Elements with height: 100% don't include the dynamic toolbar. To reproduce load testcase in Fenix Nightly and scroll down so that the dynamic toolbar disappear, observe that the grey overlay doesn't cover the bottom part of the screen.

Comment 1

[[ex-Mozilla] Agi Sferro | :agi]

Attachment: screenshot.jpg

Comment 2

[James Willcox (:snorp) ([e-mail]) (he/him)]

I think this just boils down to Fenix should not be hiding the toolbar when a page is not scrollable. I'm not sure if they even have a way of figuring that out right now. Hiro, does this sound right to you? Otherwise, the height difference would be according to spec, no?

Comment 3

[James Willcox (:snorp) ([e-mail]) (he/him)]

Botond, I think I expected us to be able to handle this by PanZoomController.onTouchEventForResult returning UNHANDLED[1], but we never seem to get that when APZ isn't actually scrolling. Is this broken, or did we never get that working, or what?

[1] https://searchfox.org/mozilla-central/rev/b2ccce862ef38d0d150fcac2b597f7f20091a0c7/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoView.java#735

Comment 4

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

If APZ isn't actually scrolling, then I would expect the result.mStatus here to be nsEventStatus_eIgnore, per the documentation here. So presumably that nsWindow code should be returning UNHANDLED in that scenario?

Comment 5

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

(The return statement at the bottom of the function is the one that would need adjusting, not the one I linked directly to)

Comment 6

[Hiroyuki Ikezoe (:hiro)]

(In reply to James Willcox (:snorp) ([e-mail]) (he/him) from comment #2)

I think this just boils down to Fenix should not be hiding the toolbar when a page is not scrollable. I'm not sure if they even have a way of figuring that out right now. Hiro, does this sound right to you? Otherwise, the height difference would be according to spec, no?

Right, Fenix should not move the toolbar. IIRC, there is an API to tell whether the content is scrollable or not. I will find it out. Maybe the API returns a wrong value in this case. Keeping NI to me.

Comment 7

[James Willcox (:snorp) ([e-mail]) (he/him)]

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

(The return statement at the bottom of the function is the one that would need adjusting, not the one I linked directly to)

Oh, hmm. Yeah, that looks wrong. I've modified it to do this instead, which seems more correct:

    if (result.mHitRegionWithApzAwareListeners) {
      return INPUT_RESULT_HANDLED_CONTENT;
    }

    switch (result.mStatus) {
      case nsEventStatus_eIgnore:
        return INPUT_RESULT_UNHANDLED;
      case nsEventStatus_eConsumeDoDefault:
        return INPUT_RESULT_HANDLED;
      default:
        MOZ_ASSERT_UNREACHABLE("Unexpected nsEventStatus");
    }

If we return UNHANDLED in that case, the current GeckoView.onTouchEvent()[1] code will then return false. That means we won't get any more events for that touch, breaking things like text selection, button clicks, etc. I guess we could just always return true?

Fenix should then be able to assume that UNHANDLED (via onTouchEventForResult[1]) on a touch down event means non-scrollable and could then show the toolbar. I think for simplicity, they should just keep it pinned for the lifetime of that page.

[1] https://searchfox.org/mozilla-central/source/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoView.java#735
[2] https://searchfox.org/mozilla-central/source/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoView.java#746

Comment 8

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

(In reply to James Willcox (:snorp) ([e-mail]) (he/him) from comment #7)

Oh, hmm. Yeah, that looks wrong. I've modified it to do this instead, which seems more correct:

Yeah your edit seems more correct.

If we return UNHANDLED in that case, the current GeckoView.onTouchEvent()[1] code will then return false. That means we won't get any more events for that touch, breaking things like text selection, button clicks, etc. I guess we could just always return true?

Hm, I'm not sure what the exact semantics of returning true/false from View.onTouchEvent is. What about the scenario where the embedding application has a small GeckoView instance embedded inside a larger scrolling thing. And let's say the content inside the GV is non-scrollable, and the user starts a pan gesture by putting their finger down on the GV and dragging. My guess is that if you always return true the GV will "consume" the event and so the "larger scrolling thing" that the GV is embedded inside will not scroll, even though the user might expect it to. Whereas if you returned false it would behave as expected?

At any rate, the current code will be returning true in that scenario anyway so it won't be a regression. /me shrugs

Fenix should then be able to assume that UNHANDLED (via onTouchEventForResult[1]) on a touch down event means non-scrollable and could then show the toolbar. I think for simplicity, they should just keep it pinned for the lifetime of that page.

That seems reasonable.

Comment 9

[James Willcox (:snorp) ([e-mail]) (he/him)]

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

(In reply to James Willcox (:snorp) ([e-mail]) (he/him) from comment #7)

Oh, hmm. Yeah, that looks wrong. I've modified it to do this instead, which seems more correct:

Yeah your edit seems more correct.

If we return UNHANDLED in that case, the current GeckoView.onTouchEvent()[1] code will then return false. That means we won't get any more events for that touch, breaking things like text selection, button clicks, etc. I guess we could just always return true?

Hm, I'm not sure what the exact semantics of returning true/false from View.onTouchEvent is. What about the scenario where the embedding application has a small GeckoView instance embedded inside a larger scrolling thing. And let's say the content inside the GV is non-scrollable, and the user starts a pan gesture by putting their finger down on the GV and dragging. My guess is that if you always return true the GV will "consume" the event and so the "larger scrolling thing" that the GV is embedded inside will not scroll, even though the user might expect it to. Whereas if you returned false it would behave as expected?

For scrolling I don't think the result matters here. There are some containers that do this sort of thing (CoordinatorLayout), but I believe they rely on NestedScrollView for the children, which we already don't support.

Comment 10

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Bug 1618582 - Make `GeckoView#onTouchEventForResult()` return correct values

This also makes GeckoView#onTouchEvent() always return true, because
returning false will cause us to not receive any more events for that
touch. We always want to receive events.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9c17be2a349a
Make `GeckoView#onTouchEventForResult()` return correct values r=kats

Comment 12

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/9c17be2a349a

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Adding a link to the comment by snorp Fenix needs to do something for this issue.

Comment 15

[Chris Peterson [:cpeterson]]

Moving toolbar bugs to the new GeckoView::Toolbar component.