New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add WPT tests for webkit-prefixed animation/transition events #19099
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Already reviewed downstream.
c8c635f
to
845b889
Compare
e39681a
to
de0c541
Compare
de0c541
to
6fe296b
Compare
4a5e9a3
to
cae049e
Compare
@stephenmcgruer it might be good to also test content attributes here, because the behavior for those is weird with these events: the content attribute is all-lowercase (like the IDL attribute) but the event name is not all-lowercase (unlike most events). This showed up in some Gecko-internal tests when I was working on the casing issue in Firefox... |
eb3120a
to
dddfe02
Compare
dddfe02
to
d9fb178
Compare
@bzbarsky I agree! I've started looking into it, but it seems non-trivial to add support for so I would like to punt on it till a follow-up PR. I will file an issue to look into it. |
d9fb178
to
37220a9
Compare
@stephenmcgruer That sounds great, thank you! |
Bug: 999895 Change-Id: I5f3679561e923d0e24809ebe982f0672e7769392 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1807190 Commit-Queue: Stephen McGruer <smcgruer@chromium.org> Reviewed-by: Philip Jägenstedt <foolip@chromium.org> Cr-Commit-Position: refs/heads/master@{#730629}
37220a9
to
9e00835
Compare
@stephenmcgruer These tests seem to be timing out quite often in Firefox CI. See https://bugzilla.mozilla.org/show_bug.cgi?id=1608980, https://bugzilla.mozilla.org/show_bug.cgi?id=1609252, https://bugzilla.mozilla.org/show_bug.cgi?id=1609027, https://bugzilla.mozilla.org/show_bug.cgi?id=1609023, https://bugzilla.mozilla.org/show_bug.cgi?id=1609834. Any ideas offhand as to what might be going on there? |
@bzbarsky Ah, this is sort of good news to me. I saw some timeouts in FF during development, but they then went away when I wanted to report them in a bug... Is it only The problem is that increasing that animation length delays the tests by the iteration length (since they all wait for the iteration event). So its I could try upping it to, say, 75ms iteration time and see how that goes? |
Good catch; it looks like it's only We could try 75ms and see what happens, sure. I'll give that a shot on the Firefox side and see what our CI says. |
So, to be clear, that means the test should be changed to say |
Ah, that is a very useful clarification! ;) |
Generally we don't use animations that are less than 100s (seconds, not milliseconds) long in Firefox CI since we have had some really slow machines in CI. If we're waiting on iteration events, we would typically use iterations of 100s and then use a negative delay to start the animation just before the first iteration ends. |
OK, so I did a push with the existing 50ms number and another with 150ms. On the first one, 15 out of 40 test runs failed. On the second, 7 out of 40. So probably a lot better (or I got really lucky/unlucky), but still pretty bad... :( |
... I should have thought of that idea (since we've used it before as well -_-). Yeah, we should try that here; @bzbarsky are you ok to try that change or shall I? (Will require a little more change than just tweaking the animation length). |
I'd appreciate it if someone who knows animations better than I created the diff, but I'm happy to do some test runs. :) |
Ack, I'll try to find time to do it this week. |
Bug: 999895
Change-Id: I5f3679561e923d0e24809ebe982f0672e7769392
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1807190
Commit-Queue: Stephen McGruer <smcgruer@chromium.org>
Reviewed-by: Philip Jägenstedt <foolip@chromium.org>
Cr-Commit-Position: refs/heads/master@{#730629}