Bug 147973 – Web Inspector: Flash DOM node attribute on change

• Component: Web Inspector
• Status: RESOLVED FIXED

[Devin Rousso]

When an attribute of a DOM node changes, flash the value (if it doesn't exist, flash the name of the attribute) to indicate that it has changed.

[Devin Rousso]

Created attachment 258882 [details]
Patch

[Timothy Hatcher]

Comment on attachment 258882 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/DOMTreeUpdater.js:120
> +                    nodeItem.nodeGenerallyChanged = nodeGenerallyChanged;

Why not just pass this as an argument to updateTitle? updateTitle can pass it around internally. It seems odd to hang this temp data off the node tree element.

[Blaze Burg]

Comment on attachment 258882 [details]
Patch

'Generally' is a filler word here, and should be removed or replaced. Can you rename nodeGenerallyChanged to nodeStateChanged?

[Devin Rousso]

(In reply to comment #2)
> Comment on attachment 258882 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=258882&action=review
> 
> > Source/WebInspectorUI/UserInterface/Views/DOMTreeUpdater.js:120
> > +                    nodeItem.nodeGenerallyChanged = nodeGenerallyChanged;
> 
> Why not just pass this as an argument to updateTitle? updateTitle can pass
> it around internally. It seems odd to hang this temp data off the node tree
> element.

I wanted to use a temp item to allow for innerHTML change flashing to work too.  I tried this before and passing it through the title didn't work and added parameters to a lot of functions.  I feel that doing it this way doesn't add more clutter in the parameter passing down the chain.

[Devin Rousso]

Created attachment 258935 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/22275991>

[Joseph Pecoraro]

Comment on attachment 258935 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:199
> +    set nodeStateChanged(change)
> +    {
> +        this._nodeStateChanged = change;
> +    }

Could there be multiple attribute changes to a node in one batch of updates? If so, does this only cover one of the updates and not all of them?

> Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:1098
> +                let attrValueElement = document.createElement("a");

This scares me. We are shadowing attrValueElement with `let`. Why not just use a new local `linkElement`?

> Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:1471
> +        if (element.classList.contains("node-state-changed"))
> +            element.classList.remove("node-state-changed");

Seems we could just call remove, if it is not there nothing happens.

> Source/WebInspectorUI/UserInterface/Views/DOMTreeUpdater.js:122
> +                nodeItem.updateTitle();
>              }

Previously there would have been a `continue` here. Now there isn't. Is that okay?

[Devin Rousso]

Comment on attachment 258935 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:1098
>> +                let attrValueElement = document.createElement("a");
> 
> This scares me. We are shadowing attrValueElement with `let`. Why not just use a new local `linkElement`?

That was an accident.  It was only supposed to be attrValueElement and not have a let in front of it.  I used the same variable because then if the link changes for an attribute, it will still flash.

>> Source/WebInspectorUI/UserInterface/Views/DOMTreeUpdater.js:122
>>              }
> 
> Previously there would have been a `continue` here. Now there isn't. Is that okay?

I restructured this so that the continue is in the check above:  if (!nodeItem)

[Devin Rousso]

Created attachment 258956 [details]
Patch

[Timothy Hatcher]

Comment on attachment 258956 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:1466
> +            this.classList.remove("node-state-changed");

Does this handle rapid updates well? We have had issues with removing class name mid animation causing it to stop abruptly. Ideally the animation here would restart and darken back up and start to animate back to transparent.

> Source/WebInspectorUI/UserInterface/Views/DOMTreeUpdater.js:114
> +                let nodeItem = this._treeOutline.findTreeElement(node);

nodeTreeElement

[Devin Rousso]

(In reply to comment #10)
> > Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:1466
> > +            this.classList.remove("node-state-changed");
> 
> Does this handle rapid updates well? We have had issues with removing class
> name mid animation causing it to stop abruptly. Ideally the animation here
> would restart and darken back up and start to animate back to transparent.

Yes, this handles reselecting and restarts the animation in that case.

[Devin Rousso]

Created attachment 258966 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 258966 [details]
Patch

Clearing flags on attachment: 258966

Committed r188429: <http://trac.webkit.org/changeset/188429>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.