2014481 - Selecting text doesn't work when toggling popover or switching display

Status: NEW

(Core :: DOM: Selection, defect)

Description

[Moritz Beier [:mbeier]]

Attachment: example.html

See the attached html as an example. When selecting text while showPopover is called or display is changed, nothing is selected. The example works fine in Chrome and Safari.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1930931

:sefeng211, since you are the author of the regressor, bug 1930931, could you take a look?

For more information, please visit BugBot documentation.

Comment 2

[Emilio Cobos Álvarez [:emilio]]

Bug 2007147 was reviewed by Masayuki and Sean is not working at MoCo anymore, so ni?ing him as well. The good thing is that bug 1930931 wasn't terribly big so maybe it's not too bad to fix :)

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Although I've not checked it with the debugger. Looks like that changing display of the parent of <input> causes reframing the <input> and the anonymous nodes are recreated. I guess that the problem of the testcase is caused by the recreation occurs during the drag which changes the selection range and the text control state fails to cache the expected selection range.

I wonder if this could be fixed by reuse the anonymous subtrees after the reframe. I think TextEditor can own that instead of the frame. However, I don't know whether it's okay for the native anonymous node management. Emilio, do you know about that?

(I guess we cannot fix this with a small patch.)

Comment 4

[Emilio Cobos Álvarez [:emilio]]

I think we should try to move the editor shadow tree into the DOM. Which is not the same as NAC but should be similar-ish. I recently did that for <select> in https://phabricator.services.mozilla.com/D280041. Would that work? We need that for field-sizing: content I think anyways.

Comment 5

[Emilio Cobos Álvarez [:emilio]]

Let me try to hack up a basic version of it.

Comment 6

[Emilio Cobos Álvarez [:emilio]]

Attachment: WIP: Bug 2014481 - Fix mismatch between GetContentForEvent and EventTargetData::ComputeElementFromFrame.

So I dug into this, and bug 2016280 is worth it but unrelated.

The key issue here is that the editor has a scrollframe with <div>text,
and BeginTrackingDragGesture ends up using the text node rather than the
element.

But PresShell::HandleEvent ends up using the element via
ComputeElementFromFrame, so nsIFrame::HandleEvent keeps bailing out
because we're tracking the text node in the gesture.

This fixes it but not sure it's the right fix.

Comment 7

[Emilio Cobos Álvarez [:emilio]]

See above. I don't think it has anything to do with selection ranges. We are just checking against the wrong tracked content.

Masayuki can you check the commit message above and see if it's the right fix, or propose one?

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Thank you for debugging! I believe that the direction of the patch must be right. However, it's a low level API so that some callers may be designed for the current behavior. So, before landing it, we need to check each user (not only the direct callers, sigh).

Comment 9

[Emilio Cobos Álvarez [:emilio]]

Could you take a look at that? You know the constraints better than me for sure...

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Sure, but not right now, later...

Comment 11

[Andreas Farre [:farre]]

Emilio, are you planning to work on this? Would you mind assigning it and set a severity?

Comment 12

[Emilio Cobos Álvarez [:emilio]]

I was hoping Masayuki could take a look as described in comment 8+...

FWIW, here's an ongoing try run: https://treeherder.mozilla.org/jobs?repo=try&revision=6fa05c6ff615625b9b231ca4a1204f42061caaa2

Severity is probably S3, but it'd be good to remove the hack bug 2007147 introduced.