Closed Bug 1365472 Opened 7 years ago Closed 7 years ago

stylo: SVG elements with SMIL-animated class attribute are probably not handled right

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Version:
53 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: bzbarsky, Assigned: birtles)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

Bug 1365472 - Use animated class names when doing selector matching in Servo;
59 bytes, text/x-review-board-request
heycam
: review+
Details 
Element::DoGetClasses uses GetAnimatedClassName on SVG elements.  I don't see an equivalent in ClassOrClassList over in ServoBindings.cpp.  I'm guessing the difference is not intentional...
Assignee: nobody → bbirtles
Priority: -- → P2
Status: NEW → ASSIGNED
Attached patch WIP patch (obsolete) — — Splinter Review 
This patch seems to work. However, it also seems to work without any of the changes to ServoElementSnapshot::AddAttrs. I guess I need to understand how snapshotting works.
I wrote some patches for this but they very occasionally hit the following assertion when running crashtest-e10s:

  Assertion failure: removedCount <= gUnusedAtomCount, at /home/worker/workspace/build/src/xpcom/ds/nsAtomTable.cpp:432

Nothing seems amiss with the patches themselves, so I wonder if the trouble is that we pass back the atom pointers from XXX_ClassOrClassList without add-refing them and keep using them beyond the point where the animated atom list on the element might be updated.

I notice we have a few other very similar bugs that fail this assertion:

  https://bugzilla.mozilla.org/buglist.cgi?quicksearch=removedCount%20%3C%3D%20gUnusedAtomCount&list_id=13641524

so perhaps there is already an existing issue here?
Just confirmed this is an existing issue since this no-op push also errored in the same way:

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=aacedc41d471a3805a12a62f01b191b99955f663&selectedJob=108425386

That said, these patches might increase the frequency of this failure.

Bug 1367374 appears to track this (bug 1368259 seems related but has less activity).
I'm going to put this up for review as-is for now. If it does, in fact, increase the frequency of bug 1367374 significantly then we can easily back it out while we investigate that. Otherwise, bug 1367374 might be something to dig into next week.
Attachment #8879012 - Attachment is obsolete: true
Comment on attachment 8879415 [details]
Bug 1365472 - Snapshot elements when their class name is animated;

https://reviewboard.mozilla.org/r/150306/#review155550

::: commit-message-f4590:6
(Diff revision 1)
> +heycam: Sorry this description is pretty weak. I don't really understand the
> +snapshotting setup. I take it it is for storing the state we need to do selector
> +matching but I don't really know why it's better to trigger it explicitly here.

We need to snapshot an element's attributes just before one is about to change, so that when it comes time to restyle, we have the old values of the attributes to compare against the current values.  Triggering explicitly here seems like the right thing to do, since the normal AttributeWillChange nsIDOMMutationEvent notification isn't used for SMIL attribute changes.

Although I wonder if it's better to model the "class list" as a separate bit of state on the snapshot, instead of just using the value in the snapshotted attributes for class="".  (Gecko, I guess, doesn't look at the SMIL animated class value for selectors like rect[class=x].  Having separate snapshot state for "class list" and the DOM class="" attribute value would let us continue doing that.  Although it's probably not super important do behave in exactly the same way here.)

::: dom/svg/nsSVGElement.cpp:140
(Diff revision 1)
> +    // For Servo, trigger a snapshot of the element.
> +    nsPresContext* presContext = shell->GetPresContext();
> +    if (presContext && presContext->RestyleManager()->IsServo()) {
> +      presContext->RestyleManager()
> +                 ->AsServo()
> +                 ->ClassAttributeChangedBySMIL(this, mClassAnimAttr);
> +    }

I think we need to do this before mClassAnimAttr is modified.  (I might have mistakenly said that afterwards is OK on IRC yesterday.)  We need the snapshot to pick up the value of the SMIL class attribute before it changes.

::: layout/base/ServoRestyleManager.h:72
(Diff revision 1)
> +  void ClassAttributeChangedBySMIL(dom::Element* aElement,
> +                                   const nsAttrValue* aNewValue);

Then I think we can rename this to ClassAttributeWillChangeFromSMIL or ClassAttributeWillBeChangedBySMIL or something.

