Picture-in-Picture test not waiting for event handler to run#58039
Picture-in-Picture test not waiting for event handler to run#58039theIDinside wants to merge 4 commits intoweb-platform-tests:masterfrom
Conversation
picture-in-picture/leave-picture-in-picture-disable-attribute.optional.html
Outdated
Show resolved
Hide resolved
picture-in-picture/leave-picture-in-picture-disable-attribute.optional.html
Outdated
Show resolved
Hide resolved
theIDinside
force-pushed
the
fix-pip-tests
branch
from
fff950f to
4900c59
Compare
|
See this documentation about |
Thanks! It seems like some tests are not passing for Chrome anymore: https://wpt.fyi/results/picture-in-picture?diff&filter=ADC&run_id=5144633593298944&run_id=5171632596582400 |
beaufortfrancois
left a comment
beaufortfrancois
left a comment
There was a problem hiding this comment.
Hopefully with the suggested change, tests should pass.
picture-in-picture/leave-picture-in-picture-disable-attribute.optional.html
Show resolved
Hide resolved
picture-in-picture/leave-picture-in-picture-disable-attribute.optional.html
Outdated
Show resolved
Hide resolved
picture-in-picture/leave-picture-in-picture-disable-attribute.optional.html
Outdated
Show resolved
Hide resolved
marcoscaceres
left a comment
marcoscaceres
left a comment
There was a problem hiding this comment.
Neat! Just a couple style nits to make the tests easier to reason about
|
I'm applying @marcoscaceres's feedback in https://chromium-review.googlesource.com/c/chromium/src/+/7664423 so that this PR remains only about splitting both tests. |
|
@theIDinside My chromium WPT changes have been upstreamed: #58559 You can now rebase your changes on top of it |
theIDinside
force-pushed
the
fix-pip-tests
branch
from
4900c59 to
143833a
Compare
|
@theIDinside I think you still need to update |
I'm sorry, I've lost the chain of thought on this one: what is it that I need to update here? |
|
Oh yeah, I think I see it. Re-remove the now re-added back in stuff due to rebase. |
Fix the tests that passed on browsers without ever hitting the asserts because we did not wait for them to run.
theIDinside
force-pushed
the
fix-pip-tests
branch
from
143833a to
4dc6203
Compare
picture-in-picture/leave-picture-in-picture-disable-attribute.optional.html
Outdated
Show resolved
Hide resolved
| assert_equals(document.pictureInPictureElement, null); | ||
| }, 'leavepictureinpicture event is fired if document.exitPictureInPicture'); | ||
|
|
||
| promise_test(async () => { |
There was a problem hiding this comment.
This test should be moved in the new file. That's all. No other changes should be needed. Am I missing something?
There was a problem hiding this comment.
Yeah that's in the optional one.
There was a problem hiding this comment.
The optional one doesn't have the same test as this one, which has been updated to address Marcos' feedback.
There was a problem hiding this comment.
Ah, got it. I'll fix it. Too much page faulting going on in 🧠 over the past few days 😛
Make optional test pull in changes made by beaufortfrancois in another review.
4 participants
See what the attribute says in the spec: https://w3c.github.io/picture-in-picture/#disable-pip:~:text=so%2E-,When,algorithm%2E and discussion w3c/picture-in-picture#184
This PR also addresses both the newly created one and the only one to actually wait for the event handler to execute so to not pass without running them as before.
This closes #58040