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

Status: RESOLVED FIXED

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

Description

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

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

Comment 1

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

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!

Comment 2

[YuFu Kao]

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()) {

2. 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;

Comment 3

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

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

Comment 4

[YuFu Kao]

Attachment: Bug 1517385 - Use "visibility: hidden" to keep the frame of AccessibleCaret alive. r=tylin

Comment 5

[YuFu Kao]

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.

Comment 6

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: A testcase that AccessibleCaret occupies no space if its appearance is none.

Comment 7

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: A testcase that AccessibleCaret occupies no space if its appearance is none.

Run the test via `./mach reftest layout/reftests/bugs/1517385.html`.

Comment 8

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

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

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2d480000f34e
Use "visibility: hidden" to keep the frame of AccessibleCaret alive. r=TYLin

Comment 10

[Razvan Maries]

Backed out for high frequency failures.

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&searchStr=windows%2C10%2Cx64%2Copt%2Ctest-windows10-64%2Fopt-marionette-e10s%2Cmn&revision=b0c83856ebcf4548dfaf208d32c58380262b2628&selectedTaskRun=PQxfOOEaRqG_4exavB0P4w.0

Log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=319338856&repo=autoland&lineNumber=54568

Backout: https://hg.mozilla.org/integration/autoland/rev/a6ba6ed8e1c9873f7a3b288a73cf2d257be07089

Comment 11

[YuFu Kao]

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

Comment 12

[YuFu Kao]

@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 ..?

Comment 13

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Nice observation!

I failed to reproduce the failure by running ./mach marionette-test layout/base/tests/marionette/test_accessiblecaret_selection_mode.py locally on my machine. However, on testcase test_long_press_to_select_when_partial_visible_word_is_selected, when "AAAAAAAA" is selected, I see the second caret (right caret) has a transition animation shifting it from right to left to its correct position. I guess this is because of the margin-left transition effect specified at [2].

I believe this can only happen when the appearance is changing from NormalNotShow to Normal the very first time when the page is loaded since this is the first time the caret gets the margin-left style [1]. This is rare on the field, and the animation won't impact the user interaction so I'm reluctant to fix AccessibleCaret.

How about we fix the test by forcing the second caret to have Normal appearance once? For example, adding the following code after word0_x, word0_y = self.word_location(el, 0) [2].

        # Long press on the first word to make both carets show to avoid
        # transition effect on margin-left later on.
        self.long_press_on_location(el, word0_x, word0_y)

If the above works for you locally, feel free to add it to your patch and add "check-in needed" tag to land it. Feel free to needinfo me if you fix other testcases and would like me to take look at the patch again; or you need help to run your patch on Firefox CI (try server) if you don't have the access.

[1] https://searchfox.org/mozilla-central/rev/d866b96d74ec2a63f09ee418f048d23f4fd379a2/layout/base/AccessibleCaret.cpp#302-304
[2] https://searchfox.org/mozilla-central/rev/d866b96d74ec2a63f09ee418f048d23f4fd379a2/layout/style/res/ua.css#412-413
[3] https://searchfox.org/mozilla-central/rev/d866b96d74ec2a63f09ee418f048d23f4fd379a2/layout/base/tests/marionette/test_accessiblecaret_selection_mode.py#639
[4] https://firefox-source-docs.mozilla.org/tools/try/index.html

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Could have bug 1663928 and bug 1663511 the same underlying reason with the transitioning?

Comment 15

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Re comment 14:
Both bugs are for intermittent in the single caret mode, and the caret's appearace is changing from None to Normal the first time. So it doesn't seem likely to be the same underlying reason as comment 13.

Comment 16

[YuFu Kao]

(In reply to Ting-Yu Lin [:TYLin] (UTC-7) from comment #13)

How about we fix the test by forcing the second caret to have Normal appearance once? For example, adding the following code after word0_x, word0_y = self.word_location(el, 0) [2].

        # Long press on the first word to make both carets show to avoid
        # transition effect on margin-left later on.
        self.long_press_on_location(el, word0_x, word0_y)

The test_long_press_to_select_when_partial_visible_word_is_selected() is just the case to test the appearance change from 'NormalNotShown' to 'Normal', so it did not work.

How about adding a preference for the transition-duration of AccessibleCaret? We can set it to 0 before testing, so we will not worry about the timing at all.

Comment 17

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

How about adding a preference for the transition-duration of AccessibleCaret? We can set it to 0 before testing, so we will not worry about the timing at all.

Yes, adding a preference to control transition-duration sounds good to me. Please add another patch for this change. Thanks!

Comment 18

[YuFu Kao]

Attachment: Bug 1517385 - Add the pref to control the transition-duration. r=tylin

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5f4e48e6315b
Add the pref to control the transition-duration. r=TYLin

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d3e9f74c23d7
Use "visibility: hidden" to keep the frame of AccessibleCaret alive. r=TYLin

Comment 21

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/5f4e48e6315b

Comment 22

[Dorel Luca [:dluca]]

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