[webkit-reviews] review denied: [Bug 178996] Web Inspector: consolidate DOMTreeElement contextmenu items into submenus : [Attachment 325397] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Oct 31 14:01:36 PDT 2017


Joseph Pecoraro <joepeck at webkit.org> has denied Devin Rousso
<webkit at devinrousso.com>'s request for review:
Bug 178996: Web Inspector: consolidate DOMTreeElement contextmenu items into
submenus
https://bugs.webkit.org/show_bug.cgi?id=178996

Attachment 325397: Patch

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




--- Comment #11 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 325397
  --> https://bugs.webkit.org/attachment.cgi?id=325397
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?


More information about the webkit-reviews mailing list