Bug 148019 - Web Inspector: Highlight DOM node attribute changes in parallel, not sequentially
Summary: Web Inspector: Highlight DOM node attribute changes in parallel, not sequenti...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-08-13 23:12 PDT by Nikita Vasilyev
Modified: 2015-08-14 15:20 PDT (History)
8 users (show)

See Also:


Attachments
[Animated GIF] Bug (1.13 MB, image/gif)
2015-08-13 23:12 PDT, Nikita Vasilyev
no flags Details
Patch (2.74 KB, patch)
2015-08-14 12:27 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (2.71 KB, patch)
2015-08-14 13:20 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (2.95 KB, patch)
2015-08-14 13:41 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.