| Summary: | Add a heuristic to identify and extract the prominent video element in element fullscreen | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Wenson Hsieh <wenson_hsieh> | ||||||||||
| Component: | Media | Assignee: | Wenson Hsieh <wenson_hsieh> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | akeerthi, calvaris, cdumez, changseok, cmarcelo, eric.carlson, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, jer.noble, kangil.han, katherine_cheney, megan_gardner, philipj, sergio, thorton, webkit-bug-importer | ||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||
| Version: | WebKit Nightly Build | ||||||||||||
| Hardware: | Unspecified | ||||||||||||
| OS: | Unspecified | ||||||||||||
| Bug Depends on: | |||||||||||||
| Bug Blocks: | 238607 | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Wenson Hsieh
2022-03-30 16:10:03 PDT
Created attachment 456187 [details]
Patch
Created attachment 456202 [details]
Patch
Created attachment 456235 [details]
Patch
Comment on attachment 456235 [details]
Patch
Nit: it appears this heuristic depends on the video element already existing in the page when fullscreen mode is entered. You may want consider a mutation observer, or listening for events bubbled up to the fullscreen element itself (like 'loadstart', 'play' or 'pause') to trigger recalculation of this heuristic.
Thanks for the review! (In reply to Jer Noble from comment #4) > Comment on attachment 456235 [details] > Patch > > Nit: it appears this heuristic depends on the video element already existing > in the page when fullscreen mode is entered. You may want consider a > mutation observer, or listening for events bubbled up to the fullscreen > element itself (like 'loadstart', 'play' or 'pause') to trigger > recalculation of this heuristic. Indeed! That is an omission from this iteration of the heuristic (as well as the case where m_mainVideoElement is unparented while in element fullscreen mode, and replaced with a new video element). I was initially planning to address this in a future patch by overriding HTMLMediaElement's tree insertion/removal method hooks (as well as other media-related methods), but I think the approach of listening for media-related events bubbled up to the fullscreen element is a lot more elegant — I'll go with that approach, and add a FIXME as well. Created attachment 456284 [details]
Patch for landing
Committed r292205 (249108@main): <https://commits.webkit.org/249108@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 456284 [details]. |