Closed Bug 1273045 Opened 3 years ago Closed 3 years ago

AccessibleCaret does not scroll together with the overflow:scroll div

Categories

(Core :: Selection, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox49 --- affected
firefox55 --- fixed

People

(Reporter: TYLin, Assigned: TYLin)

References

(Blocks 1 open bug)

Details

Attachments

(5 files)

Attached file 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.
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.
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
See Also: → 1269065
(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.
(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.
OS: Android → All
Summary: AccessibleCaret does not scroll together with the overflow:scroll div on Fennec → AccessibleCaret does not scroll together with the overflow:scroll div
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.
Duplicate of this bug: 1302180
See Also: → 1316318
Attached patch WIPSplinter Review
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.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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
Attached patch WIP part 2Splinter Review
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?
Attachment #8846107 - Flags: feedback?(tlin)
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.
Attachment #8845972 - Flags: feedback?(tlin)
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
Attachment #8845972 - Flags: feedback?(tlin) → feedback+
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 :)
Attachment #8846107 - Flags: feedback?(tlin) → feedback+
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.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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 :)
Assignee: nobody → tlin
Status: NEW → ASSIGNED
Comment on attachment 8847063 [details]
Bug 1273045 Part 1 - Convert UpdateCaretsHint to use EnumSet.

https://reviewboard.mozilla.org/r/120090/#review122286
Attachment #8847063 - Flags: review?(mtseng) → review+
Comment on attachment 8847064 [details]
Bug 1273045 Part 2 - Update carets when scrolling in subframes without APZ.

https://reviewboard.mozilla.org/r/120092/#review122288
Attachment #8847064 - Flags: review?(mtseng) → review+
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).
Pushed by tlin@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/ae9270e12577
https://hg.mozilla.org/mozilla-central/rev/62ab4dd84f7e
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(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.
Duplicate of this bug: 1244705
Duplicate of this bug: 1269065
You need to log in before you can comment on or make changes to this bug.