1252802 - Web page scrolls when dragging a single caret up or down on an input field

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Text Selection, defect)

Description

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Steps to reproduce:
1. Enter some text into an input field on www.google.com
2. Single tap on the input field to show caret.
3. Press on the single caret to drag up or down.

Expected results:
The page does not scroll.

Actual results:
The page scrolls.

FWIW, I cannot reproduce this on B2G. Also the page does not scroll when dragging one of the carets when there's a selection. APZ should be blocked by the dummy touch listener in [1]. 

[1] https://dxr.mozilla.org/mozilla-central/rev/eb25b90a05c194bfd4f498ff3ffee7440f85f1cd/layout/base/AccessibleCaret.cpp#203-204

Comment 1

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

The touch listener also needs to do a prevent default on the event, but it doesn't look like it does [1]. I'm not sure how this is working on b2g, unless there is another listener doing the prevent default somewhere.

[1] https://dxr.mozilla.org/mozilla-central/rev/eb25b90a05c194bfd4f498ff3ffee7440f85f1cd/layout/base/AccessibleCaret.h#188

Comment 2

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Strange. I follow nsRangeFrame.h in [1], and it works for ages.

I think I figure out why. The problem is not in AccessibleCaret nor in APZ. It's because Fennec has a "Full-screen browsing" mode in Settings > General. When you scroll up the page, it hides the URL bar. It just "move" the whole content regardless you're scrolling up the page or dragging the caret. So if we turn off the mode, the issue is gone.

I guess when actionbar is shown, we probably don't hide the URL bar. That's why dragging one of the carets on a selection highlight behaves normally as I said in Description.

[1] https://dxr.mozilla.org/mozilla-central/source/layout/forms/nsRangeFrame.h#203-206

Comment 3

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

When we press or release a caret, AccessibleCaret will fire a CaretStateChangeEvent with reason "presscaret" and "releasecaret", respectively [1]. We might try to use the two reasons to disable the "Full-screen browsing" mode temporarily in ActionBarHandler.js [2] when dragging the caret.

We need a front-end expert to help on this. Mark, do you know how to do this? Or someone who knows full-screen browsing mode?

[1] https://dxr.mozilla.org/mozilla-central/source/dom/webidl/CaretStateChangedEvent.webidl#12-13
[2] https://dxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/ActionBarHandler.js?from=ActionBarHandler.js#39

Comment 4

[Mark Capella [:capella]]

Clearing ?ni after our discussion in #mobile.

Recapping my thoughts, I'm not seeing dynamic toggling of the fullscreen browsing setting as being a reasonable solution.

I do think some touch events may be simply targeted around the caret to the APZ layer causing scrolling, perhaps by someone like me with a fat finger. Or as I believe kats suggests in comment #1, some events are being passed through the caret without being defaultPrevented.

As this seems related to the dynamicToolbar visibility during animation in and out, there might be a race in there between layout re-draws, window sizing, etc. (Speculating without actually knowing).

I don't use fullscreen browsing so I hadn't noticed this, and only barely see it as an issue now that you've pointed it out in detail.

You've said you'll put some more time in here, so I'm going to lurk for now :)

Comment 5

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

Oh, I misunderstood the STR. In this case it looks like this is expected behavior. If the action bar is not showing then I think it's consistent with other fennec scenarios to allow showing/hiding the url bar in this case. If the action bar is showing then the toolbar is locked into place.

Doing a preventDefault will not help in this case because the scrolling is happening in Java and the events never even make it to APZ/C++ code.

Comment 6

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

I agree with what :kats said in comment 5 that the scroll is happening Java due to the dynamic showing/hiding the url bar. This match my observation that when APZ scrolls the content, we'll see a tiny scrollbar appearing or moving up/down on the right hand side of the content. In this case, no scrollbar is showing or it's showing but not moving.

Though this bug looks like a caret usability issue, but it only happens when full-browsing mode is on. And if Mark feels dynamic toggling of the fullscreen browsing setting is not a reasonable solution in comment 4, I cannot think of another solution on top of my head after sleeping on it. Since Java is not my forte, there's little I can help.

Comment 7

[Mark Capella [:capella]]

Attachment: bug1252802.diff

I meant if pinning the DynamicToolbar solves the problem, we can simply provide a bridge from c++ -> Java instead of catching in JS and bouncing back from there.

Using a simplified numeric-only <input> (SingleLineTextControl) from an un-related bug [0] and this patch, it seems your idea works.

Can you confirm?

[0] https://bug1224216.bmoattachments.org/attachment.cgi?id=8686614

Comment 8

[Mark Capella [:capella]]

?ni so we get noticed

Comment 9

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Re comment #7: 

Mark, your patch works for me! I've tested dragging caret when the DynamicToolbar is either shown or hidden, and the page is not scrolling. Awesome!

Comment 10

[Mark Capella [:capella]]

Comment on attachment 8727071 [details] [diff] [review]
bug1252802.diff

TYLin, After all your review / analysis, the patch may be mine but you did all the hard work  :-)

Let's put the patch r? to kats and see if we can close it.

Comment 11

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Comment on attachment 8727071 [details] [diff] [review]
bug1252802.diff

