1273045 - AccessibleCaret does not scroll together with the overflow:scroll div

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect)

Description

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

Attachment: scroll.html

Steps to reproduce on Fennc
1. Open the attached scroll.html, and zoom it in until the text is big enough to perform step 2.
2. Long press on an arbitrary word like "one".
3. Scroll the text in the red box.

Expected result:
The carets will scroll together with the text.

Actual result:
The carets does not move during the scrolling.

Note:
1. If I pan the page outside of the red box, the carets do pan together with the page.
2. The issue cannot be reproduced on Firefox desktop.

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

Is there any chance this is related to absolutely-positioned top layer element not scrolled properly? I think I saw similiar issue in my initial implementation of that.

Comment 2

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

The symptom is very similar to bug 1269065.  The caret div is position:absolute [1], which lives in the position:absolute anonymous content container [2].

[1] https://dxr.mozilla.org/mozilla-central/rev/4a8ed77f6bb573f20980056bf8c1dadd125c1a85/layout/style/res/ua.css#354
[2] https://dxr.mozilla.org/mozilla-central/rev/4a8ed77f6bb573f20980056bf8c1dadd125c1a85/layout/style/res/ua.css#443

Comment 3

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

(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #0)
> 2. The issue cannot be reproduced on Firefox desktop.
This is untrue. I can reproduce this issue on desktop on a clean profile.

Comment 4

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

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #1)
> Is there any chance this is related to absolutely-positioned top layer
> element not scrolled properly? I think I saw similiar issue in my initial
> implementation of that.

I guess we just don't have enough information to tell APZ that which top elements should be scrolled with the layer generated by an scrollable div, so the caret divs are always scrolling with outer most scrollable, which is the body.

Comment 5

[Xidorn Quan [:xidorn] UTC+11]

So it seems to me top layer code has nothing to do here. Anonymous content container is a child of the canvas, which means its position is computed based on the scroll position of the canvas, not the selected fragment. Not sure how this was supposed to work. And I suspect this also affects the highlight area of devtools.

Comment 7

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

Attachment: WIP

Part of the problem here is that while the user is scrolling the AccessibleCaretEventHub is in the ScrollState, which ignores scroll position updates. So it never actually moves the carets while scrolling. The attached patch helps in the case I was testing (scrolling on https://people-mozilla.org/~kgupta/griddiv.html in Fennec while the carets are selecting something inside the div) but for some reason if I scroll the carets out of view, they don't come back after I scroll the selection back into view. So there's more that needs to be done here.

Comment 8

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

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #7)
> but for some reason if I scroll the
> carets out of view, they don't come back after I scroll the selection back
> into view.

I suspect this is because:
1) when the carets go out of view, they change from "Normal" to "NormalNotShown". That works fine.
2) when they selection comes back into view, the manager gets PositionChangedResult::Changed but has aHint==RespectOldAppearance, so it goes down [1] and the carets remain at NormalNotShown. Arguably this is incorrect behaviour, but it's not a huge problem, as long as the carets come back when the finger is lifted. However:
3) when the finger is lifted, the manager calls UpdateCaretsForSelectionMode with aHint==Default, but the manager gets back PositionChangedResult::NotChanged (because the carets were "shown" before and are still shown, so no change). Therefore we go down [2] and the carets are not made visible.

[1] http://searchfox.org/mozilla-central/rev/7cb75d87753de9103253e34bc85592e26378f506/layout/base/AccessibleCaretManager.cpp#391
[2] http://searchfox.org/mozilla-central/rev/7cb75d87753de9103253e34bc85592e26378f506/layout/base/AccessibleCaretManager.cpp#381

Comment 9

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

Attachment: WIP part 2

Indeed that seems to be what's happening. The attached patch makes the carets reappear when you lift your finger. I don't know if it breaks other scenarios though. TYLin, thoughts?

Comment 10

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

Comment on attachment 8845972 [details] [diff] [review]
WIP

Would be good to get your feedback on this one too. I *think* this patch shouldn't regress anything, but not sure.

Comment 11

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

Comment on attachment 8845972 [details] [diff] [review]
WIP

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

The reason that OnScrollPositionChanged() is not override in ScrollState is because we assume carets will always scroll with APZ, and nothing needs to update when scrolling including Fennec's toolbar. I  But now, we need to update carets position during scrolling via OnScrollPostitionChanged.

The toolbar needs to be hidden during scrolling (scroll reason) [1], and show again until the user lifts the finger. So I think this would make Fennec toolbar blinking when scrolling slowly because we keep sending CaretChangedReason::Updateposition [2] in every OnScrollPostitionChanged(). I guess we might prevent this (by abusing UpdateCaretHints) by sending [2] when 

if (!mActiveCaret && aHint == UpdateCaretsHint::Default) 

And we need to do fix UpdateCaretsForCursorMode(), too. I guess some gtest TestAccessibleCaretManager.cpp might need some fix.

