Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2757323002:
Prevent Media Controls from hiding when the user is interacting via the keyboard (Closed)
|
Created:
3 years, 9 months ago by steimel Modified:
3 years, 8 months ago CC:
blink-reviews, blink-reviews-html_chromium.org, chromium-reviews, dglazkov+blink, eric.carlson_apple.com, feature-media-reviews_chromium.org, fs, gasubic, mlamouri+watch-blink_chromium.org, nessy, Srirama Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionPrevent Media Controls from hiding when the user is interacting via the keyboard
This CL prevents the controls from hiding by resetting the hide timer on events in two places:
1) Listening for focusin and input events on the MediaControls via defaultEventHandler, which get fired as the user tabs through the controls and as the user changes the timeline/volume sliders respectively.
2) Listening for keypress events on the media panel via adding an event listener to the panel element in MediaControlsMediaEventListener, which gets fired when the user mutes/unmutes, turns CC on/off, etc.
BUG=701440
Review-Url: https://codereview.chromium.org/2757323002
Cr-Commit-Position: refs/heads/master@{#462990}
Committed: https://chromium.googlesource.com/chromium/src/+/12df228997d26f76483eb438602b22897c24d094
Patch Set 1 #
Total comments: 6
Patch Set 2 : johnme@ feedback and add unit tests #
Total comments: 7
Patch Set 3 : johnme@ feedback #Patch Set 4 : rebase #
Messages
Total messages: 41 (27 generated)
The CQ bit was checked by steimel@chromium.org 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...
Description was changed from ========== Prevent Media Controls from hiding when the user is interacting via the keyboard BUG=701440 ========== to ========== Prevent Media Controls from hiding when the user is interacting via the keyboard This CL prevents the controls from hiding by resetting the hide timer on events in two places: 1) Listening for focusin and input events on the MediaControls via defaultEventHandler, which get fired as the user tabs through the controls and as the user changes the timeline/volume sliders respectively. 2) Listening for keypress events on the media panel via adding an event listener to the panel element in MediaControlsMediaEventListener, which gets fired when the user mutes/unmutes, turns CC on/off, etc. BUG=701440 ==========
steimel@chromium.org changed reviewers: + avayvod@chromium.org, mlamouri@chromium.org
PTAL :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by steimel@chromium.org 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.
mlamouri@chromium.org changed reviewers: + johnme@chromium.org - avayvod@chromium.org
johnme@, can you take a look?
friendly ping :)
Looks good, thanks! Left a few questions. https://codereview.chromium.org/2757323002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/2757323002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:908: resetHideMediaControlsTimer(); It seems that `resetHideMediaControlsTimer` will set `m_keepShowingUntilTimerFires` to false if it is currently true. Perhaps setting `m_keepShowingUntilTimerFires` to true should be moved from `defaultEventHandler` to `startHideMediaControlsTimer`? That code is a little opaque :-| https://codereview.chromium.org/2757323002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/shadow/MediaControlsMediaEventListener.cpp (right): https://codereview.chromium.org/2757323002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/shadow/MediaControlsMediaEventListener.cpp:48: m_mediaControls->panelElement()->addEventListener(EventTypeNames::keypress, Should you also removeEventListener in detach()? https://codereview.chromium.org/2757323002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/shadow/MediaControlsMediaEventListener.cpp:133: if (event->currentTarget() == m_mediaControls->panelElement()) Why do you need to check the currentTarget? The other events don't seem to do that. Is it to avoid duplicate dispatch as it bubbles?
In addition of johnme@'s comments, can you add sweet sweet tests? :)
The CQ bit was checked by steimel@chromium.org to run a CQ dry run
PTAL https://codereview.chromium.org/2757323002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/2757323002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/shadow/MediaControls.cpp:908: resetHideMediaControlsTimer(); On 2017/03/30 17:21:41, johnme wrote: > It seems that `resetHideMediaControlsTimer` will set > `m_keepShowingUntilTimerFires` to false if it is currently true. Perhaps setting > `m_keepShowingUntilTimerFires` to true should be moved from > `defaultEventHandler` to `startHideMediaControlsTimer`? That code is a little > opaque :-| Discussed offline with mlamouri@. It will be looked at in a separate CL https://codereview.chromium.org/2757323002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/shadow/MediaControlsMediaEventListener.cpp (right): https://codereview.chromium.org/2757323002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/shadow/MediaControlsMediaEventListener.cpp:48: m_mediaControls->panelElement()->addEventListener(EventTypeNames::keypress, On 2017/03/30 17:21:41, johnme wrote: > Should you also removeEventListener in detach()? Good catch. Done. https://codereview.chromium.org/2757323002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/shadow/MediaControlsMediaEventListener.cpp:133: if (event->currentTarget() == m_mediaControls->panelElement()) On 2017/03/30 17:21:41, johnme wrote: > Why do you need to check the currentTarget? The other events don't seem to do > that. Is it to avoid duplicate dispatch as it bubbles? lethalantidote@ is working on a CL that will also look for keypress events, but on a different element. This is proactively setting it up so that when they add their listener, they can add a check for the currentTarget they're looking for
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.
lgtm with nits - thanks! https://codereview.chromium.org/2757323002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/shadow/MediaControlsMediaEventListener.cpp (right): https://codereview.chromium.org/2757323002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControlsMediaEventListener.cpp:47: if (m_mediaControls->panelElement()) { Nit: it doesn't seem possible for this to be null, since MediaControlsImpl::create initialises the media controls (which sets m_panel) immediately after constructing it. Ditto in detach. https://codereview.chromium.org/2757323002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/shadow/MediaControlsTest.cpp (right): https://codereview.chromium.org/2757323002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControlsTest.cpp:510: m_platform->advanceClockSeconds(1); Can you add a comment explaining why you need to advance the clock at the beginning of the test? https://codereview.chromium.org/2757323002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControlsTest.cpp:516: Element* panel = getElementByShadowPseudoId(mediaControls(), Nit: you can use mediaControls().panelElement() here (and shouldn't be necessary to null check it)
lgtm https://codereview.chromium.org/2757323002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/shadow/MediaControlsMediaEventListener.cpp (right): https://codereview.chromium.org/2757323002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControlsMediaEventListener.cpp:47: if (m_mediaControls->panelElement()) { On 2017/04/06 at 17:15:28, johnme wrote: > Nit: it doesn't seem possible for this to be null, since MediaControlsImpl::create initialises the media controls (which sets m_panel) immediately after constructing it. Ditto in detach. Indeed, we always assume it's available. It should probably return a reference to make this clear though.
Oh, I'm afraid you will likely need to rebase this :(
The CQ bit was checked by steimel@chromium.org 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...
PTAL. I rebased yesterday afternoon, not sure if the potential conflict you're thinking of happened after that or not https://codereview.chromium.org/2757323002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/shadow/MediaControlsMediaEventListener.cpp (right): https://codereview.chromium.org/2757323002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControlsMediaEventListener.cpp:47: if (m_mediaControls->panelElement()) { On 2017/04/07 13:21:21, mlamouri wrote: > On 2017/04/06 at 17:15:28, johnme wrote: > > Nit: it doesn't seem possible for this to be null, since > MediaControlsImpl::create initialises the media controls (which sets m_panel) > immediately after constructing it. Ditto in detach. > > Indeed, we always assume it's available. It should probably return a reference > to make this clear though. I was getting segfaults before I added this. Didn't look too much into it, but this codepath was getting hit twice, first with the panel==null, and then with the panel!=null. https://codereview.chromium.org/2757323002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/shadow/MediaControlsTest.cpp (right): https://codereview.chromium.org/2757323002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControlsTest.cpp:510: m_platform->advanceClockSeconds(1); On 2017/04/06 17:15:28, johnme wrote: > Can you add a comment explaining why you need to advance the clock at the > beginning of the test? Done. https://codereview.chromium.org/2757323002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControlsTest.cpp:516: Element* panel = getElementByShadowPseudoId(mediaControls(), On 2017/04/06 17:15:28, johnme wrote: > Nit: you can use mediaControls().panelElement() here (and shouldn't be necessary > to null check it) Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Still lgtm. No, you'll still need to rebase over a patch that moves some of the media controls code. git rebase should do a good job of migrating your changes though (it'll be easier to merge them if you run `git config --global merge.conflictstyle diff3` first and/or use a graphical merge tool).
The CQ bit was checked by steimel@chromium.org 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...
lol yep, figured once I saw all the bots had failed :). Rebased and uploaded
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by steimel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mlamouri@chromium.org, johnme@chromium.org Link to the patchset: https://codereview.chromium.org/2757323002/#ps60001 (title: "rebase")
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": 60001, "attempt_start_ts": 1491596139873360,
"parent_rev": "27458c3a4574379467168add07d7e6d7bf40c95d", "commit_rev":
"12df228997d26f76483eb438602b22897c24d094"}
Message was sent while issue was closed.
Description was changed from ========== Prevent Media Controls from hiding when the user is interacting via the keyboard This CL prevents the controls from hiding by resetting the hide timer on events in two places: 1) Listening for focusin and input events on the MediaControls via defaultEventHandler, which get fired as the user tabs through the controls and as the user changes the timeline/volume sliders respectively. 2) Listening for keypress events on the media panel via adding an event listener to the panel element in MediaControlsMediaEventListener, which gets fired when the user mutes/unmutes, turns CC on/off, etc. BUG=701440 ========== to ========== Prevent Media Controls from hiding when the user is interacting via the keyboard This CL prevents the controls from hiding by resetting the hide timer on events in two places: 1) Listening for focusin and input events on the MediaControls via defaultEventHandler, which get fired as the user tabs through the controls and as the user changes the timeline/volume sliders respectively. 2) Listening for keypress events on the media panel via adding an event listener to the panel element in MediaControlsMediaEventListener, which gets fired when the user mutes/unmutes, turns CC on/off, etc. BUG=701440 Review-Url: https://codereview.chromium.org/2757323002 Cr-Commit-Position: refs/heads/master@{#462990} Committed: https://chromium.googlesource.com/chromium/src/+/12df228997d26f76483eb438602b... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/12df228997d26f76483eb438602b... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
