Summary: | REGRESSION: [iOS] 6 media/modern-media-controls/tracks-support/ tests timing out | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Aditya Keerthi <akeerthi> | ||||||
Component: | Media | Assignee: | Aditya Keerthi <akeerthi> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | eric.carlson, ews-watchlist, glenn, hi, jer.noble, philipj, sergio, thorton, webkit-bug-importer, wenson_hsieh | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | Other | ||||||||
Hardware: | iPhone / iPad | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Aditya Keerthi
2021-04-29 12:12:17 PDT
Created attachment 427367 [details]
Patch
Comment on attachment 427367 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=427367&action=review r=me > LayoutTests/ChangeLog:14 > + of the <video> element was increased. However, on iOS, this led to the > + tracks button being positioned outside the viewport. Consequently, any LOL!!! I did not think of this at all! Great catch! > LayoutTests/ChangeLog:30 > + which lead to getTracksContextMenu() getting called before the tap > + actually completed. UIScriptController clears all callbacks after one Oh wow that is a *subtle* detail of UIScriptController :( Why was it made like this? I would think that one kind of callback wouldn't affect any other kind of callback? Personally I prefer the approach of establishing listeners/callbacks for actions before the action occurs so that there's no chance of it being missed (I also think it reads nicer). > LayoutTests/media/modern-media-controls/resources/media-controls-utils.js:49 > +function pressOnElementAsync(element) NIT: I feel like "Async" isn't the right name for this since `pressOnElement` also takes a `continuation` callback. Maybe `promisifiedPressOnElement`, or even just having `pressOnElement` return a `Promise` if it's not provided a `continuation`? ``` let promise = null; if (typeof continuation !== "function") { promise = new Promise((resolve, reject) => { continuation = resolve; }); } ... return promise || true; ``` Created attachment 427370 [details]
Patch for landing
(In reply to Devin Rousso from comment #3) > Comment on attachment 427367 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=427367&action=review > > r=me > > > LayoutTests/ChangeLog:14 > > + of the <video> element was increased. However, on iOS, this led to the > > + tracks button being positioned outside the viewport. Consequently, any > > LOL!!! I did not think of this at all! Great catch! > > > LayoutTests/ChangeLog:30 > > + which lead to getTracksContextMenu() getting called before the tap > > + actually completed. UIScriptController clears all callbacks after one > > Oh wow that is a *subtle* detail of UIScriptController :( > > Why was it made like this? I would think that one kind of callback wouldn't > affect any other kind of callback? Personally I prefer the approach of > establishing listeners/callbacks for actions before the action occurs so > that there's no chance of it being missed (I also think it reads nicer). The logic to clear callbacks on `uiScriptComplete` was added in https://trac.webkit.org/changeset/192039/webkit, to prevent callbacks from sticking around in later tests. > > LayoutTests/media/modern-media-controls/resources/media-controls-utils.js:49 > > +function pressOnElementAsync(element) > > NIT: I feel like "Async" isn't the right name for this since > `pressOnElement` also takes a `continuation` callback. Maybe > `promisifiedPressOnElement`, or even just having `pressOnElement` return a > `Promise` if it's not provided a `continuation`? > ``` > let promise = null; > if (typeof continuation !== "function") { > promise = new Promise((resolve, reject) => { > continuation = resolve; > }); > } > ... > return promise || true; > ``` Thanks for code snippet! I've made `pressOnElement` return a `Promise` if it's not provided a `continuation`. Committed r276822 (237177@main): <https://commits.webkit.org/237177@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 427370 [details]. |