Bug 178996 - Web Inspector: consolidate DOMTreeElement contextmenu items into submenus
Summary: Web Inspector: consolidate DOMTreeElement contextmenu items into submenus
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks: 179042
  Show dependency treegraph
 
Reported: 2017-10-30 01:03 PDT by Devin Rousso
Modified: 2017-11-15 12:34 PST (History)
4 users (show)

See Also:


Attachments
Patch (21.37 KB, patch)
2017-10-30 01:08 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
[Image] After Patch is applied (82.03 KB, image/png)
2017-10-30 01:08 PDT, Devin Rousso
no flags Details
[Image] Before (144.71 KB, image/png)
2017-10-30 18:07 PDT, Devin Rousso
no flags Details
Patch (26.65 KB, patch)
2017-10-30 18:08 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
[Image] After Patch is applied (161.83 KB, image/png)
2017-10-30 18:08 PDT, Devin Rousso
no flags Details
Patch (26.57 KB, patch)
2017-10-31 14:58 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (26.56 KB, patch)
2017-11-01 20:11 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2017-10-30 01:03:53 PDT
Having different sections each have a different "Copy _" item is confusing.  We should merge them into a "Copy" submenu.
Comment 1 Devin Rousso 2017-10-30 01:08:24 PDT
Created attachment 325338 [details]
Patch
Comment 2 Devin Rousso 2017-10-30 01:08:37 PDT
Created attachment 325339 [details]
[Image] After Patch is applied
Comment 3 Joseph Pecoraro 2017-10-30 11:59:56 PDT
You really need to include Before screenshots. The ability to compare side by side images is what makes screenshots for changes useful.
Comment 4 Joseph Pecoraro 2017-10-30 12:11:27 PDT
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.
Comment 5 Joseph Pecoraro 2017-10-30 12:27:49 PDT
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.
Comment 6 Devin Rousso 2017-10-30 17:35:10 PDT
(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 :)
Comment 7 Devin Rousso 2017-10-30 18:07:33 PDT
Created attachment 325396 [details]
[Image] Before
Comment 8 Devin Rousso 2017-10-30 18:08:06 PDT
Created attachment 325397 [details]
Patch
Comment 9 Devin Rousso 2017-10-30 18:08:27 PDT
Created attachment 325399 [details]
[Image] After Patch is applied
Comment 10 Joseph Pecoraro 2017-10-31 13:44:27 PDT
> > 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?
Comment 11 Joseph Pecoraro 2017-10-31 14:01:36 PDT
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?
Comment 12 Devin Rousso 2017-10-31 14:57:43 PDT
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`).
Comment 13 Devin Rousso 2017-10-31 14:58:55 PDT
Created attachment 325502 [details]
Patch
Comment 14 Joseph Pecoraro 2017-11-01 16:59:13 PDT
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.
Comment 15 Devin Rousso 2017-11-01 20:11:29 PDT
Created attachment 325669 [details]
Patch
Comment 16 WebKit Commit Bot 2017-11-01 20:34:47 PDT
Comment on attachment 325669 [details]
Patch

Clearing flags on attachment: 325669

Committed r224314: <https://trac.webkit.org/changeset/224314>
Comment 17 WebKit Commit Bot 2017-11-01 20:34:49 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 Radar WebKit Bug Importer 2017-11-15 12:34:18 PST
<rdar://problem/35567775>