Closed Bug 733880 Opened 12 years ago Closed 8 years ago

[markup view] No context menu when selecting text

Categories

(DevTools :: Inspector, defect, P2)

x86
macOS
defect

Tracking

(firefox52 fixed)

RESOLVED FIXED
Firefox 52
Tracking Status
firefox52 --- fixed

People

(Reporter: erikvvold, Assigned: pbro, Mentored)

Details

(Keywords: dev-doc-complete)

Attachments

(1 file)

Steps to reproduce:

1. Inspect a element.
2. double left click on a attribute
3. select/highlight the attribute text
4. Right click

Expected:

A context menu with copy and paste options at least.

Actual:

Nothing happens, I'm forced to use the keyboard to copy the text..
(see also bug 703643)
Are you talking about the HTML panel?
I believe he's talking about the editable textarea that appears in the HTML panel when you double click an attribute value.

Is that accurate, Erik?
(In reply to Rob Campbell [:rc] (robcee) from comment #3)
> I believe he's talking about the editable textarea that appears in the HTML
> panel when you double click an attribute value.
> 
> Is that accurate, Erik?

Yes
(In reply to Paul Rouget [:paul] from comment #1)
> (see also bug 703643)

This looks similar, might be related.
We introduced a new menu. But it still doesn't include a "copy selection" item.
Summary: No context menu when selecting text → [markup view] No context menu when selecting text
Priority: -- → P2
I think the same happens with the rule-view (the Styles tab in the sidebar). The difference is that we have a nice context menu you can use to copy rules or properties or values, but as soon as you're focused inside an inplace editor, and text is selected, you should probably just be getting the OS default copy/paste/select all context menu. Same for the markup-view.
I've looked at this problem in more details. Here's what I think we should do:

On contextmenu event, we should check whether an input field has been focused, and if yes, we should just bail out, and let the browser/OS do their thing (that is, show the default text manipulation menu).

In order to do this, I see 3 steps:
- first of all, a preliminary step is required to clean the inspector a little bit. This isn't required, but would help make some code easier to read:
  The event listener for contextmenu and the _onContextMenu callback are defined inside devtools/client/inspector/inspector-panel.js, while they are really only used in the markup-view panel, so I think we should move this code to devtools/client/inspector/markup/markup.js

- secondly, add a helper function inside devtools/client/inspector/markup/markup.js called something like isEditing that returns true whenever one of the inplace editors is focused, false otherwise

- and finally, tweak _onContextMenu so that if isEditing return true, it early returns.

Next, the rule-view has a similar problem, and I would do the same thing (except there's no cleanup step needed for this one).
Inside devtools/client/inspector/rules/rules.js, add a similar isEditing helper function and then tweak the _onContextMenu callback so it early returns if isEditing returns true.

This isn't a really simple bug, but it's not hard either. So setting myself as mentor if anyone wants to take a look at it. Make sure you've gone through our contribution guide first, and let me know if you need any other pointers to the code to get started: https://developer.mozilla.org/en-US/docs/Tools/Contributing
Mentor: pbrosset
Assignee: nobody → pbrosset
Status: NEW → ASSIGNED
Comment on attachment 8794785 [details]
Bug 733880 - Add the missing copy/paste/cut menus on all text boxes in inspector;

https://reviewboard.mozilla.org/r/81084/#review79876

::: devtools/client/framework/toolbox.js:2227
(Diff revision 1)
>    },
>  
>    /**
>     * Enable / disable necessary textbox menu items using globalOverlay.js.
>     */
>    _updateTextboxMenuItems: function () {

nit: consistency _updateTextboxMenuItems > _updateTextBoxMenuItems (not used in any other file)

::: devtools/client/inspector/computed/computed.js:553
(Diff revision 1)
>    /**
>     * Context menu handler for filter style search box.
>     */
>    _onFilterTextboxContextMenu: function (event) {
> -    try {
> -      this.styleDocument.defaultView.focus();
> +    this.styleDocument.defaultView.focus();

nit: this.styleDocument.defaultView -> this.styleWindow

Also : Not directly related to your change, but did you check if this focus was actually needed. Manually testing, I don't see any difference but who knows... 

Getting rid of it would allow to use the same implementation as in other classes, so worth checking.

::: devtools/client/inspector/fonts/fonts.js:37
(Diff revision 1)
>      this.showAllLink.addEventListener("click", this.showAll);
>      this.previewTextChanged = this.previewTextChanged.bind(this);
> -    this.previewInput =
> +    this.previewInput = this.chromeDoc.getElementById("font-preview-text-input");
> -      this.chromeDoc.getElementById("font-preview-text-input");
>      this.previewInput.addEventListener("input", this.previewTextChanged);
> +    this.previewInput.addEventListener("contextmenu",

Just a side comment, there is no context menu anywhere in the font panel, except in the input thanks to your patch. 

I tried quickly to reuse the textboxcontextmenu for the whole panel, but it looks like the cut/paste commands get enabled when triggering it on regular text, so probably not the right way to do that.

But it would be a nice to get a basic context menu working for the font panel. Maybe a follow-up?

::: devtools/client/inspector/inspector-panel.js:922
(Diff revision 1)
>      });
>    },
>  
> +  /**
> +   * This is meant to be called by all the search, filter, inplace text boxes in the
> +   * inspector, and just calls through to the toolbox openTextboxContextMenu helper.

nit: openTextboxContextMenu -> openTextBoxContextMenu :)

::: devtools/client/inspector/inspector-panel.js:928
(Diff revision 1)
> +   * @param {DOMEvent} e
> +   */
> +  onTextBoxContextMenu: function (e) {
> +    e.stopPropagation();
> +    e.preventDefault();
> +    this.toolbox.openTextBoxContextMenu(e.screenX, e.screenY);

Thinking about devtools-html, I'm not sure we should be adding a new dependency from the inspector to the toolbox. Maybe we could fire an event instead. 

That being said, some inspector classes already had a dependency on the toolbox to get the textbox content menu. You just cleaned it up and wrapped it differently.

Not 100% sure about our plans for context-menus, let's check with Brian.

::: devtools/client/inspector/inspector-panel.js:1278
(Diff revision 1)
>  
>      // create tool iframe
>      this._markupFrame = doc.createElement("iframe");
>      this._markupFrame.setAttribute("flex", "1");
>      this._markupFrame.setAttribute("tooltip", "aHTMLTooltip");
> -    this._markupFrame.addEventListener("contextmenu", this._onContextMenu, true);
> +    this._markupFrame.addEventListener("contextmenu", this._onContextMenu, false);

false needed?

::: devtools/client/inspector/inspector-panel.js:1307
(Diff revision 1)
>    _destroyMarkup: function () {
>      let destroyPromise;
>  
>      if (this._markupFrame) {
>        this._markupFrame.removeEventListener("load", this._onMarkupFrameLoad, true);
> -      this._markupFrame.removeEventListener("contextmenu", this._onContextMenu, true);
> +      this._markupFrame.removeEventListener("contextmenu", this._onContextMenu, false);

false needed?

::: devtools/client/inspector/rules/rules.js:651
(Diff revision 1)
>    /**
>     * Context menu handler for filter style search box.
>     */
>    _onFilterTextboxContextMenu: function (event) {
> -    try {
> -      this.styleWindow.focus();
> +    this.styleWindow.focus();

Same comment as for computed.js, is the focus() really needed?

::: devtools/client/inspector/test/browser_inspector_textbox-menu.js:7
(Diff revision 1)
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +"use strict";
> +
> +// Test that various text boxes throughout the inspector have the expected textbox context
> +// menu (copy/cut/paste/selectAll/undo).

nit: comment: We're not testing the menu items (cut/copy etc...), but that we are using the context menu provided by the toolbox.
Comment on attachment 8794785 [details]
Bug 733880 - Add the missing copy/paste/cut menus on all text boxes in inspector;


> ::: devtools/client/inspector/inspector-panel.js:928
> (Diff revision 1)
> > +   * @param {DOMEvent} e
> > +   */
> > +  onTextBoxContextMenu: function (e) {
> > +    e.stopPropagation();
> > +    e.preventDefault();
> > +    this.toolbox.openTextBoxContextMenu(e.screenX, e.screenY);
> 
> Thinking about devtools-html, I'm not sure we should be adding a new
> dependency from the inspector to the toolbox. Maybe we could fire an event
> instead. 
> 
> That being said, some inspector classes already had a dependency on the
> toolbox to get the textbox content menu. You just cleaned it up and wrapped
> it differently.
> 
> Not 100% sure about our plans for context-menus, let's check with Brian.

Brian, what do you think about this?
Attachment #8794785 - Flags: review?(bgrinstead)
(In reply to Julian Descottes [:jdescottes] from comment #11)
> Thinking about devtools-html, I'm not sure we should be adding a new
> dependency from the inspector to the toolbox. Maybe we could fire an event
> instead. 
> 
> That being said, some inspector classes already had a dependency on the
> toolbox to get the textbox content menu. You just cleaned it up and wrapped
> it differently.
Yeah, I thought about that while doing the change. But as you said, since those dependencies already existed and I was just moving them around, I assumed this wasn't a problem.
You're right though, we want our tools to be mostly independent from each other and from the toolbox, so that we can load them on their own without the toolbox and the other tabs.
So, we should really have a shared module for this I guess.
This will impact Alex in bug 1297758 since he's in the process of creating a toolbox stub for that bug.
(In reply to Patrick Brosset <:pbro> from comment #13)
> (In reply to Julian Descottes [:jdescottes] from comment #11)
> > Thinking about devtools-html, I'm not sure we should be adding a new
> > dependency from the inspector to the toolbox. Maybe we could fire an event
> > instead. 
> > 
> > That being said, some inspector classes already had a dependency on the
> > toolbox to get the textbox content menu. You just cleaned it up and wrapped
> > it differently.
> Yeah, I thought about that while doing the change. But as you said, since
> those dependencies already existed and I was just moving them around, I
> assumed this wasn't a problem.

Yeah, no harm in taking incremental steps here.  Moving the references around and making individual tools rely more on the inspector's version of onTextBoxContextMenu seems fine with me. 

> You're right though, we want our tools to be mostly independent from each
> other and from the toolbox, so that we can load them on their own without
> the toolbox and the other tabs.
> So, we should really have a shared module for this I guess.

I wonder if this is something the toolbox can ultimately automatically handle for tools without the tool listening for the event and calling up to the toolbox.  So basically have the toolbox listen to context menu event on the frame then check e.target and see if it's a textbox, then open the context menu.  When loading in content, we won't need any special handling to show the normal context menu so that event just won't get bound.

If we want to use overlay-type features we'll need another solution, but I believe the inspector doesn't need this.
Comment on attachment 8794785 [details]
Bug 733880 - Add the missing copy/paste/cut menus on all text boxes in inspector;

I think Comment 14 addresses the request but let me know if you want me to review the code as well
Attachment #8794785 - Flags: review?(bgrinstead)
Comment on attachment 8794785 [details]
Bug 733880 - Add the missing copy/paste/cut menus on all text boxes in inspector;

https://reviewboard.mozilla.org/r/81084/#review80566

The new toolbox method is ok, looks good to me then :)
Attachment #8794785 - Flags: review?(jdescottes) → review+
(In reply to Julian Descottes [:jdescottes] from comment #11)
> ::: devtools/client/inspector/computed/computed.js:553
> (Diff revision 1)
> >    /**
> >     * Context menu handler for filter style search box.
> >     */
> >    _onFilterTextboxContextMenu: function (event) {
> > -    try {
> > -      this.styleDocument.defaultView.focus();
> > +    this.styleDocument.defaultView.focus();
> 
> nit: this.styleDocument.defaultView -> this.styleWindow
> 
> Also : Not directly related to your change, but did you check if this focus
> was actually needed. Manually testing, I don't see any difference but who
> knows... 
> 
> Getting rid of it would allow to use the same implementation as in other
> classes, so worth checking.
I've removed these extra callbacks from computed.js and rules.js, and pushed to try. We'll see.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=328fe8a21c2d

> ::: devtools/client/inspector/fonts/fonts.js:37
> (Diff revision 1)
> >      this.showAllLink.addEventListener("click", this.showAll);
> >      this.previewTextChanged = this.previewTextChanged.bind(this);
> > -    this.previewInput =
> > +    this.previewInput = this.chromeDoc.getElementById("font-preview-text-input");
> > -      this.chromeDoc.getElementById("font-preview-text-input");
> >      this.previewInput.addEventListener("input", this.previewTextChanged);
> > +    this.previewInput.addEventListener("contextmenu",
> 
> Just a side comment, there is no context menu anywhere in the font panel,
> except in the input thanks to your patch. 
> 
> I tried quickly to reuse the textboxcontextmenu for the whole panel, but it
> looks like the cut/paste commands get enabled when triggering it on regular
> text, so probably not the right way to do that.
> 
> But it would be a nice to get a basic context menu working for the font
> panel. Maybe a follow-up?
Filed bug 1306246.
Pushed by pbrosset@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/0f2c7b2a783b
Add the missing copy/paste/cut menus on all text boxes in inspector; r=jdescottes
https://hg.mozilla.org/mozilla-central/rev/0f2c7b2a783b
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
No longer blocks: top-inspector-bugs
I have reproduced this bug with Nightly 13.0a1 (2012-03-07) on Windows 7 , 64 Bit !

This bug's fix is verified with latest Nightly  

Build   ID  20161102030205
User Agent  Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:52.0) Gecko/20100101 Firefox/52.0
[bugday-20161102]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.