Bug 148049 - Web Inspector: Unexpectedly frequent flashing of DOM node attributes
Summary: Web Inspector: Unexpectedly frequent flashing of DOM node attributes
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-14 18:59 PDT by Nikita Vasilyev
Modified: 2019-03-02 16:19 PST (History)
6 users (show)

See Also:


Attachments
[Animated GIF] Bug (340.28 KB, image/gif)
2015-08-14 18:59 PDT, Nikita Vasilyev
no flags Details
[HTML] Reduction (441 bytes, text/html)
2015-08-14 18:59 PDT, Nikita Vasilyev
no flags Details
[Animated GIF] Expected behavior (285.47 KB, image/gif)
2015-08-14 19:05 PDT, Nikita Vasilyev
no flags Details
[Animated GIF] Bug 2 (50.16 KB, image/gif)
2015-08-14 19:13 PDT, Nikita Vasilyev
no flags Details
Patch (6.09 KB, patch)
2018-09-21 10:54 PDT, Devin Rousso
hi: commit-queue-
Details | Formatted Diff | Diff
Patch (5.66 KB, patch)
2018-12-27 18:26 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (5.84 KB, patch)
2019-03-02 15:40 PST, 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-14 18:59:09 PDT
Created attachment 259067 [details]
[Animated GIF] Bug

See the attached GIF.

I changed the highlight color to black to exaggerate the effect.

For span#time, the animation duration is 1 second, as expected.
All the other elements blink black and white close to 60 times per second.

span#time is different from the rest as it doesn't have any immediate text nodes changing. #minutes, #seconds, and #ms change 60 times per second, so their attributes should always stay highlighted.
Comment 1 Radar WebKit Bug Importer 2015-08-14 18:59:19 PDT
<rdar://problem/22296830>
Comment 2 Nikita Vasilyev 2015-08-14 18:59:48 PDT
Created attachment 259068 [details]
[HTML] Reduction
Comment 3 Devin Rousso 2015-08-14 19:05:14 PDT
I think that this is more of an issue with the fact that we recreate the DOM tree element anytime there is any sort of change, even if that change happens to not change anything (for example, changing the title of minute from 1 to 1).  I'll take a look and see if I can add anything to either prevent this from happening or limit the number of DOM update animations possible per given period of time.
Comment 4 Nikita Vasilyev 2015-08-14 19:05:25 PDT
Created attachment 259070 [details]
[Animated GIF] Expected behavior

Same in Chrome DevTools.
Comment 5 Nikita Vasilyev 2015-08-14 19:13:53 PDT
Created attachment 259072 [details]
[Animated GIF] Bug 2

(In reply to comment #3)
> I think that this is more of an issue with the fact that we recreate the DOM
> tree element anytime there is any sort of change, even if that change
> happens to not change anything (for example, changing the title of minute
> from 1 to 1).  I'll take a look and see if I can add anything to either
> prevent this from happening or limit the number of DOM update animations
> possible per given period of time.

There are two issues: one that you mentioned and one shown on this attached GIF. Even though we don't change textContent, span#ms still blinks and not stays highlighted.
Comment 6 Timothy Hatcher 2015-08-15 08:36:56 PDT
Maybe we should avoid sending DOM change notifications from the backend if the text does not change value. I guess it is getting a new text node, so that might not work. At least then front end should not treat it as a change. Though, that is what the page is doing…
Comment 7 Timothy Hatcher 2015-08-15 08:45:58 PDT
Devin is right, the issue is the tree element is recreated for the text child node change. Then the title attribute changes. When the text child node changes again, the existing attribute change is lost when the parent tree element is recreated again. Rinse and repeat. If only the inner text node is changing, we should be able to update the tree element instead of recreating the parent tree element.
Comment 8 Timothy Hatcher 2015-08-15 08:48:18 PDT
This wouldn't be an issue for long text nodes, since we show those as separate tree elements. I suspect Chrome is doing smart small text node updating when it is combined with the parent node.
Comment 9 Devin Rousso 2018-09-21 10:54:44 PDT
Created attachment 350384 [details]
Patch

When run from the console, this works fine, but it doesn't have an effect when run inside DOMTreeElement.js 🤔
Comment 10 Devin Rousso 2018-12-27 18:26:15 PST
Created attachment 358113 [details]
Patch
Comment 11 Joseph Pecoraro 2019-03-02 01:11:27 PST
Comment on attachment 358113 [details]
Patch

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

r=me, this seems to address the majority of flashing issues.

I do still see poor animations on frequently updating elements, perhaps based on the resolution of performance.now()?

Visible in just the seconds element of:

    <span id="time" title="?">
        <span id="minutes">?</span>:<span id="seconds">?</span>
    </span>
    <script>
    setInterval(function() {
        var date = new Date;
        minutes.textContent = minutes.title = date.getMinutes();
        seconds.textContent = seconds.title = date.getSeconds();
    }, 1000/60);
    </script>

90% of the time the animation works as expected, but sometimes it does not.

> Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:274
> +            element: null,

We could put `listener: null` as well.
Comment 12 Devin Rousso 2019-03-02 15:40:36 PST
Created attachment 363434 [details]
Patch
Comment 13 WebKit Commit Bot 2019-03-02 16:19:01 PST
Comment on attachment 363434 [details]
Patch

Clearing flags on attachment: 363434

Committed r242322: <https://trac.webkit.org/changeset/242322>
Comment 14 WebKit Commit Bot 2019-03-02 16:19:03 PST
All reviewed patches have been landed.  Closing bug.