1343919 - KeyframeUtils seems to be using the wrong base URI

Status: RESOLVED FIXED

(Core :: DOM: Animation, enhancement, P3)

Description

[Boris Zbarsky [:bzbarsky]]

It's using GetDocumentURI() for the base URI.  That seems wrong.  This code was added in bug 1245748.

Comment 1

[Brian Birtles (:birtles)]

Yeah, that seems wrong. I suspect I just copied the code added in bug 1208951 that does that same (except that it makes sense to use document URI in that bug since we're only parsing timing functions there).

Comment 2

[Brian Birtles (:birtles)]

Attachment: Test case

Test case demonstrating the problem.

On the upside, as far as I can tell, this only affects animations created using the API and only when using longhand properties and only for properties that take URIs (which we only recently only implemented).

Comment 3

[Brian Birtles (:birtles)]

I guess we'll need test cases for when we re-assign the target element to an element from another document and for when the base URI is updated (which we might not be able to fix in this bug since we're currently pretty bad at responding to changes in context).

Comment 4

[Brian Birtles (:birtles)]

In fact, in order to respond to dynamic changes, we might need to store these values as token streams and re-parse them before we produce computed values in UpdateProperties. So this might not be as trivial as just passing the document's base URI.

Then again, if you animate an element in another document using JS, which base URI do you expect to be used? The spec doesn't say (yet) but I wonder what authors would expect. I'm guessing it should use the base URI of the target element.

Comment 5

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1343919 - Fix base URI in KeyframeEffect. r=#style,#layout-reviewers

This is the only meaningful consumer of
ServoCSSParser::GetParsingEnvironment right now, but seems worth fixing
before other folks add more.

Comment 6

[Emilio Cobos Álvarez [:emilio]]

I didn't test the edge cases discussed above, but other browsers agree with our behavior in the tests I added, and our behavior in those is clearly wrong, so I think it's worth addressing.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/eb45c4f28c17
Fix base URI in KeyframeEffect. r=birtles

Comment 8

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/34311 for changes under testing/web-platform/tests

Comment 9

[Cristina Cozmuta (:CrissCozmuta)]

https://hg.mozilla.org/mozilla-central/rev/eb45c4f28c17

Comment 10

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot