Closed Bug 686203 Opened 9 years ago Closed 8 years ago

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

Categories

(Core :: DOM: Editor, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla14

People

(Reporter: graememcc, Assigned: graememcc)

References

(Depends on 1 open bug)

Details

Attachments

(2 files)

Attached file 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
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?
Blocks: 578771
Attached patch v1Splinter Review
>(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.
Attachment #614858 - Flags: review?(ehsan)
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?
Attachment #614858 - Flags: review?(ehsan)
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 on attachment 614858 [details] [diff] [review]
v1

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

Thanks!
Attachment #614858 - Flags: review+
Comment on attachment 614858 [details] [diff] [review]
v1

No risk for mobile, as the undo functionality does not exist there.
Attachment #614858 - Flags: approval-mozilla-central?
(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!
Attachment #614858 - Flags: approval-mozilla-central? → approval-mozilla-central+
https://hg.mozilla.org/integration/mozilla-inbound/rev/26679372aaf4
Assignee: nobody → graememcc_firefox
Flags: in-testsuite+
Target Milestone: --- → mozilla14
https://hg.mozilla.org/mozilla-central/rev/26679372aaf4
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.