389372 - Contenteditable node is still editable without focus

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Albert Brand]

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.

Comment 1

[Albert Brand]

Attachment: Testcase

Comment 2

[Martijn Wargers (dead)]

Confirmed.
This is not what IE7 is doing.

Comment 3

[Tim]

Attachment: 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.

Comment 4

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

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.

Comment 5

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

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.

Comment 7

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

Attachment: Patch v1.0

Comment 8

[(no longer active)]

This is a dupe of bug 567213, which has already been fixed on trunk.

Comment 9

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

(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

[(no longer active)]

(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?

Comment 11

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

(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

[(no longer active)]

OK then.  Let's take your patch!  :-)  Want me to do a drive-by review?

Comment 13

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

Attachment: 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.

Comment 14

[(no longer active)]

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.

Comment 15

[(no longer active)]

(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?

Comment 16

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

> 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).

Comment 17

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

> 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.

Comment 18

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

Attachment: Patch v1.1

Comment 19

[(no longer active)]

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

r=me, sans the content and IME parts.

Comment 20

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

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

Roc:

Would you review the content part and IME tests in the new tests?

Comment 21

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

+  /**
+   * 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.

Comment 22

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

(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.

Comment 23

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

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.

Comment 24

[Martijn Wargers (dead)]

But there is large behavior difference, by default. In designMode, javascript isn't executed, while in contenteditable mode, it is.

Comment 25

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

Right now that's true, but we're going to change that. See bug 519928.

Comment 26

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

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.

Comment 27

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

> And what's a better of the method?

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

Comment 28

[(no longer active)]

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.

Comment 29

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

Attachment: Patch v1.2

Comment 30

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

Comment on attachment 454951 [details] [diff] [review]
Patch v1.2

oops, sorry for the spam, this patch includes unnecessary change.

Comment 31

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

Attachment: Patch v1.2

Comment 32

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

Comment on attachment 454958 [details] [diff] [review]
Patch v1.2

Sorry, the first request was not sr.

Comment 33

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

+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?

Comment 34

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

(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.

Comment 35

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

+  // 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?

Comment 36

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

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.

Comment 37

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

In other words, the check keeps current behavior in synthesized input event cases.

Comment 38

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

Attachment: Patch v1.2.1

updated for latest trunk.

Roc, please check the my reply (comment 36 and comment 37).

Comment 39

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

Comment on attachment 457283 [details] [diff] [review]
Patch v1.2.1

The content parts need jst's review.

Comment 40

[Johnny Stenback (:jst)]

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.

Comment 41

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

Attachment: Patch v1.2.2 (for check-in)

Thank you, roc and jst.

Comment 42

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

http://hg.mozilla.org/mozilla-central/rev/616bd049acbc

Comment 43

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

backed-out, the patch needs approval. Sorry for the spam.

Comment 44

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

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.

Comment 45

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/0eee576149df

Comment 46

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

Oh, thanks a lot, Ehsan!

Comment 47

[(no longer active)]

Thank _you_ for fixing this bug! :-)

BTW, this was landed after Beta 2 cut-off, setting the TM field to the correct value.