Closed Bug 927802 Opened 6 years ago Closed 6 years ago

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

Categories

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

27 Branch
defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 28

People

(Reporter: horvandillus, Assigned: sfoster)

References

Details

(Whiteboard: [beta28] feature=defect c=tbd u=tbd p=2)

Attachments

(2 files)

Attached image 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.
Summary: There is no right click menue if you don't mark something → Defect - There is no right click menue if you don't mark something
Whiteboard: [triage] feature=defect c=tbd u=tbd p=0
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Defect - There is no right click menue if you don't mark something → Defect - Right-click in a contentEditable element should display a context menu
Whiteboard: [triage] feature=defect c=tbd u=tbd p=0 → feature=defect c=tbd u=tbd p=0
Whiteboard: feature=defect c=tbd u=tbd p=0 → [release28] feature=defect c=tbd u=tbd p=0
No longer blocks: 855434
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
Assignee: nobody → sfoster
Attachment #828380 - Flags: review?(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?
Attachment #828380 - Flags: review?(mbrubeck) → review+
Whiteboard: [release28] feature=defect c=tbd u=tbd p=0 → [beta28] feature=defect c=tbd u=tbd p=0
Blocks: metrov1it21
No longer blocks: metrov1backlog
Status: NEW → ASSIGNED
Priority: -- → P2
QA Contact: jbecerra
Hey Sam, can you provide a point value.
Flags: needinfo?(sfoster)
p=2
Flags: needinfo?(sfoster)
Whiteboard: [beta28] feature=defect c=tbd u=tbd p=0 → [beta28] feature=defect c=tbd u=tbd p=2
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.
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.
Moving to IT#20 as patch is landing.
Blocks: metrov1it20
No longer blocks: metrov1it21
https://hg.mozilla.org/mozilla-central/rev/d0f6fb31e982
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 28
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.
You need to log in before you can comment on or make changes to this bug.