Bug 178996 – Web Inspector: consolidate DOMTreeElement contextmenu items into submenus

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Devin Rousso]

Having different sections each have a different "Copy _" item is confusing.  We should merge them into a "Copy" submenu.

[Devin Rousso]

Created attachment 325338 [details]
Patch

[Devin Rousso]

Created attachment 325339 [details]
[Image] After Patch is applied

[Joseph Pecoraro]

You really need to include Before screenshots. The ability to compare side by side images is what makes screenshots for changes useful.

[Joseph Pecoraro]

Based on the screenshot it looks like we could end up with submenus with a single option. That seems kind of weird, but I do very much like the groupings.

I do think Copy Link Address should probably stay top level. I think of the Add/Edit/Copy/Delete as properties of the node, but the link is not a property of the node.

[Joseph Pecoraro]

I think my ideal order for an element would end up like:

    Log Element
    Capture Screenshot
    Scroll into View
    ----------------
    Add (HTML, Attribute?, Child)
    Edit (HTML, Attribute?, Tag?)
    Copy (HTML, Attribute?, Selector, XPath)
    Delete
    Hide
    ----------------
    :active
    :hover
    ...
    ----------------
    Break on...

A few are modeled after Chrome. I like that Chrome puts the pseudo-element states at the top level, its easy to see immediately which states are activated (checkmarks). It also makes the Show/Hide element (H keyboard shortcut) discoverable without users knowing that "H" exists.

The Foo? syntax means that a Foo would be available when it is applicable. I'd hate for the Add/Edit menus to have just 1 value "HTML" most of the time, because if that is the case then a non-submenu would work better.

[Devin Rousso]

