1109968 - caret disappears upon entering a div with contenteditable flag set to false.

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[George Bytenskiy]

Attachment: Html to repro the lost caret issue

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/39.0.2171.71 Safari/537.36

Steps to reproduce:

1. have a div with contenteditable set to "true"
2. create another div with contenteditable set to "false". place it inside of the first div
3. create an <a href=""> element and place it inside the inner div.
4. place caret just before the outer div, press right arrow key

Please see a sample html file attached for a repro


Actual results:

notice that caret gets lost and is non-recoverable


Expected results:

the caret should not get lost. even if it's invisible, when i hit the left arrow key or keep hitting the right arrow key I should be able to recover my caret position.

Comment 1

[:Gijs (he/him)]

It looks like the presence of a text node before the <a href> inside the contenteditable="false" node works around this in the part of the test case where the cursor does not get stuck. It looks like everywhere else, the link gets focused and from there, the arrow keys no longer move the cursor out of there. It seems to me like we should not let the link get focused when navigating through the outer contenteditable="true" div using the arrow keys ("even" when there is no whitespace around it). Ehsan, does that sound right?

Comment 2

[(no longer active)]

We should make nsTextEditorState::SetValue fallible.  Its callers in HTMLInputElement and HTMLTextAreaElement are fallible so this should not be a big deal.

Comment 3

[(no longer active)]

Err... wrong bug.  I'll investigate this later...

Comment 4

[(no longer active)]

Yeah, we should disallow focusing links when they are in editable regions.  I have a fix.

Comment 5

[(no longer active)]

Attachment: Make all links in editable regions unfocusable

The content inside an editable region is either editable itself, or
is inside a contenteditable="false" subtree.  In the first case,
it should not be focusable since it is editable.  In the second
case, it should not be focusable since the entire non-editable
region is treated as a special single entity for the purposes of
selection and caret movement, and having something focusable in
the middle of such a subtree breaks that model.

Comment 6

[(no longer active)]

Comment on attachment 8550789 [details] [diff] [review]
Make all links in editable regions unfocusable

Oh, sorry, I should have caught this, but this refactored function doesn't really help in this case.  In fact, we need nothing fancier than just walking up the parent chain and looking for the first editable element.  Ultimately we'd check the document node, which is set to editable in designMode.

Comment 7

[(no longer active)]

Attachment: Make all links in editable regions unfocusable

The content inside an editable region is either editable itself, or
is inside a contenteditable="false" subtree.  In the first case,
it should not be focusable since it is editable.  In the second
case, it should not be focusable since the entire non-editable
region is treated as a special single entity for the purposes of
selection and caret movement, and having something focusable in
the middle of such a subtree breaks that model.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8553483 [details] [diff] [review]
Make all links in editable regions unfocusable

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

::: dom/html/HTMLAnchorElement.cpp
@@ +196,5 @@
> +static bool
> +IsNodeInEditableRegion(nsINode* aNode)
> +{
> +  while (aNode) {
> +    if (aNode->IsEditable()) {

Don't you want to walk across document boundaries here? A link inside an iframe inside a contenteditable=false container should not be focusable, presumably?

Comment 9

[(no longer active)]

Comment on attachment 8553483 [details] [diff] [review]
Make all links in editable regions unfocusable

(In reply to Robert O'Callahan (:roc) (offline Jan 17-22) (Mozilla Corporation) from comment #8)
> ::: dom/html/HTMLAnchorElement.cpp
> @@ +196,5 @@
> > +static bool
> > +IsNodeInEditableRegion(nsINode* aNode)
> > +{
> > +  while (aNode) {
> > +    if (aNode->IsEditable()) {
> 
> Don't you want to walk across document boundaries here? A link inside an
> iframe inside a contenteditable=false container should not be focusable,
> presumably?

Hmm, why?  Content inside an editable iframe is not made editable, and functions normally from every respect (link navigate when they are clicked, etc.)  I don't understand why we should make focusability the exception for them.  Please reconsider this?

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8553483 [details] [diff] [review]
Make all links in editable regions unfocusable

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

OK

Comment 11

[Carsten Book [:Tomcat]]

btw that landed as https://hg.mozilla.org/integration/mozilla-inbound/rev/2b79bd646942

and sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=5809727&repo=mozilla-inbound

Comment 12

[(no longer active)]

(In reply to Carsten Book [:Tomcat] from comment #11)
> btw that landed as
> https://hg.mozilla.org/integration/mozilla-inbound/rev/2b79bd646942
> 
> and sorry had to back this out for test failures like
> https://treeherder.mozilla.org/logviewer.html#?job_id=5809727&repo=mozilla-
> inbound

Ah, we need to run these without the touch caret:

https://hg.mozilla.org/integration/mozilla-inbound/rev/f2d4c3820220

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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