RESOLVED FIXED 148019
Web Inspector: Highlight DOM node attribute changes in parallel, not sequentially
https://bugs.webkit.org/show_bug.cgi?id=148019
Summary Web Inspector: Highlight DOM node attribute changes in parallel, not sequenti...
Nikita Vasilyev
Reported 2015-08-13 23:12:34 PDT
Created attachment 258986 [details] [Animated GIF] Bug Steps: 1. Open http://n12v.com/focus-transition-2/ 2. Inspect the animated GIF 3. Click on the animated GIF Expected: Both "width" and "class" attributes are highlighted for the same duration. Actual: First "width" get highlighted for a fraction of a second (far less than 1s), then "class". Both attributes are never highlighted at the same time. On the attached GIF I changed the animation duration to 10s to exaggerate the effect.
Attachments
[Animated GIF] Bug (1.13 MB, image/gif)
2015-08-13 23:12 PDT, Nikita Vasilyev
no flags
Patch (2.74 KB, patch)
2015-08-14 12:27 PDT, Devin Rousso
no flags
Patch (2.71 KB, patch)
2015-08-14 13:20 PDT, Devin Rousso
no flags
Patch (2.95 KB, patch)
2015-08-14 13:41 PDT, Devin Rousso
no flags
Radar WebKit Bug Importer
Comment 1 2015-08-13 23:13:04 PDT
Radar WebKit Bug Importer
Comment 2 2015-08-13 23:15:04 PDT
Devin Rousso
Comment 3 2015-08-14 12:27:18 PDT
Devin Rousso
Comment 4 2015-08-14 13:20:07 PDT
Blaze Burg
Comment 5 2015-08-14 13:29:17 PDT
Comment on attachment 259024 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=259024&action=review > Source/WebInspectorUI/ChangeLog:3 > + Web Inspector: Highlight DOM node attribute changes in parallel, not consequent I fixed the bug title grammar, please ditto. > Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:1470 > + for (let i = 0; i < this._nodeStateChanges; ++i) { It makes me sad we have to duplicate our helper Array.prototype.remove but we can't do much better until arrows land. Maybe one day it could be: this._nodeStateChanges.removeIf((node) => element === node.element); > Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:1477 > + this._boundNodeChangedAnimationEnd = animationEnd.bind(this); Should only need to bind this once, right? > Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:1486 > element.classList.add("node-state-changed"); There (used to be) a bug where adding and removing a class would not cause the animation to restart, so we would have to use delayedWork. Is this no longer the case? Do new animations interrupt old ones?
Devin Rousso
Comment 6 2015-08-14 13:41:57 PDT
Blaze Burg
Comment 7 2015-08-14 14:17:07 PDT
Comment on attachment 259033 [details] Patch r=me
WebKit Commit Bot
Comment 8 2015-08-14 15:20:21 PDT
Comment on attachment 259033 [details] Patch Clearing flags on attachment: 259033 Committed r188496: <http://trac.webkit.org/changeset/188496>
WebKit Commit Bot
Comment 9 2015-08-14 15:20:26 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.