(In reply to Joseph Pecoraro from comment #5)
> I think my ideal order for an element would end up like:
> 
>     Log Element
>     Capture Screenshot
>     Scroll into View
>     ----------------
>     Add (HTML, Attribute?, Child)
>     Edit (HTML, Attribute?, Tag?)
>     Copy (HTML, Attribute?, Selector, XPath)
>     Delete
>     Hide
>     ----------------
>     :active
>     :hover
>     ...
>     ----------------
>     Break on...
> 
> A few are modeled after Chrome. I like that Chrome puts the pseudo-element
> states at the top level, its easy to see immediately which states are
> activated (checkmarks). It also makes the Show/Hide element (H keyboard
> shortcut) discoverable without users knowing that "H" exists.
This no longer appears to be the case.  Chrome Canary puts them in a "Force State" menu.  I personally prefer them in a submenu, as most of the time I don't need them.  Also, you can immediately see whether a node has a forced pseudo-state by looking at whether there is a little blue dot on the far left of the node's row.

Personally, I don' find "Log Element", "Capture Screenshot", and "Scroll into View" to be that useful.  I typically use `$0`, and I'm not even sure I've used the latter two (outside of implementing/testing).  I would rather have them lower in the list, but I could be convinced otherwise.

> The Foo? syntax means that a Foo would be available when it is applicable.
> I'd hate for the Add/Edit menus to have just 1 value "HTML" most of the
> time, because if that is the case then a non-submenu would work better.
So you're saying have them visible, but disabled?  If so, I like that idea :)

[Devin Rousso]

Created attachment 325396 [details]
[Image] Before

[Devin Rousso]

Created attachment 325397 [details]
Patch

[Devin Rousso]

Created attachment 325399 [details]
[Image] After Patch is applied

[Joseph Pecoraro]

> > The Foo? syntax means that a Foo would be available when it is applicable.
> > I'd hate for the Add/Edit menus to have just 1 value "HTML" most of the
> > time, because if that is the case then a non-submenu would work better.
>
> So you're saying have them visible, but disabled?  If so, I like that idea :)

I considered this and originally came down against it, but I could go either way. I think it would be weird if you right click in a an Element's tag:

    <div id="my-div">
      ^
      |

And you get "Attribute" in the Edit menu:

    Edit > HTML
           Attribute
           Tag

But maybe if it is disabled that would be fine. It would indicate to users hey, if you context menu on an Attribute this would be enabled.

Likewise "Tag" on things like a TextNode. Perhaps we should have a "Text" value for that case?:

    Edit > HTML
           Attribute?
           Tag?
           Text?

[Joseph Pecoraro]

Comment on attachment 325397 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=325397&action=review

> Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:286
> +    hideElement()

This should be hide/show element. Since it toggles. Maybe "toggleElementVisibility" is the best name? r- for the questions surrounding this.

> Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:766
> +        contextMenu.appendItem(WI.UIString("Hide"), this.hideElement.bind(this));

This can't just say Hide. Since it toggles visibility. Some options are:

1. Hide / Show change name:

    When element is visible:
        Hide
    When element is hidden:
        Show

2. Hide with checkmark:

    When element is visible:
        Hide
    When element is hidden:
     ✔  Hide

3. Single item "Toggle Visibility" that never changes.

    Always:
        Toggle Visibility

> Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:784
> +            let hasNonWordCharacters = /\w/.test(attributeValue);

The variable has the exact opposite name of what it is doing. Did you mean \W?

That said, I think we should always quote the value. If I see `<input type=text>` our UI will be showing `<input type="text">` I'd expect to get: `type="text"` in my clipboard. Or maybe even just the value.

I think we should probably make this work for non-value attributes. If someone has <div data-super-duper-long-attribute-name-without-value>...</div> I'd like to be able to copy that attribute. Right now the option looks like it is disabled if !attributeValue.

> Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:800
> +        subMenus.edit.appendItem(WI.UIString("Tag"), () => {
> +            this._startEditingTagName();
>          });

I suspect we shouldn't be able to edit the Tag of Shadow Element content.

Does this code get reached for a Comment Node / Text Node?

[Devin Rousso]

Comment on attachment 325397 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=325397&action=review

>> Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:766
>> +        contextMenu.appendItem(WI.UIString("Hide"), this.hideElement.bind(this));
> 
> This can't just say Hide. Since it toggles visibility. Some options are:
> 
> 1. Hide / Show change name:
> 
>     When element is visible:
>         Hide
>     When element is hidden:
>         Show
> 
> 2. Hide with checkmark:
> 
>     When element is visible:
>         Hide
>     When element is hidden:
>      ✔  Hide
> 
> 3. Single item "Toggle Visibility" that never changes.
> 
>     Always:
>         Toggle Visibility

Toggle Visibility makes the most sense to me.  From an implementation standpoint, it's also the easiest since the "__WebInspectorHideElement__" class can be manually removed by the user, which makes keeping track of the "force-hidden" state very very tricky.

>> Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:784
>> +            let hasNonWordCharacters = /\w/.test(attributeValue);
> 
> The variable has the exact opposite name of what it is doing. Did you mean \W?
> 
> That said, I think we should always quote the value. If I see `<input type=text>` our UI will be showing `<input type="text">` I'd expect to get: `type="text"` in my clipboard. Or maybe even just the value.
> 
> I think we should probably make this work for non-value attributes. If someone has <div data-super-duper-long-attribute-name-without-value>...</div> I'd like to be able to copy that attribute. Right now the option looks like it is disabled if !attributeValue.

I completely forgot about value-less attributes!  Good catch :)

>> Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:800
>>          });
> 
> I suspect we shouldn't be able to edit the Tag of Shadow Element content.
> 
> Does this code get reached for a Comment Node / Text Node?

Comment nodes go through `_populateNodeContextMenu`.
Text nodes go through `_populateTextContextMenu` (which also calls `_populateNodeContextMenu`).

[Devin Rousso]

Created attachment 325502 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 325502 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=325502&action=review

r=me. Its very hard to read the diff with all the line/indentation change.

> Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:786
> +        }, !isNonShadowEditable);

I think you will still want to disable this if !attribute. Otherwise I don't know what this does. (For example if you right click on the tag name).

> Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:790
> +        }, !attributeValue || !isNonShadowEditable);

Same idea here, you should be able to remove an attribute even if it doesn't have an attributeValue. So probably if !attributeName instead of !attributeValue.

[Devin Rousso]

Created attachment 325669 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 325669 [details]
Patch

Clearing flags on attachment: 325669

Committed r224314: <https://trac.webkit.org/changeset/224314>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35567775>