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

VERIFIED FIXED in Firefox 29

Status

defect
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: sfoster, Assigned: azasypkin)

Tracking

unspecified
Firefox 29
x86_64
Windows 8.1
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=mbrubeck@mozilla.com][lang=js] r=ff29)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

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

STR: 
* Load a page with designMode set to true. E.g. http://www.kevinroth.com/rte/demo.htm
* 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 https://wiki.mozilla.org/Firefox/Windows_8_Integration
Whiteboard: [triage] → [triage][mentor=mbrubeck@mozilla.com][lang=js]
Blocks: metrobacklog
No longer blocks: metrov1backlog
Whiteboard: [triage][mentor=mbrubeck@mozilla.com][lang=js] → [mentor=mbrubeck@mozilla.com][lang=js]

Updated

5 years ago
Whiteboard: [mentor=mbrubeck@mozilla.com][lang=js] → [mentor=mbrubeck@mozilla.com][lang=js][defect]
Assignee: nobody → azasypkin
Status: NEW → ASSIGNED
Posted 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 jsfiddle.net and http://www.kevinroth.com/rte/demo.htm) 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)
(Reporter)

Comment 3

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

Comment 6

5 years ago
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
https://hg.mozilla.org/mozilla-central/rev/d8ab8158574d
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
Whiteboard: [mentor=mbrubeck@mozilla.com][lang=js][defect] → [mentor=mbrubeck@mozilla.com][lang=js] r=ff29

Updated

5 years ago
Keywords: verifyme
Verified as fixed on latest Nightly (build ID: 20140226030202) and latest Aurora (build ID: 20140226004001) using a Surface Pro 2 device.
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.