Closed Bug 1362400 Opened 2 years ago Closed 2 years ago

cannot animate a link into existence using SMIL

Categories

(Core :: SVG, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: longsonr, Assigned: longsonr)

Details

Attachments

(3 files, 4 obsolete files)

Attached file testcase
Assignee: nobody → longsonr
Attachment #8864858 - Attachment description: link.html → testcase
Attached patch 1362400.txt (obsolete) — Splinter Review
No idea how to do tests for this.
Attachment #8864859 - Flags: review?(jwatt)
Attached patch don't need the friend bit (obsolete) — Splinter Review
Attachment #8864859 - Attachment is obsolete: true
Attachment #8864859 - Flags: review?(jwatt)
Attachment #8864863 - Flags: review?(jwatt)
Attached patch reftest (obsolete) — Splinter Review
Attachment #8864868 - Flags: review?(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.
And do you happen to know if Chrome is still pushing to deprecate SMIL? If so maybe we shouldn't add this support?
(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.
(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.
(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.
Attached patch update per review comments (obsolete) — Splinter Review
Attachment #8864863 - Attachment is obsolete: true
Attachment #8864868 - Attachment is obsolete: true
Attachment #8864863 - Flags: review?(jwatt)
Attachment #8864868 - Flags: review?(jwatt)
Attachment #8864914 - Flags: review?(jwatt)
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.
Attachment #8864914 - Flags: review?(bugs)
(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.
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.
Or is Chrome doing that wrong?

(I'm not familiar with <animate>)
Flags: needinfo?(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.
Flags: needinfo?(longsonr)
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.
Attachment #8864914 - Flags: review?(bugs) → review+
(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.
(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?
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.
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.
does this still need more comments or is it obvious enough now we have a SMIL modification type?
Attachment #8864914 - Attachment is obsolete: true
Attachment #8864914 - Flags: review?(jwatt)
Attachment #8865077 - Flags: review?(jwatt)
Attachment #8865077 - Attachment description: Distinguish SMIL changes → Part 1 - Distinguish SMIL changes
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).
Attachment #8865077 - Flags: review?(jwatt) → review+
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.
Attachment #8865078 - Flags: review?(jwatt) → review+
Pushed by longsonr@gmail.com:
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)
Keywords: leave-open
Pushed by longsonr@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7dfb3eba4c4
part 2 - optimise SMIL image updates r=jwatt
Keywords: leave-open
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/005d5107cd83
https://hg.mozilla.org/mozilla-central/rev/b7dfb3eba4c4
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.