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.
Created attachment 258882 [details] Patch
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.
Comment on attachment 258882 [details] Patch 'Generally' is a filler word here, and should be removed or replaced. Can you rename nodeGenerallyChanged to nodeStateChanged?
(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.
Created attachment 258935 [details] Patch
<rdar://problem/22275991>
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?
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)
Created attachment 258956 [details] Patch
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
(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.
Created attachment 258966 [details] Patch
Comment on attachment 258966 [details] Patch Clearing flags on attachment: 258966 Committed r188429: <http://trac.webkit.org/changeset/188429>
All reviewed patches have been landed. Closing bug.