Closed Bug 947505 Opened 8 years ago Closed 8 years ago

Right-click in a designMode document should display a context menu


(Firefox for Metro Graveyard :: Input, defect)

Windows 8.1
Not set


(Not tracked)

Firefox 29


(Reporter: sfoster, Assigned: azasypkin)



(Whiteboard: [][lang=js] r=ff29)


(1 file, 1 obsolete file)

Follow-up from bug 927802, we need to add support for designMode in the ContentMenuHandler. 

* Load a page with designMode set to true. E.g.
* Select some text and right-click or long-tap to bring up a context menu

Expected results:
* A context menu appears, including cut/copy/paste items as appropriate for the selection

Actual results:
* The tab strip and appbar appear as if no text was selected
Whiteboard: [triage]
If anyone wants to work on this, the patch from bug 927802 will show you where the relevant code lives.  You'll need to build and run Firefox for Windows 8 Touch ("Metro") to test your work; see the developer docs at
Whiteboard: [triage] → [triage][][lang=js]
Blocks: metrobacklog
No longer blocks: metrov1backlog
Whiteboard: [triage][][lang=js] → [][lang=js]
Whiteboard: [][lang=js] → [][lang=js][defect]
Assignee: nobody → azasypkin
Attached patch design mode v1.diff (obsolete) — Splinter Review
Initial patch for design mode's context menu. I have several concerns though:

1. I would like to somehow join "isContentEditable && isOwnerDocumentIndesignMode" into single method to avoid repetition, and seems we always use both conditions together in this file. But we have one exception where we check for actual contenteditbale attribute value ("contentEditable" == "true"). So it's separate for now;

2. I've tried to compare behavior of desktop Fx and Metro Fx in design mode and with just contenteditable attribute for single node (ex. with and and noticed some differences (at least for selectAll and copy). This patch contains draft fix for selectAll context command, but Copy is a bit off for this initial patch.

3. Can't verify it on touch device as haven't received my surface pro.
Attachment #8361709 - Flags: review?(sfoster)
Attachment #8361709 - Flags: review?(mbrubeck)
Comment on attachment 8361709 [details] [diff] [review]
design mode v1.diff

Review of attachment 8361709 [details] [diff] [review]:

Discussed in IRC. Util.isEditableContent needs fixing for use here: if (aElement.isContentEditable || aElement.designMode == "on") should be if (aElement.isContentEditable || aElement.ownerDocument.designMode == "on")

If there are other bugs preventing testing, please mark them as blockers. For the designMode handling and logic, reference the implementation in desktop at: browser/base/content.nsContextMenu.js
Attachment #8361709 - Flags: review?(sfoster) → feedback+
Comment on attachment 8361709 [details] [diff] [review]
design mode v1.diff

Review of attachment 8361709 [details] [diff] [review]:

r+ after sfoster's comments are addressed
Attachment #8361709 - Flags: review?(mbrubeck) → review+
Blocks: 961702
Blocks: 961706
Reused Util.js, added automated tests + tested in touch environment (MS Windows Simulator).

Added follow-up bugs: 961702 and 961706.
Attachment #8361709 - Attachment is obsolete: true
Attachment #8362492 - Flags: review?(sfoster)
Comment on attachment 8362492 [details] [diff] [review]
design mode v2.diff

Review of attachment 8362492 [details] [diff] [review]:

Look great, tested manually and ran mochitest-metro - thanks for adding that test.

::: browser/metro/base/content/Util.js
@@ +91,2 @@
>    isEditableContent: function isEditableContent(aElement) {
> +    return !!aElement && (aElement.isContentEditable ||

I'm OK with this as it stands, but lets not be shy about using if/else: clarity over brevity
Attachment #8362492 - Flags: review?(sfoster) → review+
Sure, will avoid long\complex ternaries in the future patches, thanks for review!
Keywords: checkin-needed
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
Whiteboard: [][lang=js][defect] → [][lang=js] r=ff29
Keywords: verifyme
Verified as fixed on latest Nightly (build ID: 20140226030202) and latest Aurora (build ID: 20140226004001) using a Surface Pro 2 device.
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.