Closed Bug 969773 Opened 6 years ago Closed 6 years ago

Firefox erroneously selects text if you set contenteditable=false and then focus a node containing text

Categories

(Core :: DOM: Editor, defect)

27 Branch
x86_64
All
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: bunch.jesse, Assigned: mats)

Details

(Keywords: testcase)

Attachments

(2 files)

(note: the attached test case demonstrates this)

1. Create div#1 with contenteditable=true and tabindex=0
2. Create div#2 with some text
3. Focus the div#1 by calling .focus()
4. Set contenteditable=false on div#1
5. Focus div#2 by calling .focus()

Whammy.

What happens:
div#2 is focused and all of it's text is selected.

What should happen: 
div#2 should gain focus and it's text should not be focused.

Workaround:
Call .blur() on div#1 before setting contenteditable=false

Other Notes:
Reproduces on both OSX (v27) and Windows (v27)
Attached patch fix+reftestSplinter Review
The editor has this focus/blur listener that manipulates the Selection:
http://hg.mozilla.org/mozilla-central/annotate/7133bb431eba/editor/libeditor/base/nsEditorEventListener.cpp#l955
Among other things, it's installs an ancestor limiter on focus and
removes it on blur.  In the testcase at hand we don't blur though,
we simply disconnect this listener leaving the ancestor limiter in place.
So when the next node is focused we fail to Collapse the selection
because the new node is not a descendent of the limiter and is rejected.
This happens here:
http://hg.mozilla.org/mozilla-central/annotate/7133bb431eba/dom/base/nsFocusManager.cpp#l2124
The AddRange succeeds and selects the contents of the focused node.
The CollapseToStart fails, and leaves the selected state as is.
http://hg.mozilla.org/mozilla-central/annotate/7133bb431eba/dom/base/nsFocusManager.cpp#l2125

https://tbpl.mozilla.org/?tree=Try&rev=5f1951267a2c
Assignee: nobody → matspal
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attachment #8373060 - Flags: review?(ehsan)
This seems like a rather major bug and I'm surprised we haven't caught it until now.
Is it a regression perhaps?
Component: Layout → Editor
Keywords: testcase
By regressing with previous release builds, it looks like this first happens with Firefox 11.
Comment on attachment 8373060 [details] [diff] [review]
fix+reftest

Review of attachment 8373060 [details] [diff] [review]:
-----------------------------------------------------------------

I'm pretty sure that this has been broken since forever. :(  Thanks for the investigation and the patch!
Attachment #8373060 - Flags: review?(ehsan) → review+
https://hg.mozilla.org/mozilla-central/rev/9009d48cb9c7
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Mats, how do you feel about uplifting this to Aurora?  (See bug 965690 comment 6)
This patch is low risk and likely applies as is, so it should be fine to uplift.
You need to log in before you can comment on or make changes to this bug.