[1] http://searchfox.org/mozilla-central/rev/ef0b6a528e0e7b354ddf8cdce08392be8b8ca679/mobile/android/chrome/content/ActionBarHandler.js#70,73
[2] http://searchfox.org/mozilla-central/rev/ef0b6a528e0e7b354ddf8cdce08392be8b8ca679/layout/base/AccessibleCaretManager.cpp#430

::: layout/base/AccessibleCaretEventHub.cpp
@@ +247,5 @@
> +  virtual void OnScrollPositionChanged(
> +    AccessibleCaretEventHub* aContext) override
> +  {
> +    aContext->mManager->OnScrollPositionChanged();
> +  }

Nit: It would be good to define OnScrollPositionChanged() after OnScrollEnd() to match the order of their declarations in
http://searchfox.org/mozilla-central/rev/ef0b6a528e0e7b354ddf8cdce08392be8b8ca679/layout/base/AccessibleCaretEventHub.h#207-208

Comment 12

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

Comment on attachment 8846107 [details] [diff] [review]
WIP part 2

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

I guess this should be OK, but we need to do the same for UpdateCaretsForCursorMode(), too. 

One tricky case on top of my head is scrolling on a input field like Google search for the single caret mode.

1) Type a very long text to overflow the input field.
2) Single tap on the field, and tap the caret to show the Fenenc toolbar.
3) Scroll the field horizontally. Try to scroll the caret, and have it stay inside, or make it scroll outside and then back inside the scroll port. See if the toolbar is correctly hide and re-appear after scroll.

If this approach turn out to be good. We might not need to distinguish between PositionChangedResult::NotChange and Changed. Of course, this is out of the scope of this bug :)

Comment 13

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

Would you mind stealing my patches to finish off the work needed here? You're much more familiar with the intricacies and possible pitfalls of the code than I am, and to be honest I'm swamped with a pretty big backlog of other work too. I'll have patches up soon for bug 1316318 which will disable APZ on the scrollable subframes when the carets are visible, but we'll want to address the issues in this bug to improve the UX.

(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #11)
> So I think this would make
> Fennec toolbar blinking when scrolling slowly because we keep sending
> CaretChangedReason::Updateposition [2] in every OnScrollPostitionChanged().

Yes, you're right - I had noticed this behaviour while testing but wasn't sure what was causing it.

Comment 14

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

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #13)
> Would you mind stealing my patches to finish off the work needed here?
> You're much more familiar with the intricacies and possible pitfalls of the
> code than I am, and to be honest I'm swamped with a pretty big backlog of
> other work too. I'll have patches up soon for bug 1316318 which will disable
> APZ on the scrollable subframes when the carets are visible, but we'll want
> to address the issues in this bug to improve the UX.

No problem. I'm happy to take this bug :)

Comment 15

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

Attachment: Bug 1273045 Part 1 - Convert UpdateCaretsHint to use EnumSet.

Part 2 is going to add a new hint which will use with RespectOldAppearance.
Hence this patch.

Remove #include "mozilla/WeakPtr.h" and "nsWeakReference.h" because they're
not used in the header.

Review commit: https://reviewboard.mozilla.org/r/120090/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/120090/

Comment 16

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

Attachment: Bug 1273045 Part 2 - Update carets when scrolling in subframes without APZ.

Override OnScrollPositionChanged() in ScrollState because we want to update
carets during scrolling in subframes without APZ.

Due to the observation in bug 1273045 comment 8, we do not distinguish
PositionChangedResult::NotChanged and PositionChangedResult::Changed.
Instead, we always update caret even if its position is not changed.

To avoid excessive CaretStateChangedEvents are dispatched in
OnScrollPositionChanged(), we add IsScrollStarted to distinguish whether
OnScrollStart() is called or not.

Review commit: https://reviewboard.mozilla.org/r/120092/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/120092/

Comment 17

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Comment on attachment 8847063 [details]
Bug 1273045 Part 1 - Convert UpdateCaretsHint to use EnumSet.

https://reviewboard.mozilla.org/r/120090/#review122286

Comment 18

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Comment on attachment 8847064 [details]
Bug 1273045 Part 2 - Update carets when scrolling in subframes without APZ.

https://reviewboard.mozilla.org/r/120092/#review122288

Comment 19

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

kats, thank you for jumping into this bug and providing those WIP patches.

My patches here needs to work perfectly with APZ disabled in subframes in bug 1316318. Without bug 1316318, the carets will be laggy when scrolling subframes, but it is still better than no update at all.  I'm going to land this, and I suppose bug 1316318 is going to land soon (r+ in bug 1316318 comment 20 and bug 1316318 comment 21).

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ae9270e12577
Part 1 - Convert UpdateCaretsHint to use EnumSet. r=mtseng
https://hg.mozilla.org/integration/autoland/rev/62ab4dd84f7e
Part 2 - Update carets when scrolling in subframes without APZ. r=mtseng

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ae9270e12577
https://hg.mozilla.org/mozilla-central/rev/62ab4dd84f7e

Comment 22

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

(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #19)
> I suppose bug 1316318 is going to land soon (r+ in bug 1316318
> comment 20 and bug 1316318 comment 21).

Yeah I might as well land those and then do any follow-up work in another bug.