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

RESOLVED FIXED in mozilla38

Status

()

Core
Editor
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: George Bytenskiy, Assigned: Ehsan)

Tracking

({reproducible, testcase})

34 Branch
mozilla38
reproducible, testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
Created attachment 8534724 [details]
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.

Updated

4 years ago
Component: Untriaged → DOM: Core & HTML
Product: Firefox → Core

Comment 1

4 years ago
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?
Status: UNCONFIRMED → NEW
Component: DOM: Core & HTML → Editor
Ever confirmed: true
Flags: needinfo?(ehsan)
Keywords: reproducible, testcase
OS: Windows 8.1 → All
Hardware: x86_64 → All
(Assignee)

Comment 2

4 years ago
We should make nsTextEditorState::SetValue fallible.  Its callers in HTMLInputElement and HTMLTextAreaElement are fallible so this should not be a big deal.
Assignee: nobody → ehsan
Flags: needinfo?(ehsan)
(Assignee)

Comment 3

4 years ago
Err... wrong bug.  I'll investigate this later...
Assignee: ehsan → nobody
Flags: needinfo?(ehsan)
(Assignee)

Comment 4

4 years ago
Yeah, we should disallow focusing links when they are in editable regions.  I have a fix.
Assignee: nobody → ehsan
Flags: needinfo?(ehsan)
(Assignee)

Comment 5

4 years ago
Created attachment 8550789 [details] [diff] [review]
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.
Attachment #8550789 - Flags: review?(roc)
(Assignee)

Comment 6

4 years ago
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.
Attachment #8550789 - Attachment is obsolete: true
(Assignee)

Comment 7

4 years ago
Created attachment 8553483 [details] [diff] [review]
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.
Attachment #8553483 - Flags: review?(roc)
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?
Attachment #8553483 - Flags: review?(roc) → review-
(Assignee)

Comment 9

4 years ago
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?
Attachment #8553483 - Flags: review- → review?(roc)
Comment on attachment 8553483 [details] [diff] [review]
Make all links in editable regions unfocusable

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

OK
Attachment #8553483 - Flags: review?(roc) → review+
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
Flags: needinfo?(ehsan)
(Assignee)

Comment 12

4 years ago
(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
Flags: needinfo?(ehsan)
https://hg.mozilla.org/mozilla-central/rev/f2d4c3820220
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.