Review of attachment 8727071 [details] [diff] [review]:
-----------------------------------------------------------------

> TYLin, After all your review / analysis, the patch may be mine but you did all the hard work  :-)

Thank you for the patch! I don't have enough Java/JNI knowledge to write a patch like this :)

::: layout/base/AccessibleCaretManager.cpp
@@ +8,5 @@
>  
>  #include "AccessibleCaret.h"
>  #include "AccessibleCaretEventHub.h"
>  #include "AccessibleCaretLogger.h"
> +#include "AndroidBridge.h"

We might need to wrap this in #ifdef MOZ_WIDGET_ANDROID so that it builds on desktop.

@@ +825,5 @@
>    }
> +
> +  // Pin Fennecs DynamicToolbar in place before/after dragging, to avoid
> +  // co-incident screen scrolling.
> +  mozilla::AndroidBridge::Bridge()->OnAccessibleCaretDrag(aState);

Ditto.

Comment 12

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

Comment on attachment 8727071 [details] [diff] [review]
bug1252802.diff

Review of attachment 8727071 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM, r+ with TYLin's comments addressed. Thanks!

Comment 13

[Mark Capella [:capella]]

try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0213cf657760

Comment 14

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

Comment on attachment 8727071 [details] [diff] [review]
bug1252802.diff

Review of attachment 8727071 [details] [diff] [review]:
-----------------------------------------------------------------

If landed as-is, this will just have to be rewritten if we want to support multiple windows.

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java
@@ +1586,5 @@
>      }
>  
> +    @Override
> +    public void onAccessibleCaretDrag(final boolean state) {
> +        // Called from GeckoThread, requires UI thread.

That means some events will get through before the call is made on the UI thread, right? We should try to rework the pinning stuff so it's threadsafe enough to avoid this problem. Not in the scope of this patch, obviously.

::: mobile/android/base/java/org/mozilla/gecko/GeckoAppShell.java
@@ +2518,5 @@
>          return GeckoScreenOrientation.getInstance().getScreenOrientation().value;
>      }
>  
>      @WrapForJNI
> +    public static void onAccessibleCaretDrag(boolean state) {

Please don't put more stuff into GeckoAppShell if we can avoid it. We should be able to route this through the widget and into the appropriate GeckoView/App/Toolbar

::: widget/android/AndroidBridge.h
@@ +288,5 @@
>      // requires a lot of changes...
>      uint32_t GetScreenOrientation();
>      uint16_t GetScreenAngle();
>  
> +    void OnAccessibleCaretDrag(bool state);

I don't think there is a good reason to have wrappers like this in AndroidBridge anymore

Comment 15

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

Actually I guess because there's only ever one toolbar some of what I said above is wrong. However, I still don't really want more stuff going into GeckoAppShell. Ugh.

Comment 16

[Mark Capella [:capella]]

Attachment: bug1252802.diff

Well, you made me work a little harder, but TIL, so it's a cool thing ;)

This comes at the solution through window, and clarifies the underlying reason it works a little bit more.

I love it, tell me you love it?

Comment 17

[Mark Capella [:capella]]

Attachment: bug1252802.diff

Quick tweak I overlooked

Comment 18

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

Comment on attachment 8728327 [details] [diff] [review]
bug1252802.diff

Review of attachment 8728327 [details] [diff] [review]:
-----------------------------------------------------------------

This is a *much* better patch, IMHO. I just don't like the setPinnedNative() call or the nsIWidget change, but those should be easy fixes.

::: mobile/android/base/java/org/mozilla/gecko/gfx/DynamicToolbarAnimator.java
@@ +152,5 @@
>  
>      public void setPinned(boolean pinned) {
>          mPinned = pinned;
>      }
> +    public void setPinnedNative(boolean pinned) {

I think we should do something like the DynamicToolbar pinning (a list of flags). Maybe just move that stuff into here, actually.

::: widget/nsIWidget.h
@@ +1201,5 @@
>        LAYER_MANAGER_CURRENT = 0,
>        LAYER_MANAGER_PERSISTENT
>      };
>  
> +    virtual void SetSelectionDragState(bool aState) {}

I don't think you need to do this. Just cast to nsWindow in AccessibleCaretManager instead. I don't think any other widgets will really want this method.

Comment 19

[Mark Capella [:capella]]

Yep, fairly easy fixes.

Comment 20

[Mark Capella [:capella]]

Attachment: bug1252802.diff

Comment 21

[Mark Capella [:capella]]

Attachment: bug1252802.diff

Sorry bugmail spam, forgot to yank a bit of debug code.

Comment 22

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

Comment on attachment 8728791 [details] [diff] [review]
bug1252802.diff

Review of attachment 8728791 [details] [diff] [review]:
-----------------------------------------------------------------

Very nice

Comment 23

[Mark Capella [:capella]]

Final patch to TRY
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a0fa5e6de1c6

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/c1d6cd4595a0

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c1d6cd4595a0

Comment 26

[:Margaret Leibovic]

This is tracking 47. Does it need to be uplifted? Or is that flag a mistake?

Comment 27

[Mark Capella [:capella]]

The flag seems in error. aiui, APZ and AccessibleCarets fell back to v48, currently Aurora.