Bug 148049 – Web Inspector: Unexpectedly frequent flashing of DOM node attributes

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Nikita Vasilyev]

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.

[Radar WebKit Bug Importer]

<rdar://problem/22296830>

[Nikita Vasilyev]

Created attachment 259068 [details]
[HTML] Reduction

[Devin Rousso]

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.

[Nikita Vasilyev]

Created attachment 259070 [details]
[Animated GIF] Expected behavior

Same in Chrome DevTools.

[Nikita Vasilyev]

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.

[Timothy Hatcher]

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…

[Timothy Hatcher]

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.

[Timothy Hatcher]

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.

[Devin Rousso]

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 🤔

[Devin Rousso]

Created attachment 358113 [details]
Patch

[Joseph Pecoraro]

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.

[Devin Rousso]

Created attachment 363434 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 363434 [details]
Patch

Clearing flags on attachment: 363434

Committed r242322: <https://trac.webkit.org/changeset/242322>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.