Update SVGAElement to match attributes on HTMLAnchorElement #409
Conversation
I removed [SameObject] as it isn't usedo n HTML. I didn't include CEReactions as that is for Custom Elements and should be ignored for non-HTML namespace.
And fixed other attributes to not be readonly and not animatable (presumably strings can't be animated)
|
Tests for some of these: web-platform-tests/wpt#10275 |
master/definitions.xml
Outdated
| <attribute name='rel' href='linking.html#AElementRelAttribute' animatable='yes'/> | ||
| <attribute name='hreflang' href='linking.html#AElementHreflangAttribute' animatable='yes'/> | ||
| <attribute name='type' href='linking.html#AElementTypeAttribute' animatable='yes'/> | ||
| <attribute name='target' href='linking.html#AElementTargetAttribute' animatable='no'/> |
There was a problem hiding this comment.
target is animatable in WebKit as far as I can see. This was implemented before the Blink split so I am positive that it is in Blink as well. Could you add the link to the WG resolution if this was resolved on please?
There was a problem hiding this comment.
target is an SVGAnimatedString in SVG 1.1 and in all browsers including Edge.
The following example works in all browsers, using target.baseVal to read the target.
https://jsfiddle.net/ericwilligers/bL9zvd1w/
There was a problem hiding this comment.
Yes, I think the animatable values should be reversed: target is animatable for legacy reasons, the others needn't be.
Similarly, we've agreed to make all new attributes DOMString, but the legacy ones were to stay SVGAnimatedString, and then SVGAnimatedString was going to be given stringifier & setters to make it behave more like a regular string (see #175). Not sure if we can get implementations to do that second part in SVG 2, though.
There was a problem hiding this comment.
we agreed to change target to DOMString as the usage is so tiny.
There was a problem hiding this comment.
Here's the resolution for that decision:
#315 (comment)
# Conflicts: # master/linking.html
With extra note about the breaking change for `target`.
(These should have been added in the merge in 86aaeef )
|
The Working Group just discussed The full IRC log of that discussion<AmeliaBR> Topic: Breaking change re a/target IDL representation<AmeliaBR> GitHub: https://github.com//pull/409 <AmeliaBR> Amelia: I was tidying up the PR for merging, but we did have comments concerned about the breaking change. <AmeliaBR> ... We had previously resolved on that change, wanted to confirm before merging. <AmeliaBR> Dirk: Do we have usage statistics? <AmeliaBR> https://github.com//issues/315#issuecomment-374485871 <AmeliaBR> Yes, posted in the issue discussion ^^ <AmeliaBR> Dirk: So usage is 0.002058% <krit> V8SVGAElement_Target_AttributeGetter 0.002058% <AmeliaBR> Eric: That's tiny. We'd be happy to match other browsers. Also no desire to preserve the SMIL use case. <AmeliaBR> Amelia: We did discuss (in #312) also enhancing SVGAnimatedString with putForwards, so it can behave a little more like a DOMString, for cases like href where we can't change the actual IDL type. <AmeliaBR> Dirk: One issue is can we delay this change to 2.1? <AmeliaBR> Amelia: The longer its out there as SVGAnimatedString, the more potential for web compatibility issues. <AmeliaBR> Dirk: But there already is the compat risk. We have compatible implementations in all browsers. <ericwilligers> My comment: https://github.com//pull/409#discussion_r200943051 <AmeliaBR> Amelia: The new attributes may also need to be removed if we don't get implementations. But that's less of an issue, if they're not breaking. <AmeliaBR> Dirk: I'm open to the change, I'm just worried we won't get implementations. <AmeliaBR> Amelia: Can we make the change, but mark it at-risk pending implementations? But I guess we still need to tell implementers what they should do. <AmeliaBR> Eric: What's the risk of no change? <AmeliaBR> Amelia: Risk is increased incompatibility versus HTML. <AmeliaBR> ... I'd recommend making the change, but we need to follow up with browser bugs to make them update to match. <AmeliaBR> Eric: We really need some commitments. <AmeliaBR> ... We wouldn't make the change in Blink until there is movement from other browsers. <AmeliaBR> Amelia: I can revert the changes for `target` specifically in the PR, merge the other changes, and then open up an issue specifically for `target`. <AmeliaBR> ... We need commitments from multiple implementers to all make the change at the same time if it is going to happen at all. |
- DOMString and USVString aren't auto-linkable - Fix copy-pasta duplicate ID error
Keep it specified as an animatable attribute & an SVGAnimatedString DOM property, as implemented in all major browsers, unless/until we can get agreement to change implementations. PS, If that agreement happens, in addition to reverting this commit, we also need to change the "yes" animatable in the table in linking.html.
|
Okay, I'm merging in the changes to everything except This means the spec matches the Firefox implementation. And hopefully means that other browsers can move ahead with matching their implementations! We'll continue discussion of the potential breaking change re |
This is temporary until the HTML spec includes the HTMLOrSVGAnchorElement mixin (or equivalent), when we can just refer to that spec. I just copied the reflect rules for
textfrom HTML for example.Apologies if I didn't add things in all the right places or got the references wrong. I'm still learning how things should be structured and edited.