686203 - Focused contenteditable can prevent other elements from getting focused on right-click

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Graeme McCutcheon [:graememcc]]

Attachment: Testcase

In another bug, I stated that the bug shown in the testcase should be fixed by bug 682338. I was wrong.

In the testcase, focusing the content-editable, and then right-clicking in the input, you get a context menu whose options reflect the state of the div rather than the input - indeed the actions operate on the div, not the input.

On platforms where the context menu is shown on mouse down, nsXULPopupListener explicitly focuses the target to ensure the context menu commands are dispatched to the correct element. This doesn't need to happen on platforms where the menu is shown on mouse button up, as the target should be focused by the event state manager's PostHandleEvent code.

However, in the testcase, the contenteditable's editor event listeners consume the mouse down event, which prevents the input from getting focused by the esm.
See http://hg.mozilla.org/mozilla-central/file/06b2977afb85/editor/libeditor/html/nsHTMLEditorEventListener.cpp#l244

Comment 1

[(no longer active)]

I can't reproduce this on Mac with a build before bug 682338 was landed on mozilla-central...

(In reply to Graeme McCutcheon from comment #0)
> However, in the testcase, the contenteditable's editor event listeners
> consume the mouse down event, which prevents the input from getting focused
> by the esm.
> See
> http://hg.mozilla.org/mozilla-central/file/06b2977afb85/editor/libeditor/
> html/nsHTMLEditorEventListener.cpp#l244

Hmm, we should probably call IsNodeInActiveEditor there and ignore events on non-editable stuff completely (same thing for MouseUp too).  Are you willing to write a patch for that?

Comment 2

[Graeme McCutcheon [:graememcc]]

Attachment: v1

>(same thing for MouseUp too)

Just MouseDown. A similar check on MouseUp would break object resizing.

I delayed posting this for a few months as I kept running into 597331 and 600570 failures on Win7 on try, but those have since became random-orange bug 718316 with no interference from me. Currently green on try modulo other random-orange.

Comment 3

[(no longer active)]

Comment on attachment 614858 [details] [diff] [review]
v1

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

::: editor/libeditor/html/nsHTMLEditorEventListener.cpp
@@ +141,4 @@
>    NS_ENSURE_TRUE(target, NS_ERROR_NULL_POINTER);
>    nsCOMPtr<nsIDOMElement> element = do_QueryInterface(target);
>  
> +  // Contenteditable should disregard mousedowns outwith it

Nit: outside.

::: editor/libeditor/html/tests/test_bug686203.html
@@ +30,5 @@
> +        var input = document.getElementById("input");
> +        ce.focus();
> +      
> +        var eventDetails = { button : 2 };
> +        synthesizeMouseAtCenter(input, eventDetails);

Doesn't this cause a context menu to get opened?  If so, then how would the rest of the test proceed without problems?

Comment 4

[Graeme McCutcheon [:graememcc]]

Context menus are opened on "contextmenu" mouse events. The various nsWindow implementations dispatch them on real mouse clicks, but synthesized events don't unless you specifically ask for it (for example, see openContextMenu in browser/base/tests/test_contextMenu.js)

Comment 5

[(no longer active)]

Comment on attachment 614858 [details] [diff] [review]
v1

OK, then this is fine.  r=me with the nit in comment 3 fixed.

Thanks!

Comment 6

[(no longer active)]

Comment on attachment 614858 [details] [diff] [review]
v1

No risk for mobile, as the undo functionality does not exist there.

Comment 7

[(no longer active)]

(In reply to Ehsan Akhgari [:ehsan] from comment #6)
> Comment on attachment 614858 [details] [diff] [review]
> v1
> 
> No risk for mobile, as the undo functionality does not exist there.

Ignore the second half of that sentence!

Comment 8

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/26679372aaf4

Comment 9

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/26679372aaf4

Comment 10

[Graeme McCutcheon [:graememcc]]

Pushed the comment nit from comment 3:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c4a2b5f08e0

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/6c4a2b5f08e0