[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