1418082 - stylo: element whose classList is changed is not traversed correctly

Status: RESOLVED INVALID

(Core :: CSS Parsing and Computation, defect, P3)

Description

[Hiroyuki Ikezoe (:hiro)]

+++ This bug was initially created as a clone of Bug #1417600 +++

You can see this issue when running browser/components/downloads/test/browser/browser_downloads_autohide.js with stylo-chrome and *without* the explicit flush which will be introduced in bug 1417600.

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

In that specific case of bug 1417600, it seems that the issue is that the code synchronously removes the class than add the class back, and per spec it seems it is allowed to not trigger animation in that case. It is not clear to me why it works in Gecko, though.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

As far as I can tell, the element in question is traversed once after removing 'animate-out' and before adding 'animation-out', so the animation should be started.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

What's going on there roughly based on my observation;

1) The servo data for the element is cleared
2) 'animation-out' class is removed
3) The element is traversed with UnstyledOnly
4) The element is traversed normally
5) 'animation-out' class is added
6) The element is tried to be traversed but collect_invalidations does no detect the classList changes

The servo data is also cleared somewhere 3) or 4).

What I don't still understand is what the trigger of clearing servo data is and what the trigger of UnstyledOnly is.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

(In reply to Xidorn Quan [:xidorn] UTC-8 (less responsive Nov 5 ~ Dec 16) from comment #1)
> In that specific case of bug 1417600, it seems that the issue is that the
> code synchronously removes the class than add the class back, and per spec
> it seems it is allowed to not trigger animation in that case. It is not
> clear to me why it works in Gecko, though.

Gecko synchronously runs style invalidation, so it'll schedule the restyle as soon as the attribute changes.

Servo will coalesce them using snapshots.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

This is somewhat related to toolbarpalette [1]

At 1) in comment 3 the element is moved into the toolbarpalette which seems to have 'display:none' style, then 'animate-out' is removed, at this moment we fails to take snapshot since the element has no servo data.  After this, I don't still understand what's going on.  Anyway, at 5) in comment 3, we succeeds to take snapshot but ServoElementSnapshot::AddAttrs() does not work as expected since the snapshot had already Flags::Attributes.

[1] https://hg.mozilla.org/mozilla-central/file/a3f183201f7f/browser/base/content/browser.xul#l1034
[2] https://hg.mozilla.org/mozilla-central/file/a3f183201f7f/toolkit/content/xul.css#l59

Comment 6

[Emilio Cobos Álvarez (:emilio)]

(In reply to Hiroyuki Ikezoe (:hiro) (PTO on 11/24) from comment #5)
> This is somewhat related to toolbarpalette [1]
> 
> At 1) in comment 3 the element is moved into the toolbarpalette which seems
> to have 'display:none' style, then 'animate-out' is removed, at this moment
> we fails to take snapshot since the element has no servo data.  After this,
> I don't still understand what's going on.  Anyway, at 5) in comment 3, we
> succeeds to take snapshot but ServoElementSnapshot::AddAttrs() does not work
> as expected since the snapshot had already Flags::Attributes.

That makes total sense. We only want to record the state before the _first_ attribute change. So given the other attribute change happens when the first attribute change hasn't been processed by the style system yet, it looks like the attribute change gets coalesced.

So I think this bug is just invalid.

Please reopen if you don't think the same, or if what I said doesn't make sense for any reason :P