Bug 148019

Summary: Web Inspector: Highlight DOM node attribute changes in parallel, not sequentially
Product: WebKit Reporter: Nikita Vasilyev <nvasilyev>
Component: Web InspectorAssignee: Devin Rousso <hi>
Status: RESOLVED FIXED    
Severity: Normal CC: bburg, commit-queue, graouts, joepeck, mattbaker, nvasilyev, timothy, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
[Animated GIF] Bug
none
Patch
none
Patch
none
Patch none

Description Nikita Vasilyev 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.
Comment 1 Radar WebKit Bug Importer 2015-08-13 23:13:04 PDT
<rdar://problem/22283189>
Comment 2 Radar WebKit Bug Importer 2015-08-13 23:15:04 PDT
<rdar://problem/22283207>
Comment 3 Devin Rousso 2015-08-14 12:27:18 PDT
Created attachment 259024 [details]
Patch
Comment 4 Devin Rousso 2015-08-14 13:20:07 PDT
Created attachment 259031 [details]
Patch
Comment 5 BJ Burg 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?
Comment 6 Devin Rousso 2015-08-14 13:41:57 PDT
Created attachment 259033 [details]
Patch
Comment 7 BJ Burg 2015-08-14 14:17:07 PDT
Comment on attachment 259033 [details]
Patch

r=me
Comment 8 WebKit Commit Bot 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>
Comment 9 WebKit Commit Bot 2015-08-14 15:20:26 PDT
All reviewed patches have been landed.  Closing bug.