927802 - Defect - Right-click in a contentEditable element should display a context menu

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: Input, defect, P2)

Description

[horvandillus]

Attachment: no-context-menue.jpg

User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:27.0) Gecko/20100101 Firefox/27.0 (Beta/Release)
Build ID: 20131006030201

Steps to reproduce:

While trying to make some spell checks in my mails 


Actual results:

I noticed that there is no change to get a right click menu in Metro style except you mark a line to copy it to your clipboard. So there is no change for the user tu use any option that may be usable and accessible when it is only available in the context menue. 


Expected results:

Instead of displaying the adress bar and the website tabs what is the normal behavior in Metro styled apps there should be a button or an option that says "Hey Im your friendly context menu and I'm still HERE" If this is not fixed webapps like text editors mail applications and any other application you could imagine that needs some context menu options are not accessible in metro style mode.

Comment 1

[Matt Brubeck (:mbrubeck)]

When I use Metro Firefox to right-click in an editable text input or textarea (like the ones on this page) I get a context menu with a "Paste" command.  However, this does not work for "rich" text editors that use the contentEditable property.  We should fix this.

Comment 2

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

Attachment: Hook up contextmenu for contenteditables, fix cut, paste handling for contenteditables

I /think/ I have the logic right here for getting the contextmenu commands showing up for contenteditable elements, but its a bit of a minefield, my refactoring notwithstanding. 

I also hooked up the cut and paste handling for contenteditables. 

I'm seeing a little flakiness while testing this, with the contextmenu sometimes showing up, sometimes not, which I think may be a distinct input/event issue but let me know if you spot anything

Comment 3

[Matt Brubeck (:mbrubeck)]

Comment on attachment 828380 [details] [diff] [review]
Hook up contextmenu for contenteditables, fix cut, paste handling for contenteditables

Review of attachment 828380 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, though I think we'll have some follow-up work to do.

::: browser/metro/base/content/contenthandlers/ContextMenuHandler.js
@@ +255,5 @@
>            isText = true;
>            break;
> +        }
> +        // is the target contentEditable (not just inheriting contentEditable)
> +        else if (elem.contentEditable == "true") {

We should file a follow-up bug to support designMode too.

@@ +262,5 @@
> +          isText = true;
> +          uniqueStateTypes.add("input-text");
> +
> +          if (elem.textContent.length) {
> +            uniqueStateTypes.add("select-all");

I think this should be "selectable".  Though the selection commands didn't actually work for me when I tested them in contentEditable, so maybe we should just comment this out until they do.

@@ +365,2 @@
>  
> +    state.types = [type for (type of uniqueStateTypes)];

This is fine, though if you want to further refactor the code that reads this to use a Set, that would be great too.

::: browser/metro/modules/ContentUtil.jsm
@@ +121,5 @@
>  
>      // Return the modified object
>      return target;
> +  },
> +  isSelectionWithinElement: function(selection, elem) {

It looks like this isn't used here.  Do you plan to use it in a future patch?

Comment 4

[Marco Mucci [:MarcoM]]

Hey Sam, can you provide a point value.

Comment 5

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

p=2

Comment 6

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

Carried r+ from mbrubeck and landed on fx-team: https://hg.mozilla.org/integration/fx-team/rev/d0f6fb31e982

I removed the unused isSelectionWithinElement from ContentUtil. I didn't try to fix the .types issue, at least a couple of bindings expect this to be an array which seemed like side-effects we could do without right now. 

Follow-up filed for the designMode support as bug 947505.

Comment 7

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

Oh, also the "selectable" note - I left this in as these commands do seem to work for touch, just not with the mouse. As we're touch-first it seemed useful to have.

Comment 8

[Marco Mucci [:MarcoM]]

Moving to IT#20 as patch is landing.

Comment 9

[Carsten Book [:Tomcat]]

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

Comment 10

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

Mozilla/5.0 (Windows NT 6.2; rv:29.0) Gecko/20100101 Firefox/29.0

Verified as fixed on desktop metro (build ID: 20131215030202) as bug 947505 was filled as a follow-up for the remaining issues.

This issue also needs to be verified using a touch device.