The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla14

Status

()

Core
Editor
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: graememcc, Assigned: graememcc)

Tracking

(Depends on: 1 bug)

Trunk
mozilla14
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

6 years ago
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
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?

Updated

6 years ago
Blocks: 578771
(Assignee)

Comment 2

5 years ago
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.
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)
(Assignee)

Comment 4

5 years ago
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Comment 10

5 years ago
Pushed the comment nit from comment 3:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c4a2b5f08e0
https://hg.mozilla.org/mozilla-central/rev/6c4a2b5f08e0
You need to log in before you can comment on or make changes to this bug.