Open Bug 1517385 Opened 2 years ago Updated 10 hours ago

Tilted carets become normal if they're scroll out of and back into viewport

Categories

(Core :: DOM: Selection, defect, P4)

defect

Tracking

()

ASSIGNED

People

(Reporter: TYLin, Assigned: lonocvb, NeedInfo)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

Steps to reproduce:
1. On desktop, set pref layout.accessiblecaret.hide_carets_for_mouse_input=false and layout.accessiblecaret.enabled=true.
2. Open an arbitrary page
3. Select a character, the carets should be it tilt mode, i.e. the left caret is tilted to the left, and the right caret is tilted to the right.
4. Scroll the page up so that both carets are out of viewport.
5. Scroll the page down to bring both carets back into the viewport.

Expected result:
The carets remain in tilt mode.

Actual result:
The carets become normal mode.

This bug affects only non-Android devices because Android is in always tilt mode [1].

[1] https://searchfox.org/mozilla-central/rev/0ee0b63732d35d16ba22d5a1120622e2e8d58c29/mobile/android/app/mobile.js#819
Assignee: nobody → lonocvb
Status: NEW → ASSIGNED

Lono, I assign the bug to you per your message on #layout channel. Feel free to ask questions, or let us know if you are unable to work on this problem. Thanks!

The bug is caused by incorrect carets intersection under the onScrollEnd() -> UpdateCarets() -> UpdateCaretsForOverlappingTilt() without FlushLayout();

  1. The condition is false; the firstCaretResult and secondCaretResult are both NotChanged.
    mIsCaretPositionChanged =
    firstCaretResult == PositionChangedResult::Position ||
    secondCaretResult == PositionChangedResult::Position;
  
  if (mIsCaretPositionChanged) {
    // Flush layout to make the carets intersection correct.
    if (!FlushLayout()) {
  1. Even we make the condition pass, it seems that is not allowed to call FlushLayout() in the onScrollEnd() callback by the change https://phabricator.services.mozilla.com/D15537
void AccessibleCaretManager::OnScrollStart() {
  AC_LOG("%s", __FUNCTION__);

  AutoRestore<bool> saveAllowFlushingLayout(mAllowFlushingLayout);
  mAllowFlushingLayout = false;

Yeah, we deliberately forbid AccessibleCaret to flush layout after certain operations due to security issues (as flushing the layout can trigger arbitrary javascript script of the web pages, and it can possibly destroy the frame tree in which AccessibleCaret lives).

It seems AccessibleCaret::Intersects doesn't work if the caret is being scrolling back into the viewport. As their position is not changed, but their appearance is changing from Appearance::NormalNotShown back to something visible. This does require layout to flush to regenerate the real frame for AccessibleCaret's element because the caret's element is just a <div> and currently we tweak its class to none in AccessibleCaret::SetAppearance [1] so that its hidden by applying display:none [2].

It may worth trying to simply set visibility: hidden when the caret has Appearance::NormalNotShown to preserve the frame so that caret's intersection can work. If it's not working, I'll be curious to know why. Otherwise, I imaging if we want to cache the caret's rect in C++, it'd be tricky to invalidate the cache at the right place, because we certainly need to store the value after we really does the layout flush. I hope we don't need to pursue this path.

[1] https://searchfox.org/mozilla-central/rev/61728de8273c04fe2417c475fc0637e8b79210d7/layout/base/AccessibleCaret.cpp#89
[2] https://searchfox.org/mozilla-central/rev/61728de8273c04fe2417c475fc0637e8b79210d7/layout/style/res/ua.css#454-456

The bug is fixed as you suggest, submitted for review.
I get a lot out of the information from your comment. Thank you for your advice.

Run the test via `./mach reftest layout/reftests/bugs/1517385.html`.
Attachment #9182788 - Attachment is obsolete: true

No problem! Thanks for fixing this bug.

Actually, changing both Appearance::NormalNotShown and Appearance::None to use visibility:hidden introduces a bug.

We might want to set the visibility: hidden only if Appearance::NormalNotShown, and keep display: none when Appearance::None. We can add something like the following to ua.css,

div:-moz-native-anonymous.moz-accessiblecaret.hidden {
  visibility: hidden;
}

and add a new switch case arm to AccessibleCaret::AppearanceString [1] to return "hidden" if Appearance::NormalNotShown.

I attached patch with a testcase that fails with your current approach. I believe the testcase should pass with my thoughts above. You can merge my patch into your final revision.

[1] https://searchfox.org/mozilla-central/rev/25d5a4443a7e13cfa58eff38f1faa5e69f0b170f/layout/base/AccessibleCaret.cpp#116-119

Pushed by aethanyc@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/2d480000f34e
Use "visibility: hidden" to keep the frame of AccessibleCaret alive. r=TYLin

Oops, I will check that. I can reproduce the issue and the fail rate is about 5/10 in my local machine.

@TYLin, there is a CSS transition when we change the appearance to Normal(visibility:visible) from NormalNotShown (visibility:hidden), the caret may not be exactly at the left or right of the selection rectangle yet, so we may fail some of the tests at test_accessiblecaret_selection_mode.py when we try to touch caret.

Not sure what is the preferred way to fix the side effect, do you have any suggestions?

  • Avoid triggering the CSS transition, to cancel the CSS transition, e.g. add a ".notransition" for AccessibleCaret, and then remove in a short timeout.
  • Add a delay (250ms) in the tests; make sure that the animation is complete before touching it.
  • Others ..?
Flags: needinfo?(lonocvb) → needinfo?(aethanyc)
You need to log in before you can comment on or make changes to this bug.