::: layout/base/ServoRestyleManager.cpp:856
(Diff revision 1)
> +  AttributeWillChange(aElement, kNameSpaceID_None, nsGkAtoms::_class,
> +                      nsIDOMMutationEvent::MODIFICATION, aNewValue);

We don't need aNewValue to take the snapshot.  Could you factor out the work that AttributeWillChange does to a new method, maybe called TakeSnapshotForAttributeChange or similar, and have AttributeWillChange and ClassAttributeWillChangeFromSMIL call through to it?
Attachment #8879415 - Flags: review?(cam) → review+
(In reply to Cameron McCormack (:heycam) from comment #8)
> Although I wonder if it's better to model the "class list" as a separate bit
> of state on the snapshot, instead of just using the value in the snapshotted
> attributes for class="".  (Gecko, I guess, doesn't look at the SMIL animated
> class value for selectors like rect[class=x].  Having separate snapshot
> state for "class list" and the DOM class="" attribute value would let us
> continue doing that.  Although it's probably not super important do behave
> in exactly the same way here.)

Yes, you're right. In Gecko if we have:

  circle.green {
    fill: green;
  }
  rect[class="green"] {
    fill: green;
  }

and we animate the class attribute to be "green", only the first one will match from above.

Test here: https://jsfiddle.net/Ldxr7h8b/

However, it seems like with Stylo when we do attribute matching we don't call xxx_ClassOrClassList so we end up with the same result for the above test case (that is, with the patches in this bug applied).
In CSS, .foo means "match on class membership as defined by the document language", whereas [class="foo"] means "attribute named 'class' with value 'foo'".

How attribute selectors get matched is not really defined all that well by specs: CSS doesn't really say whether the matching is on the same things as getAttribute() returns, but I think that's somewhat implied when matching against a DOM (which CSS claims it's not doing).  The DOM spec doesn't say how CSS selector matching works.  The SVG spec itself does not define this.  HTML doesn't worry about it, because in HTML it's clear what "the value of an attribute" means.

In any case, specs could totally define that attribute selectors in CSS match on DOM getAttribute() (hence base values, not affected by animations... I think; in Chrome animations seem to affect what getAttribute() returns, but I think that's just a Chrome bug) while .foo matches on "belonging to a class" which can be affected by animations in SVG.  That would match browser behavior in Firefox, Safari and Chrome, modulo Chrome's getAttribute behavior not matching Firefox and Safari.  I did test in Edge, and it seems to not do this animation stuff at all?

Anyway, we should raise some spec issues around this stuff, and probably a bug on Chrome about animations affecting what getAttribute() returns, because afaict https://www.w3.org/TR/2001/REC-smil-animation-20010904/#BasicAnim clearly says:

    When an animation is running, it should not actually change the attribute values in the DOM.

and Chrome is violating that.
Actually, I've pretty much convinced myself that Chrome's getAttribute thing is a bug, and a bug specific to the "class" attribute at that.  Filed https://bugs.chromium.org/p/chromium/issues/detail?id=735820 on it.
(In reply to Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-) from comment #12)
> I did test in Edge, and it seems to not do this animation stuff at all?

That's right. Edge doesn't support SMIL.

(In reply to Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-) from comment #13)
> Actually, I've pretty much convinced myself that Chrome's getAttribute thing
> is a bug, and a bug specific to the "class" attribute at that.  Filed
> https://bugs.chromium.org/p/chromium/issues/detail?id=735820 on it.

Thank you!

I'll update the commit message from the first patch to reflect that.
I'm still hoping your or Cameron will file the relevant spec issues, by the way.  ;)
(In reply to Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-) from comment #15)
> I'm still hoping your or Cameron will file the relevant spec issues, by the
> way.  ;)

Will do.
So for the snapshot thing...

Seems like for matching .foo selectors against a snapshot we want snapshot of the old animated value, but for matching [class=foo] we want a snapshot of the old base value, right?  Otherwise seems to me like a selector that uses both .foo and [class=foo] at once might not get matched correctly against a snapshot.
Yeah, that makes sense. I'll see if I can make a test case for that.
(In reply to Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-) from comment #19)
> So for the snapshot thing...
> 
> Seems like for matching .foo selectors against a snapshot we want snapshot
> of the old animated value, but for matching [class=foo] we want a snapshot
> of the old base value, right?  Otherwise seems to me like a selector that
> uses both .foo and [class=foo] at once might not get matched correctly
> against a snapshot.

I attempted to make a test case that would fail because of this: https://jsfiddle.net/5u8y9zej/

But, with my local build with these patches applied, it passes and I don't really know why. I also notice that these patches produce the correct result even without the snapshot changes so perhaps it's simply the way SMIL is driving restyling.
I'm probably not going to get to the bottom of this today, so for my own notes I'm looking at the following test case:

<style>
circle.a {
  fill: red;
}
circle.a[class="b"] {
  fill: black;
}
</style>
<svg>
  <circle cx="60" cy="60" r="50" class="b">
    <set attributeName="class" to="a b" begin="1s" />
  </circle>
</svg>

The reasoning being that if we clobber the class attribute value in the snapshot with "a b" then the [class="b"] selector will not match and the circle will go red, even if only until the next restyle. It currently doesn't appear to do so, however, so I need to look into this further.
I spent a bit more time trying to create a failing test case for this but haven't succeeded in doing so yet.

Here's what I think I understand. There may be other ways we use snapshots, but I believe we are using them for style invalidation by comparing the previous state with the current state of an element. (Looking at invalidator.rs it looks like we actually check for selectors that will begin/stop matching based on those two states.)

In this case, the failure I am trying to produce comes from us adding an animated class name to the set of attributes such that it matches attribute selectors when it should not. So presumably we want to produce a case where the snapshot includes (or lacks) some class in its set of attributes (which incorrectly reflects animated state) but the updated element also includes (or lacks) that class in its set of attributes (which does not reflect animated state) such that when we compare the sets of attributes we incorrectly judge that no change has taken place.

I tried to produce this case with a combination of class selectors and attribute selectors but even if the attribute comparison returns no change, we end up detecting a change in the classes, or that's my guess, anyway.

Using only attribute selectors probably requires synchronizing DOM changes with SMIL changes which, I think, will also trigger style invalidation.

So at this stage I'm tempted to give up on writing a failing test case for this. However I will try making ServoElementSnapshot store a separate animated list of classes if only to make the code more intuitive (since both Cameron and Boris have raised concern about this).
Filed and issue on the SVG spec for clarifying this: https://github.com/w3c/svgwg/issues/328
Attachment #8879415 - Attachment is obsolete: true
Comment on attachment 8879414 [details]
Bug 1365472 - Use animated class names when doing selector matching in Servo;

https://reviewboard.mozilla.org/r/150304/#review159074

Some initial comments.  (And thanks for the detailed commit message.)

::: layout/base/ServoRestyleManager.h:178
(Diff revision 4)
>    }
>  
>    const SnapshotTable& Snapshots() const { return mSnapshots; }
>    void ClearSnapshots();
>    ServoElementSnapshot& SnapshotFor(mozilla::dom::Element* aElement);
> +  void SnapshotForAttributeChange(mozilla::dom::Element* aElement,

Nit: Can we have a slightly different name for this function?  Compared with "SnapshotFor", which is getting a snapshot, "SnapshotForAttributeChange" is about creating (or updating) a snapshot.  How about "TakeSnapshotForAttributeChange"?

::: layout/style/ServoElementSnapshot.cpp:70
(Diff revision 4)
>    if (aElement->MayHaveClass()) {
> +    const nsAttrValue* classValue = aElement->GetClasses();

aElement->GetClasses() will already do the MayHaveClass() check, so I think we can just remove the if statement.
Comment on attachment 8879414 [details]
Bug 1365472 - Use animated class names when doing selector matching in Servo;

https://reviewboard.mozilla.org/r/150304/#review159074

> aElement->GetClasses() will already do the MayHaveClass() check, so I think we can just remove the if statement.

Actually I think it should just be:

  if (const nsAttrValue* classValue = aElement->GetClasses()) {
    mClass = *classValue;
    mContains |= Flags::MaybeClass;
  }
Comment on attachment 8879414 [details]
Bug 1365472 - Use animated class names when doing selector matching in Servo;

https://reviewboard.mozilla.org/r/150304/#review159078
Attachment #8879414 - Flags: review?(cam) → review+
Pushed by bbirtles@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9d0c157be767
Use animated class names when doing selector matching in Servo; r=heycam
https://hg.mozilla.org/mozilla-central/rev/9d0c157be767
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: