Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2618323002:
Block animation of the SVGScriptElement (Closed)
|
Created:
3 years, 11 months ago by fs Modified:
3 years, 11 months ago CC:
darktears, blink-reviews, blink-reviews-animation_chromium.org, chromium-reviews, krit, Eric Willigers, f(malita), gyuyoung2, kouhei+svg_chromium.org, pdr+svgwatchlist_chromium.org, rjwright, rwlbuis, Stephen Chennney, shans Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionBlock animation of the SVGScriptElement
'href' should not be animatable for SVGScriptElements, but currently is.
This will "break" animation of 'className' on the same element.
R=mkwst@chromium.org,pdr@chromium.org
BUG=679291
Review-Url: https://codereview.chromium.org/2618323002
Cr-Commit-Position: refs/heads/master@{#442915}
Committed: https://chromium.googlesource.com/chromium/src/+/97501194ee5fac524c6126c7e7bd2184d9fcfca6
Patch Set 1 #
Total comments: 7
Patch Set 2 : Blacklist instead #Patch Set 3 : Missing include #
Messages
Total messages: 27 (18 generated)
The CQ bit was checked by fs@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
LGTM https://codereview.chromium.org/2618323002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/svg/SVGScriptElement.cpp (right): https://codereview.chromium.org/2618323002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/svg/SVGScriptElement.cpp:85: // TODO(fs): Recover head rotation. O_o
LGTM % some additional tests I'd like to see and some tiny nits. https://codereview.chromium.org/2618323002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/svg/animations/script-href-no-animate.html (right): https://codereview.chromium.org/2618323002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/svg/animations/script-href-no-animate.html:15: requestAnimationFrame(t.step_func_done(_ => { Why two frames? I'll defer to y'all with regard to SVG timing, but I'm curious. :) https://codereview.chromium.org/2618323002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/svg/animations/script-href-no-animate.html:24: <set href="#x" attributeName="href" to="resources/set-foo.js"/> Would you mind adding a similar test for `<animate>` (are there others?) and for `xlink:href`? https://codereview.chromium.org/2618323002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/svg/SVGScriptElement.cpp (right): https://codereview.chromium.org/2618323002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/svg/SVGScriptElement.cpp:145: if (name == SVGNames::typeAttr || name == SVGNames::hrefAttr) XLinkNames::hrefAttr?
Looks like this breaks the synch-single-attr case, so I'll need to come up with a slightly different approach to combat that... Will ping when ready. https://codereview.chromium.org/2618323002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/svg/animations/script-href-no-animate.html (right): https://codereview.chromium.org/2618323002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/svg/animations/script-href-no-animate.html:15: requestAnimationFrame(t.step_func_done(_ => { On 2017/01/09 at 19:31:19, Mike West (sloooooow) wrote: > Why two frames? I'll defer to y'all with regard to SVG timing, but I'm curious. :) This is a bit of a frailty with the test I'm afraid - it's the "allow X time wherein nothing happens". We need the first to know the animation started (although "after 'load'" works for that too), but that's not enough of a wait since we may need to wait for the script to load and execute. I guess this makes this test a potential "flaky failure (with a high probability)"-type test. https://codereview.chromium.org/2618323002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/svg/animations/script-href-no-animate.html:24: <set href="#x" attributeName="href" to="resources/set-foo.js"/> On 2017/01/09 at 19:31:19, Mike West (sloooooow) wrote: > Would you mind adding a similar test for `<animate>` (are there others?) and for `xlink:href`? I can do that. https://codereview.chromium.org/2618323002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/svg/SVGScriptElement.cpp (right): https://codereview.chromium.org/2618323002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/svg/SVGScriptElement.cpp:85: // TODO(fs): Recover head rotation. On 2017/01/09 at 18:50:47, pdr. wrote: > O_o Yes, this function is obviously broken (or, when is the name of the attribute going to match the _value_ of the attribute...) I'll fix it up separately.
The CQ bit was checked by fs@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by fs@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/01/10 at 10:15:16, fs wrote: > Looks like this breaks the synch-single-attr case, so I'll need to come up with a slightly different approach to combat that... Will ping when ready. Ok, re-done differently. I think a SVGStaticHref solution might be nicer, but I did not want to spend that amount of time right now.
Description was changed from ========== Don't add 'href' to the SVGElement property map for SVGScriptElement 'href' should not be animatable for SVGScriptElements, to remove it from the property map, and handle things "manually" instead. R=mkwst@chromium.org,pdr@chromium.org BUG=679291 ========== to ========== Block animation of the SVGScriptElement 'href' should not be animatable for SVGScriptElements, but currently is. This will "break" animation of 'className' on the same element. R=mkwst@chromium.org,pdr@chromium.org BUG=679291 ==========
On 2017/01/10 14:56:02, fs wrote: > On 2017/01/10 at 10:15:16, fs wrote: > > Looks like this breaks the synch-single-attr case, so I'll need to come up > with a slightly different approach to combat that... Will ping when ready. > > Ok, re-done differently. I think a SVGStaticHref solution might be nicer, but I > did not want to spend that amount of time right now. LGTM. Doing check in resolveTargetProperty makes sense to me.
mkwst@chromium.org changed reviewers: + kouhei@chromium.org
Test changes LGTM. Thanks!
The CQ bit was checked by fs@opera.com
The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org Link to the patchset: https://codereview.chromium.org/2618323002/#ps40001 (title: "Missing include")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1484145922141430,
"parent_rev": "e5cfe692b132aa62616a5db9f56e23621c4b9c9e", "commit_rev":
"97501194ee5fac524c6126c7e7bd2184d9fcfca6"}
Message was sent while issue was closed.
Description was changed from ========== Block animation of the SVGScriptElement 'href' should not be animatable for SVGScriptElements, but currently is. This will "break" animation of 'className' on the same element. R=mkwst@chromium.org,pdr@chromium.org BUG=679291 ========== to ========== Block animation of the SVGScriptElement 'href' should not be animatable for SVGScriptElements, but currently is. This will "break" animation of 'className' on the same element. R=mkwst@chromium.org,pdr@chromium.org BUG=679291 Review-Url: https://codereview.chromium.org/2618323002 Cr-Commit-Position: refs/heads/master@{#442915} Committed: https://chromium.googlesource.com/chromium/src/+/97501194ee5fac524c6126c7e7bd... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/97501194ee5fac524c6126c7e7bd... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
