1516963 - Prevent AccessibleCaret from appearing when dismissing instagram's "use the app" banner on Fennec / GeckoView

Status: RESOLVED FIXED

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

Description

[Emilio Cobos Álvarez (:emilio)]

STR:

 * Go to an instagram profile on Fennec, e.g: https://www.instagram.com/mozilla/
 * Try to dismiss the "The best Instagram experience is on the app." banner.

Expected:

 * You can dismiss it.

Actual:

 * We show carets when tapping over it, and you can't dismiss the banner.

This may be a Layout / AccessibleCaret bug, I guess, so CC'ing Ting-Yu.

Comment 1

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

Attachment: caret-devtools.png

On desktop devtools responsive design mode with prefs [1], this can be reproduced by clicking on the <div> around the "X" button like the screenshot shown. (On fennec, the banner can still be closed by pressing on the X button precisely, but this is very hard to do on mobile devices.)

AccessibleCaret receives a OnSelectionChanged() event with a non-collapsed selection, and it just UpdateCarets() with the selection range there.

The root cause should be why there's a non-collapsed range with just a *single* click? I haven't look into it nor have a reduced test case.

[1] We need layout.accessiblecaret.hide_carets_for_mouse_input=false and layout.accessiblecaret.enabled=true.

Comment 2

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

Beside the caret bug. I saw a separate issue here.

With layout.accessiblecaret.enabled_on_touch=false on Fennec, I still cannot dismiss the Instagram's "Use the App" banner easily. I need to click it multiple times, or force zoom-in the page to make the X larger enough to click. I don't feel the same problem on Google Chrome.

Comment 3

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

Attachment: selection-flex-grid-container.html

The reduced test case has a grid or flex container containing only a simple <button>. If one clicks on the background of the container, we'll create a non-collapsed range. That's why AccessibleCaret shows.

<div style="display: grid;">
  <button>grid</button>
</div>

Perhaps we should make the selection behavior of the flex or grid container more like a block?

Comment 4

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

NI for comment 3.

Given more website adopting flex and grid, this bug might be more prominent on mobile (See bug 1464792).

Mats, given your experience in grid and selection. Perhaps you could provide some input?

Emilio, you've hacked bug 1423331. I guess you might know something?

Comment 5

[Mats Palmgren (inactive)]

The root of the problem seems to be in GetSelectionClosestFrame:
https://searchfox.org/mozilla-central/rev/e62b311b2e0e02633afda2004ba4056b09fbcaa4/layout/generic/nsFrame.cpp#4883
We skip the special handling for block frames at the top,
and then SelfIsSelectable(kid, aFlags) is false for the <button>
so we end up at the last line (4909). In particular, the first
'false' there is for "FrameTarget::frameEdge":
https://searchfox.org/mozilla-central/rev/e62b311b2e0e02633afda2004ba4056b09fbcaa4/layout/generic/nsFrame.cpp#4665
which is somewhat mysteriously documented as:

frameEdge says whether one end of the frame is the result
(in which case different handling is needed)

but I think it controls whether we end up treating it
as a single point or a range.
https://searchfox.org/mozilla-central/rev/e62b311b2e0e02633afda2004ba4056b09fbcaa4/layout/generic/nsFrame.cpp#5001-5015

Comment 6

[Mats Palmgren (inactive)]

Attachment: Testcase #2

BTW, the problem also occurs for an empty Grid/Flex container.

Comment 7

[Mats Palmgren (inactive)]

Attachment: wip

Something like this might work. ¯_(ツ)_/¯

Comment 9

[Emilio Cobos Álvarez (:emilio)]

I agree with Mats' diagnostic. Though maybe I'd do something like:

  // Fall back to the last targetable frame.
  if (aFrame->IsBlockOutside()) {
    return DrillDownToSelectionFrame(aFrame, false, aFlags);
  }
  return FrameTarget(aFrame, false, false);

That'd make the fall-back consistent with what we do for blocks more often, probably.

Comment 10

[csheany]

Will this have an impact on Bug 1532386?

Comment 11

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

Attachment: Bug 1516963 - Make grid, flex, etc. be a selection target themselves if they contain no selectable children.

Exclude replaced frame likes image frames otherwise
editor/libeditor/tests/test_abs_positioner_positioning_elements.html
will break.

Comment 12

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

Both approaches in comment 7 and comment 9 break editor/libeditor/tests/test_abs_positioner_positioning_elements.html like
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7334f76b833b564d6803346748e974005ef33098&selectedJob=237731148 So I exclude replaced element in my patch.

I'd like to have this bug fixed. Mats, I hope you don't mind I modify your WIP patch, and add some tests.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5bbc58029265
Make grid, flex, etc. be a selection target themselves if they contain no selectable children. r=emilio

Comment 14

[Bogdan Tara[:bogdan_tara | bogdant]]

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

Comment 15

[Chris Peterson [:cpeterson]]

Ting-Yu, should we uplift your fix to Fennec 67 Beta? Not being able to dismiss the "Use the app" banner sounds like an annoying user experience, but I don't know if the fix is too risky to uplift.

Comment 16

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

My patch is small, but the selection code is prone to regression. I'd like to have it ride the train.

Comment 17

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

Chris, I filed a follow-up bug 1542381 for my comment 12 because my patch fixed only part of the story.

Comment 18

[Chris Peterson [:cpeterson]]

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

My patch is small, but the selection code is prone to regression. I'd like to have it ride the train.

Sounds good. 67=wontfix