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

Status: VERIFIED FIXED

(Firefox for Metro Graveyard :: Input, defect)

Description

[Sam Foster [:sfoster] (he/him)]

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

Comment 1

[Matt Brubeck (:mbrubeck)]

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

Comment 2

[Oleg Zasypkin [:azasypkin]]

Attachment: design mode v1.diff

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.

Comment 3

[Sam Foster [:sfoster] (he/him)]

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

Comment 4

[Matt Brubeck (:mbrubeck)]

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

Comment 5

[Oleg Zasypkin [:azasypkin]]

Attachment: design mode v2.diff

Reused Util.js, added automated tests + tested in touch environment (MS Windows Simulator).

Added follow-up bugs: 961702 and 961706.

Comment 6

[Sam Foster [:sfoster] (he/him)]

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

Comment 7

[Oleg Zasypkin [:azasypkin]]

Sure, will avoid long\complex ternaries in the future patches, thanks for review!

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/d8ab8158574d

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d8ab8158574d

Comment 10

[Cornel Ionce [:noni] [Hubs QA]]

Verified as fixed on latest Nightly (build ID: 20140226030202) and latest Aurora (build ID: 20140226004001) using a Surface Pro 2 device.