Last Comment Bug 686203 - Focused contenteditable can prevent other elements from getting focused on right-click
: Focused contenteditable can prevent other elements from getting focused on ri...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: x86_64 Linux
: -- normal (vote)
: mozilla14
Assigned To: Graeme McCutcheon [:graememcc]
:
: Makoto Kato [:m_kato]
Mentors:
Depends on: 560201
Blocks: 578771
  Show dependency treegraph
 
Reported: 2011-09-11 04:16 PDT by Graeme McCutcheon [:graememcc]
Modified: 2012-04-27 06:52 PDT (History)
2 users (show)
ehsan: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Testcase (878 bytes, text/html)
2011-09-11 04:16 PDT, Graeme McCutcheon [:graememcc]
no flags Details
v1 (3.59 KB, patch)
2012-04-13 11:01 PDT, Graeme McCutcheon [:graememcc]
ehsan: review+
blassey.bugs: approval‑mozilla‑central+
Details | Diff | Splinter Review

Description Graeme McCutcheon [:graememcc] 2011-09-11 04:16:13 PDT
Created attachment 559757 [details]
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 :Ehsan Akhgari 2011-09-12 11:07:37 PDT
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] 2012-04-13 11:01:09 PDT
Created attachment 614858 [details] [diff] [review]
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 :Ehsan Akhgari 2012-04-16 12:31:15 PDT
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] 2012-04-17 10:43:59 PDT
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 :Ehsan Akhgari 2012-04-17 15:10:38 PDT
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 :Ehsan Akhgari 2012-04-17 15:11:22 PDT
Comment on attachment 614858 [details] [diff] [review]
v1

No risk for mobile, as the undo functionality does not exist there.
Comment 7 :Ehsan Akhgari 2012-04-17 15:15:58 PDT
(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 10 Graeme McCutcheon [:graememcc] 2012-04-26 01:52:31 PDT
Pushed the comment nit from comment 3:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c4a2b5f08e0
Comment 11 Ed Morley [:emorley] 2012-04-27 06:52:58 PDT
https://hg.mozilla.org/mozilla-central/rev/6c4a2b5f08e0

Note You need to log in before you can comment on or make changes to this bug.