1362400 - cannot animate a link into existence using SMIL

Status: RESOLVED FIXED

(Core :: SVG, enhancement)

Description

[Robert Longson [:longsonr]]

Apparently this works on Chrome per http://stackoverflow.com/questions/43500417/svgs-animate-href-not-working-in-firefox

Comment 1

[Robert Longson [:longsonr]]

Attachment: testcase

Comment 2

[Robert Longson [:longsonr]]

Attachment: 1362400.txt

No idea how to do tests for this.

Comment 3

[Robert Longson [:longsonr]]

Attachment: don't need the friend bit

Comment 4

[Robert Longson [:longsonr]]

Attachment: reftest

Comment 5

[Jonathan Watt [:jwatt]]

Comment on attachment 8864863 [details] [diff] [review]
don't need the friend bit

Review of attachment 8864863 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/Link.h
@@ +115,4 @@
>    virtual size_t
>      SizeOfExcludingThis(mozilla::MallocSizeOf aMallocSizeOf) const;
>  
> +  virtual bool ElementHasHref() const;

I vaguely recall from my limited interaction with this code that this may be performance sensitive (possibly called for every single element?). At any rate, you'll need a DOM peer's review to land changes to this file.

::: dom/svg/SVGAElement.cpp
@@ +86,5 @@
> +bool
> +SVGAElement::ElementHasHref() const
> +{
> +  return mStringAttributes[HREF].IsExplicitlySet() ||
> +         mStringAttributes[XLINK_HREF].IsExplicitlySet();

Given 'xlink:href' is deprecated, I think you should drop the XLINK_HREF check so as not to encourage further use.

@@ +291,2 @@
>  
> +  if ((!useXLink || mStringAttributes[XLINK_HREF].IsExplicitlySet()) &&

Might read slightly better if called 'useBareHref'? Change or leave as you prefer.

@@ +379,4 @@
>    if (aAttr == nsGkAtoms::href &&
>        (aNameSpaceID == kNameSpaceID_XLink ||
>         aNameSpaceID == kNameSpaceID_None)) {
> +    Link::ResetLinkState(!!aNotify, Link::ElementHasHref());

I'm guessing we don't need this if we handle the change in nsSVGAFrame::AttributeChanged.

::: layout/svg/nsSVGAFrame.cpp
@@ +94,5 @@
>      NotifySVGChanged(TRANSFORM_CHANGED);
>    }
> +  if ((aNameSpaceID == kNameSpaceID_None ||
> +       aNameSpaceID == kNameSpaceID_XLink) &&
> +      aAttribute == nsGkAtoms::href) {

Virtually all checks for the namespace will resolve to true, so it would be slightly faster to do the atom pointer check first.

@@ +98,5 @@
> +      aAttribute == nsGkAtoms::href) {
> +
> +    dom::SVGAElement *content = static_cast<dom::SVGAElement*>(mContent);
> +
> +    content->ResetLinkState(true, content->ElementHasHref());

I'm not sure when we pass normally pass false as the value for aNotify, but hardcoding the value to true looks wrong. At the very least this would require a comment explaining why that's okay.

Comment 6

[Jonathan Watt [:jwatt]]

And do you happen to know if Chrome is still pushing to deprecate SMIL? If so maybe we shouldn't add this support?

Comment 7

[Robert Longson [:longsonr]]

(In reply to Jonathan Watt [:jwatt] from comment #5)

> > +  virtual bool ElementHasHref() const;
> 
> I vaguely recall from my limited interaction with this code that this may be
> performance sensitive (possibly called for every single element?). At any
> rate, you'll need a DOM peer's review to land changes to this file.

OK

> 
> ::: dom/svg/SVGAElement.cpp
> @@ +86,5 @@
> > +bool
> > +SVGAElement::ElementHasHref() const
> > +{
> > +  return mStringAttributes[HREF].IsExplicitlySet() ||
> > +         mStringAttributes[XLINK_HREF].IsExplicitlySet();
> 
> Given 'xlink:href' is deprecated, I think you should drop the XLINK_HREF
> check so as not to encourage further use.

I think that should be a separate bug.

> 
> @@ +291,2 @@
> >  
> > +  if ((!useXLink || mStringAttributes[XLINK_HREF].IsExplicitlySet()) &&
> 
> Might read slightly better if called 'useBareHref'? Change or leave as you
> prefer.

happy to change that.

> 
> @@ +379,4 @@
> >    if (aAttr == nsGkAtoms::href &&
> >        (aNameSpaceID == kNameSpaceID_XLink ||
> >         aNameSpaceID == kNameSpaceID_None)) {
> > +    Link::ResetLinkState(!!aNotify, Link::ElementHasHref());
> 
> I'm guessing we don't need this if we handle the change in
> nsSVGAFrame::AttributeChanged.

will try that.

> 
> ::: layout/svg/nsSVGAFrame.cpp
> @@ +94,5 @@
> >      NotifySVGChanged(TRANSFORM_CHANGED);
> >    }
> > +  if ((aNameSpaceID == kNameSpaceID_None ||
> > +       aNameSpaceID == kNameSpaceID_XLink) &&
> > +      aAttribute == nsGkAtoms::href) {
> 
> Virtually all checks for the namespace will resolve to true, so it would be
> slightly faster to do the atom pointer check first.
> 

OK

> 
> I'm not sure when we pass normally pass false as the value for aNotify, but
> hardcoding the value to true looks wrong. At the very least this would
> require a comment explaining why that's okay.

I had it false to begin with, however if you mouseover the testcase, the cursor does not change and it should. Will add a comment.

Comment 8

[Robert Longson [:longsonr]]

(In reply to Jonathan Watt [:jwatt] from comment #6)
> And do you happen to know if Chrome is still pushing to deprecate SMIL? If
> so maybe we shouldn't add this support?

As far as I know they've given up on deprecation after receiving lots of pushback when they announced it.

Comment 9

[Robert Longson [:longsonr]]

(In reply to Robert Longson from comment #7)

> 
> > 
> > @@ +379,4 @@
> > >    if (aAttr == nsGkAtoms::href &&
> > >        (aNameSpaceID == kNameSpaceID_XLink ||
> > >         aNameSpaceID == kNameSpaceID_None)) {
> > > +    Link::ResetLinkState(!!aNotify, Link::ElementHasHref());
> > 
> > I'm guessing we don't need this if we handle the change in
> > nsSVGAFrame::AttributeChanged.
> 

I'm going to limit the Frame code to modify (that's all SMIL uses) so that we don't call ResetLinkState too often. I'll need to keep this code, especially as it doesn't always notify and the frame code does.

Comment 10

[Robert Longson [:longsonr]]

Attachment: update per review comments

Comment 11

[Robert Longson [:longsonr]]

Comment on attachment 8864914 [details] [diff] [review]
update per review comments

jwatt suggested that the change to virtual of a Link.h method should have a DOM peer review.

Comment 12

[Robert Longson [:longsonr]]

(In reply to Jonathan Watt [:jwatt] from comment #5)

> 
> Given 'xlink:href' is deprecated, I think you should drop the XLINK_HREF
> check so as not to encourage further use.

Unfortunately Safari does not yet support bare href so it's probably too early discourage further use till it does so.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Could you explain this a bit? Why we don't just set href attribute, and then the existing Link::ElementHasHref() would work.

Chrome certainly sets the actual DOM attribute.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Or is Chrome doing that wrong?

(I'm not familiar with <animate>)

Comment 15

[Robert Longson [:longsonr]]

SVG 1.1 has SMIL animation separate from the DOM.
SVG 2 unifies SMIL animation and the DOM so animation changes the DOM.

Firefox and Safari's SMIL animation is SVG 1.1, Chrome's is SVG 2. IE/Edge doesn't support it at all.

Unifying SMIL animation is a big task. For instance how to we revert to the original non-animated value once animation finishes?

This gives us a little more compatibility with Chrome but does not tackle SMIL animation unification which would be a pretty large undertaking.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8864914 [details] [diff] [review]
update per review comments

ok, thanks for clarifying.

I think making that method virtual shouldn't be too bad perf regression.

jwatt should review the actual implementation.

Comment 17

[Jonathan Watt [:jwatt]]

(In reply to Robert Longson from comment #7)
> (In reply to Jonathan Watt [:jwatt] from comment #5)
> > I'm not sure when we pass normally pass false as the value for aNotify, but
> > hardcoding the value to true looks wrong. At the very least this would
> > require a comment explaining why that's okay.
> 
> I had it false to begin with, however if you mouseover the testcase, the
> cursor does not change and it should. Will add a comment.

What I meant is that sometimes when AttributeChanged is called we will need to pass true, but other times maybe we may want to pass false (although probably only for perf reasons if at all). Figuring out when we might want to pass false might require a fairly detailed analysis of the code though. Probably the question boils down to "when is 'false' passed to SetAttr, and is AttributeChanged called in any of those cases?" Probably it's not that big a deal so I guess we could make do with tacking an XXX comment noting this issue onto the end of the comment you already added.

Comment 18

[Jonathan Watt [:jwatt]]

(In reply to Robert Longson from comment #9)
> I'm going to limit the Frame code to modify (that's all SMIL uses) so that
> we don't call ResetLinkState too often. I'll need to keep this code,
> especially as it doesn't always notify and the frame code does.

That looks weird. nsIDOMMutationEvent::MODIFICATION is exactly when I'd expect us *not* to require a ResetLinkState call since that's the case when we shouldn't be flipping between is-a-link and is-not-a-link. Can you explain this a bit more?

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Hmm, I didn't look at the rest of the patch. It is highly unlikely you want to pass false as notify.
If we don't notify about the attribute changes, MutationRecords won't be created, so MutationObserver doesn't work correctly.

Comment 20

[Robert Longson [:longsonr]]

We only want to call ResetLinkState if the caller is SMIL as otherwise ResetLinkState will be handled in the SVGAElement and the SVGAElement code knows when to notify and pretty much whether we're creating or deleting an attribute.

SMIL animation always calls AttributeChanged with nsIDOMMutationEvent::MODIFICATION

https://dxr.mozilla.org/mozilla-central/source/dom/svg/nsSVGElement.cpp#2422

At that point there's no way to tell whether we're updating or creating an attribute.

If we have an <a> element with no href and we animate it to have a href (as in the testcase). Then we need to call ResetLinkState. The only place to process that for <a> element's is in the SVGAFrame's AttributeChanged.

I suppose we could change the SMIL code to send a new nsIDOMMutationEvent::SMIL_MODIFICATION and fix up any affected AttributeChanged SVG frames.

Comment 21

[Robert Longson [:longsonr]]

Attachment: Part 1 - Distinguish SMIL changes

does this still need more comments or is it obvious enough now we have a SMIL modification type?

Comment 22

[Robert Longson [:longsonr]]

Attachment: Part 2 - additional optimisations that are possible with SMIL notifications

Comment 23

[Jonathan Watt [:jwatt]]

Comment on attachment 8865077 [details] [diff] [review]
Part 1 - Distinguish SMIL changes

Review of attachment 8865077 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

(Interesting that none of the AttributeChanged overrides or the overridden functions that they call currently check aModType.)

::: layout/svg/nsSVGAFrame.cpp
@@ +94,4 @@
>      // and cause DoApplyRenderingChangeToTree to make the SchedulePaint call.
>      NotifySVGChanged(TRANSFORM_CHANGED);
>    }
> +  if (aModType == nsIDOMMutationEvent::SMIL &&

Please add a comment before this line something like:

  // Currently our SMIL implementation does not modify the DOM attributes. Once
  // we implement the SVG 2 SMIL behaviour this can be removed
  // (SVGAElement::SetAttr/UnsetAttr's ResetLinkState() call will be sufficient).

@@ +98,5 @@
> +      aAttribute == nsGkAtoms::href &&
> +      (aNameSpaceID == kNameSpaceID_None ||
> +       aNameSpaceID == kNameSpaceID_XLink)) {
> +
> +    dom::SVGAElement *content = static_cast<dom::SVGAElement*>(mContent);

Nit: left align the *

@@ +100,5 @@
> +       aNameSpaceID == kNameSpaceID_XLink)) {
> +
> +    dom::SVGAElement *content = static_cast<dom::SVGAElement*>(mContent);
> +
> +    // SMIL may cause an <a> element to become a link, if so we will

It may also cause it to stop being a link, so I think "SMIL may change whether or not an element is a link" would be better. I'd also s/if so/in which case/

I think the first sentence is enough (i.e. you could delete the second).

Comment 24

[Jonathan Watt [:jwatt]]

Comment on attachment 8865078 [details] [diff] [review]
Part 2 - additional optimisations that are possible with SMIL notifications

Review of attachment 8865078 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/svg/SVGFEImageFrame.cpp
@@ +127,4 @@
>                 "Observers observe the filter, so that's what we must invalidate");
>      nsSVGEffects::InvalidateDirectRenderingObservers(GetParent());
>    }
> +  if (aModType == nsIDOMMutationEvent::SMIL &&

Similar "SVG 2" comment here.

Nit: You could also move the nsGkAtoms::href check before the namespace check while you're here. :)

::: layout/svg/nsSVGImageFrame.cpp
@@ +227,4 @@
>        return NS_OK;
>      }
>    }
> +  if (aModType == nsIDOMMutationEvent::SMIL &&

And here.

Comment 25

[Robert Longson [:longsonr]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b67cefe31240ca23acdaa2974285a65c366acbc7

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/005d5107cd83
part 1 - Make it possible for SMIL to change the link state of an SVG anchor element r=jwatt r=smaug (DOM)

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7dfb3eba4c4
part 2 - optimise SMIL image updates r=jwatt

Comment 28

[Iris Hsiao [:ihsiao]]

https://hg.mozilla.org/mozilla-central/rev/005d5107cd83
https://hg.mozilla.org/mozilla-central/rev/b7dfb3eba4c4