Contenteditable node is still editable without focus

RESOLVED FIXED in mozilla2.0b3

Status

()

Core
Editor
RESOLVED FIXED
11 years ago
8 years ago

People

(Reporter: Albert Brand, Assigned: masayuki)

Tracking

({testcase})

Trunk
mozilla2.0b3
testcase
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 -
wanted1.9 +
in-testsuite +

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

Attachments

(4 attachments, 5 obsolete attachments)

(Reporter)

Description

11 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a7pre) Gecko/2007072319 Minefield/3.0a7pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a7pre) Gecko/2007072319 Minefield/3.0a7pre

See the upcoming testcase. When you click approx 5 pixels above or beneath the gray box (which is not the contenteditable) or closer to the contenteditable region, you can edit the contenteditable (type something). No focus ring is visible. The left and right side can be clicked anywhere to get the same effect. 

I've seen the same with simple text surrounding a contenteditable box.

Reproducible: Always



Expected Results:  
Only when the contenteditable region has focus, you should be able to type something.
(Reporter)

Comment 1

11 years ago
Created attachment 273552 [details]
Testcase
(Reporter)

Updated

11 years ago
Blocks: 237964
Keywords: testcase
Version: unspecified → Trunk
Confirmed.
This is not what IE7 is doing.
Status: UNCONFIRMED → NEW
Component: General → Editor
Ever confirmed: true
Product: Firefox → Core
QA Contact: general → editor
Flags: blocking1.9? → blocking1.9-
Whiteboard: [wanted-1.9]
Flags: wanted1.9+
Whiteboard: [wanted-1.9]

Comment 3

8 years ago
Created attachment 424844 [details]
Another example of the focus problem.

In this repro, if you click in the editable region, click tab, and keep typing, then you will see the focus shift to the button, but characters will continue to enter the CE region.
Taking. This may be difficult, but this kills some shortcut keys, e.g., "/", "," of FAYT, so, some accessibility issues could be caused by this.

See also bug 544277 comment 13.
Assignee: nobody → masayuki
Blocks: 467715
No longer blocks: 467715
Looks like there are two bugs:

1. nsHTMLEditor::BeginingOfDocument() sets a selection DOM range to the start of first editable text node. So, when we load a document whose non-root element has contenteditable="true", the selection is moved to the first editable content without focus. This looks bad behavior in caret browsing mode.

2. nsEditorEventListener always sends all input events to editor. So, they don't check whether the event target is in editor or not.

I guess that that the selection change doesn't cause the focus change is valid.

If so, we shouldn't set selection in nsHTMLEditor::BeginningOfDocument() when the editor's root element isn't editable.

And also, nsEditorEventListener should check whether the target is editable or not.

But my idea has an issue. If the editor tries to edit without selection ranges, it failed with assertions. So, I need to check the impact.
Status: NEW → ASSIGNED
OS: Windows Vista → All
Hardware: x86 → All
Blocks: 563865
Duplicate of this bug: 442811
Blocks: 467715
Blocks: 481626
Depends on: 564669

Comment 8

8 years ago
This is a dupe of bug 567213, which has already been fixed on trunk.
Assignee: masayuki → ehsan
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 567213
(In reply to comment #8)
> This is a dupe of bug 567213, which has already been fixed on trunk.
> 
> *** This bug has been marked as a duplicate of bug 567213 ***

Um.. no, the patch is not perfect, see my patch. I'll fix the remains in bug 467715.

Comment 10

8 years ago
(In reply to comment #9)
> (In reply to comment #8)
> > This is a dupe of bug 567213, which has already been fixed on trunk.
> > 
> > *** This bug has been marked as a duplicate of bug 567213 ***
> 
> Um.. no, the patch is not perfect, see my patch. I'll fix the remains in bug
> 467715.

Is there any case where for a given event, IsTargettedEvent returns true, but IsModifiable on its target returns false?
(In reply to comment #10)
> Is there any case where for a given event, IsTargettedEvent returns true, but
> IsModifiable on its target returns false?

I guess that events for input elements in a contenteditable editor are so. And also generated events for non-focused editable element.

And the patch didn't modify IME handling.

Comment 12

8 years ago
OK then.  Let's take your patch!  :-)  Want me to do a drive-by review?
Assignee: ehsan → masayuki
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Created attachment 452626 [details] [diff] [review]
Patch v1.0

This patch checks:

* Whether the event target is the editor.
** If the editor is a plain text editor, i.e., <input> or <textarea>, checks the target is the element.
** Otherwise, the target is a contenteditable host element of current selection.

* Whether the event target is focused if the event isn't trusted because WebKit doesn't process the dispatched events from content.

For simpler, this patch adds two APIs to nsIContent. One is HasIndependentSelection(), this is same as nsHTMLEditor::IsIndependentSelectionContent(). The other is GetContentEditableEditingHost(), this returns a contenteditable host content for current selection. If the selection isn't editable or the selection is in an independent selection content, this returns NULL.

Note that we need to check the focus in DOM window level. E.g., we need to handle some dispatched events in background tabs or back window. Furthermore, by bug 519913, we need to receive the composition events after the editor window was deactivated.
Attachment #443831 - Attachment is obsolete: true
Attachment #452626 - Flags: review?(ehsan)

Comment 14

8 years ago
Comment on attachment 452626 [details] [diff] [review]
Patch v1.0

>diff --git a/docshell/test/navigation/test_bug430624.html b/docshell/test/navigation/test_bug430624.html
>--- a/docshell/test/navigation/test_bug430624.html
>+++ b/docshell/test/navigation/test_bug430624.html
>@@ -26,22 +26,25 @@ https://bugzilla.mozilla.org/show_bug.cg
> 
> function onLoad() {
>   window.frames[0].frameElement.onload = onReload;
>   window.frames[0].location = window.frames[0].location;
> }
> 
> function onReload() {
>   var bodyElement = window.frames[0].frameElement.contentDocument.body;
>-  sendChar('S', bodyElement);
>-  sendChar('t', bodyElement);
>-  sendChar('i', bodyElement);
>-  sendChar('l', bodyElement);
>-  sendChar('l', bodyElement);
>-  sendChar(' ', bodyElement);
>+  bodyElement.focus();

Why is this necessary with this patch?

>+
>+  var win = window.frames[0].frameElement.contentWindow;
>+  synthesizeChar('S', win);
>+  synthesizeChar('t', win);
>+  synthesizeChar('i', win);
>+  synthesizeChar('l', win);
>+  synthesizeChar('l', win);
>+  synthesizeChar(' ', win);

Why did you need to switch from using sendChar?

>diff --git a/editor/libeditor/html/tests/test_contenteditable_text_input_handling.html b/editor/libeditor/html/tests/test_contenteditable_text_input_handling.html

The tests look great!

>+  function getTextValue(aElement)

Is the result of this function going to be different with aElement.textContent?

>+    // IME

I skimmed over this part of the test, but I don't claim to understand most of the IME tests/code.  Could you ask for a review form someone on the content team, and someone who knows IME code as well?  (Smaug?)

>diff --git a/layout/generic/test/test_bug468167.html b/layout/generic/test/test_bug468167.html
>-var e = document.createEvent("KeyEvents");
>-e.initKeyEvent("keypress", true, true, document.defaultView,
>-               false, false, false, false,
>-               KeyEvent.DOM_VK_BACK_SPACE, 0);
>-ta.dispatchEvent(e);
>+synthesizeKey("VK_BACK_SPACE", { });

You need to wrap the js for this test inside waitForFocus, otherwise this is prone to random oranges on Linux.

>+function synthesizeChar(aChar, aWindow)

I'm not sure what the advantage of adding this function is, when we already have synthesizeKey.
Attachment #452626 - Flags: review?(ehsan)

Comment 15

8 years ago
(In reply to comment #13)
> Note that we need to check the focus in DOM window level. E.g., we need to
> handle some dispatched events in background tabs or back window. Furthermore,
> by bug 519913, we need to receive the composition events after the editor
> window was deactivated.

Besides bug 519913, do you have any other cases in mind where we have to handle dispatched events in background tabs or windows?
> do you have any other cases in mind where we have to handle
> dispatched events in background tabs or windows?

MozMill tests are, see bug 497839. And some web contents might dispatch them (but it's not worth).
> test_bug430624.html

Ah, I don't need to switch SendChar to SynthesizeChar after I set focus to the body element.

>>+  function getTextValue(aElement)
> 
> Is the result of this function going to be different with aElement.textContent?

textContent returns null even if I QIed to nsIDOM3Node.
Created attachment 452989 [details] [diff] [review]
Patch v1.1
Attachment #452626 - Attachment is obsolete: true
Attachment #452989 - Flags: review?(ehsan)

Updated

8 years ago
Attachment #452989 - Flags: review?(ehsan) → review+

Comment 19

8 years ago
Comment on attachment 452989 [details] [diff] [review]
Patch v1.1

r=me, sans the content and IME parts.
Comment on attachment 452989 [details] [diff] [review]
Patch v1.1

Roc:

Would you review the content part and IME tests in the new tests?
Attachment #452989 - Flags: review?(roc)
+  /**
+   * If the content is a part of contenteditable editor, this returns editing
+   * host content.  When the content is in designMode, this returns null
+   * because the contenteditable doesn't have any meaning.  Also, when the
+   * content isn't editable, this returns null.
+   */
+  nsIContent* GetContentEditableEditingHost();

Could we make this more useful by returning the root element or <body> when in designMode? Then null would mean the content isn't editable, which would be nice.

Instead of a boolean parameter to HasFocus, how about a flags parameter so the caller can pass a meaningful flag name instead of true/false. Although here it's probably even better to just introduce a new method instead.

+  // Whether the event is for this editor or not.
+  virtual PRBool IsAcceptableInputEvent(nsIDOMEvent* aEvent);

This needs to be better documented.
(In reply to comment #21)
> +  /**
> +   * If the content is a part of contenteditable editor, this returns editing
> +   * host content.  When the content is in designMode, this returns null
> +   * because the contenteditable doesn't have any meaning.  Also, when the
> +   * content isn't editable, this returns null.
> +   */
> +  nsIContent* GetContentEditableEditingHost();
> 
> Could we make this more useful by returning the root element or <body> when in
> designMode? Then null would mean the content isn't editable, which would be
> nice.

Hmm, I don't think it's better. The selection root is root element but actual editable root is <body> in designMode. The difference could cause some bugs in future.

And I don't assume that the method is called in designMode path because in most cases, designMode should be processed with a path different from contenteditable.

If some callers see the new assertion in designMode, there is some hidden bugs there.
HTML5 wants 'designMode' to behave exactly as if contenteditable was set on the <body>. So I really want to see us moving towards designMode being processed with the same path as contenteditable.
But there is large behavior difference, by default. In designMode, javascript isn't executed, while in contenteditable mode, it is.
Right now that's true, but we're going to change that. See bug 519928.
O.K. So, we need to refactor the focus handling too.

But for this bug, should I return <body> for it? And what's a better of the method? GetEditingHost()?

FYI: The spec said as:

> If an element is editable and its parent element is not, or if an element is
> editable and it has no parent element, then the element is an editing host.
> And what's a better of the method?

I meant "what's a better name of the method?".

Comment 28

8 years ago
As roc mentioned in comment 23, according to the HTML5 spec, setting the designMode="on" in a document should be equivalent to setting the contenteditable attribute on the <body> element.  Therefore, I would expect the GetEditingHost method to return the body element when the document is in design mode.

As far as the name goes, GetEditingHost reflects the HTML5 terminology, and I think it's a good choice.
Created attachment 454951 [details] [diff] [review]
Patch v1.2
Attachment #452989 - Attachment is obsolete: true
Attachment #454951 - Flags: superreview?(roc)
Attachment #454951 - Flags: review+
Attachment #452989 - Flags: review?(roc)
Comment on attachment 454951 [details] [diff] [review]
Patch v1.2

oops, sorry for the spam, this patch includes unnecessary change.
Attachment #454951 - Flags: superreview?(roc)
Created attachment 454958 [details] [diff] [review]
Patch v1.2
Attachment #454951 - Attachment is obsolete: true
Attachment #454958 - Flags: superreview?(roc)
Attachment #454958 - Flags: review+
Comment on attachment 454958 [details] [diff] [review]
Patch v1.2

Sorry, the first request was not sr.
Attachment #454958 - Flags: superreview?(roc)
Attachment #454958 - Flags: review?(roc)
Attachment #454958 - Flags: review+
+nsEditor::IsActiveInDOMWindow()
+nsEditor::IsAcceptableInputEvent(nsIDOMEvent* aEvent)

Who calls these? why do we need them? Can we just have a single implementation that does what the nsHTMLEditor versions do?
(In reply to comment #33)
> +nsEditor::IsActiveInDOMWindow()
> +nsEditor::IsAcceptableInputEvent(nsIDOMEvent* aEvent)
> 
> Who calls these? why do we need them? Can we just have a single implementation
> that does what the nsHTMLEditor versions do?

nsEditorEventListener calls it when the editor is nsPlaintextEditor. nsEditorEventListener doesn't know whether the editor is nsPlaintextEditor or nsHTMLEditor. And also even if it's nsPlaintextEditor, we need to check focus in IsAcceptableInputEvent(). Furthermore, nsEditor::IsAcceptableInputEvent() needs to call inherited IsActiveInDOMWindow(). So, they should be separated methods.
+  // If the event is trusted, the event should always cause input.
+  nsCOMPtr<nsIDOMNSEvent> NSEvent = do_QueryInterface(aEvent);
+  NS_ENSURE_TRUE(NSEvent, PR_FALSE);

Why are we always allowing trusted events to cause input? Don't the user-input testcases in this bug involve trusted events?
Currently, all input events are accepted (handled) by *first* listening editor. The cause of this bug is, editor doesn't check the event target whether it's for the editor or another editor.

When I checked the behavior of WebKit at working on this bug, it ignores the input events when it's not focused. Therefore, I add the same limitation to the patch.

Note that all trusted input events always sent to a last focused content in a top level window. So, we don't need to worry about the unfocused case in synthesized event case.

So, we may be able to remove the check, but I'm not sure it doesn't cause any regressions.
In other words, the check keeps current behavior in synthesized input event cases.
Created attachment 457283 [details] [diff] [review]
Patch v1.2.1

updated for latest trunk.

Roc, please check the my reply (comment 36 and comment 37).
Attachment #454958 - Attachment is obsolete: true
Attachment #457283 - Flags: review?(roc)
Attachment #454958 - Flags: review?(roc)
Comment on attachment 457283 [details] [diff] [review]
Patch v1.2.1

The content parts need jst's review.
Attachment #457283 - Flags: superreview?(jst)
Attachment #457283 - Flags: review?(roc)
Attachment #457283 - Flags: review+
Comment on attachment 457283 [details] [diff] [review]
Patch v1.2.1

- In nsIContent::HasIndependentSelection():

+  nsIFrame* frame = static_cast<nsIContent*>(this)->GetPrimaryFrame();

No need for the static_cast here, this is already an nsIContent.

- In nsIContent::GetEditingHost():

+  if (doc->HasFlag(NODE_IS_EDITABLE)) {
+    nsCOMPtr<nsIContent> body = do_QueryInterface(doc->GetBodyElement());
+    return body;

No need for the nsCOMPtr there, just return doc->GetBodyElement() here, avoiding the reference count bump etc.

- In nsEditor::IsActiveInDOMWindow():

+  nsCOMPtr<nsIDocument> doc = do_QueryReferent(mDocWeak);
+  nsCOMPtr<nsPIDOMWindow> ourWindow = do_QueryInterface(doc->GetWindow());

I don't think ourWindow needs to be a nsCOMPtr here, just a raw nsPIDOMWindow* would do.

+  nsCOMPtr<nsPIDOMWindow> win;
+  nsCOMPtr<nsIContent> content =
+    nsFocusManager::GetFocusedDescendant(ourWindow, PR_FALSE,
+                                         getter_AddRefs(win));

No need for content to be an nsCOMPtr here.

- In nsHTMLEditor::IsActiveInDOMWindow():

+  nsCOMPtr<nsPIDOMWindow> ourWindow = do_QueryInterface(doc->GetWindow());
+  nsCOMPtr<nsPIDOMWindow> win;
+  nsCOMPtr<nsIContent> content =
+    nsFocusManager::GetFocusedDescendant(ourWindow, PR_FALSE,
+                                         getter_AddRefs(win));

Same here, ourWindow and content can be raw pointers.

sr=jst with that fixed.
Attachment #457283 - Flags: superreview?(jst) → superreview+
Created attachment 458893 [details] [diff] [review]
Patch v1.2.2 (for check-in)

Thank you, roc and jst.
http://hg.mozilla.org/mozilla-central/rev/616bd049acbc
Status: REOPENED → RESOLVED
Last Resolved: 8 years ago8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b3
backed-out, the patch needs approval. Sorry for the spam.
Status: RESOLVED → REOPENED
blocking2.0: --- → ?
Resolution: FIXED → ---
Comment on attachment 458893 [details] [diff] [review]
Patch v1.2.2 (for check-in)

This patch fixes the focus problem with contenteditable editor and input/textarea element. This fixes accessibility problem of them. And this includes many testcases for them.
Attachment #458893 - Flags: approval2.0?

Updated

8 years ago
blocking2.0: ? → betaN+

Updated

8 years ago
Attachment #458893 - Flags: approval2.0?

Comment 45

8 years ago
http://hg.mozilla.org/mozilla-central/rev/0eee576149df
Status: REOPENED → RESOLVED
Last Resolved: 8 years ago8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Oh, thanks a lot, Ehsan!
Target Milestone: mozilla2.0b3 → mozilla2.0b2

Comment 47

8 years ago
Thank _you_ for fixing this bug! :-)

BTW, this was landed after Beta 2 cut-off, setting the TM field to the correct value.
Target Milestone: mozilla2.0b2 → mozilla2.0b3
You need to log in before you can comment on or make changes to this bug.