Closed Bug 1252802 Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox for Android :: Text Selection, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 48
Tracking Status
firefox47 --- disabled
firefox48 --- fixed
fennec 48+ ---

People

(Reporter: TYLin, Assigned: capella)

References

(Blocks 1 open bug)

Details

(Keywords: correctness, Whiteboard: [gfx-noted])

Attachments

(1 file, 4 obsolete files)

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
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
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
Component: Panning and Zooming → Text Selection
Product: Core → Firefox for Android
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
Flags: needinfo?(markcapella)
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 :)
Flags: needinfo?(markcapella)
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.
tracking-fennec: --- → ?
tracking-fennec: ? → 47+
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.
Attached patch bug1252802.diff (obsolete) — Splinter Review
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
?ni so we get noticed
Flags: needinfo?(tlin)
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!
Flags: needinfo?(tlin)
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.
Attachment #8727071 - Flags: review?(bugmail.mozilla)
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 on attachment 8727071 [details] [diff] [review]
bug1252802.diff

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

LGTM, r+ with TYLin's comments addressed. Thanks!
Attachment #8727071 - Flags: review?(bugmail.mozilla) → review+
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
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
Attachment #8727071 - Flags: feedback-
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.
Attached patch bug1252802.diff (obsolete) — Splinter Review
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?
Attachment #8727071 - Attachment is obsolete: true
Attachment #8728233 - Flags: review?(snorp)
Attached patch bug1252802.diff (obsolete) — Splinter Review
Quick tweak I overlooked
Attachment #8728233 - Attachment is obsolete: true
Attachment #8728233 - Flags: review?(snorp)
Attachment #8728327 - Flags: review?(snorp)
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.
Attachment #8728327 - Flags: review?(snorp) → review-
Yep, fairly easy fixes.
Attached patch bug1252802.diff (obsolete) — Splinter Review
Attachment #8728327 - Attachment is obsolete: true
Attachment #8728790 - Flags: review?(snorp)
Attached patch bug1252802.diffSplinter Review
Sorry bugmail spam, forgot to yank a bit of debug code.
Attachment #8728790 - Attachment is obsolete: true
Attachment #8728790 - Flags: review?(snorp)
Attachment #8728791 - Flags: review?(snorp)
Comment on attachment 8728791 [details] [diff] [review]
bug1252802.diff

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

Very nice
Attachment #8728791 - Flags: review?(snorp) → review+
https://hg.mozilla.org/mozilla-central/rev/c1d6cd4595a0
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
This is tracking 47. Does it need to be uplifted? Or is that flag a mistake?
Flags: needinfo?(markcapella)
The flag seems in error. aiui, APZ and AccessibleCarets fell back to v48, currently Aurora.
Flags: needinfo?(markcapella)
tracking-fennec: 47+ → 48+
You need to log in before you can comment on or make changes to this bug.