Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2622273003:
Fixed volume slider element event handling (Closed)
|
Created:
3 years, 11 months ago by mustaq Modified:
3 years, 11 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, vcarbune.chromium Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFixed volume slider element event handling
MediaControlVolumeSliderElement::defaultEventHandler has making
redundant calls to setVolume() & setMuted() on mouse activity. E.g. if
a mouse click changed the slider position, the above calls were made 4
times, once for each of these events: mousedown, input, mouseup,
DOMActive, click. This crack got exposed when PointerEvents are enabled
by default on M55, adding pointermove, pointerdown & pointerup to the
list.
This CL fixes the code to trigger the calls to setVolume() & setMuted()
only when the slider position is changed. Also added pointer events to
certain lists of mouse events in the code.
BUG=677900
Review-Url: https://codereview.chromium.org/2622273003
Cr-Commit-Position: refs/heads/master@{#446032}
Committed: https://chromium.googlesource.com/chromium/src/+/74fce5949bdf05a92c2bc0bd98e6e3e977c55376
Patch Set 1 #Patch Set 2 : Remvoed logs, added a TODO. #
Total comments: 3
Patch Set 3 : Added a TODO #
Total comments: 8
Patch Set 4 : Addressed chcunningham's comments. #
Total comments: 9
Patch Set 5 : mlamouri's comments. #
Messages
Total messages: 28 (12 generated)
mustaq@chromium.org changed reviewers: + chcunningham@chromium.org
ptal. Manual testing shows the bug has been fixed, and the volume slider works as expected.
mustaq@chromium.org changed reviewers: + liberato@chromium.org
liberato@chromium.org changed reviewers: + mlamouri@chromium.org
oh! i misread the original bug. i thought it was the mute button! if i'd have realized, i could have saved you some trouble. really sorry about that. anyway, +mlamouri for owners review. i don't actually own this code. lg*m from my non-OWNERS perspective. :) -fl https://codereview.chromium.org/2622273003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp (right): https://codereview.chromium.org/2622273003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:765: event->type() == EventTypeNames::pointermove) can we (eventually) just ignore mouse* here in favor of pointers? if so, might be worth a TODO. i think that mouse vs pointer does have some special-case logic for when to restart the timer to hide the control bar. otherwise, i'm not sure that we'd want to care about the difference.
https://codereview.chromium.org/2622273003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp (right): https://codereview.chromium.org/2622273003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:765: event->type() == EventTypeNames::pointermove) On 2017/01/11 16:07:26, liberato wrote: > can we (eventually) just ignore mouse* here in favor of pointers? > if so, might be worth a TODO. > > i think that mouse vs pointer does have some special-case logic for when to > restart the timer to hide the control bar. otherwise, i'm not sure that we'd > want to care about the difference. I totally agree, will add a TODO. It would involve a change across all UI elements. I didn't attempt it here because I am not familiar with the code. E.g. I don't know how isUserInteractionEventForSlider() affects events through nested elements.
https://codereview.chromium.org/2622273003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp (right): https://codereview.chromium.org/2622273003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:765: event->type() == EventTypeNames::pointermove) On 2017/01/11 17:34:35, mustaq wrote: > On 2017/01/11 16:07:26, liberato wrote: > > can we (eventually) just ignore mouse* here in favor of pointers? > > if so, might be worth a TODO. > > > > i think that mouse vs pointer does have some special-case logic for when to > > restart the timer to hide the control bar. otherwise, i'm not sure that we'd > > want to care about the difference. > > I totally agree, will add a TODO. It would involve a change across all UI > elements. I didn't attempt it here because I am not familiar with the code. E.g. > I don't know how isUserInteractionEventForSlider() affects events through nested > elements. Done.
More non-owner questions. https://codereview.chromium.org/2622273003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp (right): https://codereview.chromium.org/2622273003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:100: type == EventTypeNames::mouseout || What is this method's purpose? I see its used to decide whether to keepEventInNode... but I'm confused about what that means. Do we prevent these events from bubbling beyond the current node? If so, why? https://codereview.chromium.org/2622273003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:763: event->type() == EventTypeNames::pointerover || If we can move l781 - 783 into the if (type == input) block, then this early return stuff can all be deleted. https://codereview.chromium.org/2622273003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:776: // TODO(mustaq): On a single click, this block is executed 5 or 6 times Why not go ahead and move this into the if ( type == input ) block above?
ptal https://codereview.chromium.org/2622273003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp (right): https://codereview.chromium.org/2622273003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:100: type == EventTypeNames::mouseout || On 2017/01/12 21:48:46, chcunningham_ooo_jan11 wrote: > What is this method's purpose? I see its used to decide whether to > keepEventInNode... but I'm confused about what that means. Do we prevent these > events from bubbling beyond the current node? If so, why? I too found it weird: EventTarget::keepEventInNode returns true for only MediaControlElements, among all descendants of EventTarget. I have no clue what's so special about the MediaControlElements. Added a TODO here for a later cleanup, to avoid breaking some assumption somewhere in media ui code which I am not familiar with. https://codereview.chromium.org/2622273003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:763: event->type() == EventTypeNames::pointerover || On 2017/01/12 21:48:46, chcunningham_ooo_jan11 wrote: > If we can move l781 - 783 into the if (type == input) block, then this early > return stuff can all be deleted. Done with the change you suggested. I wanted to do the same thing too but I wasn't sure if the MediaControlTimelineElement can possibly change w/o firing any slider "input" event. https://codereview.chromium.org/2622273003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:776: // TODO(mustaq): On a single click, this block is executed 5 or 6 times On 2017/01/12 21:48:46, chcunningham_ooo_jan11 wrote: > Why not go ahead and move this into the if ( type == input ) block above? Done. Again, I wasn't sure about correct the set of events that should trigger this.
Non owner LGTM - but I strongly encourage thorough owner review https://codereview.chromium.org/2622273003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp (right): https://codereview.chromium.org/2622273003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:100: type == EventTypeNames::mouseout || On 2017/01/17 19:32:42, mustaq wrote: > On 2017/01/12 21:48:46, chcunningham_ooo_jan11 wrote: > > What is this method's purpose? I see its used to decide whether to > > keepEventInNode... but I'm confused about what that means. Do we prevent these > > events from bubbling beyond the current node? If so, why? > > I too found it weird: EventTarget::keepEventInNode returns true for only > MediaControlElements, among all descendants of EventTarget. I have no clue > what's so special about the MediaControlElements. > > Added a TODO here for a later cleanup, to avoid breaking some assumption > somewhere in media ui code which I am not familiar with. Thinking more, it may be that we want to keep these events internal because, for better or worse, we don't expose the HTML behind the media player UI... its simply the <video> tag. So perhaps we don't want to then expose events for hidden DOM elements. Just my guess though https://codereview.chromium.org/2622273003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:776: // TODO(mustaq): On a single click, this block is executed 5 or 6 times On 2017/01/17 19:32:42, mustaq wrote: > On 2017/01/12 21:48:46, chcunningham_ooo_jan11 wrote: > > Why not go ahead and move this into the if ( type == input ) block above? > > Done. Again, I wasn't sure about correct the set of events that should trigger > this. Yeah, I'm also not sure. But this seems right to me. Look forward to owner feedback ;)
The CQ bit was checked by mlamouri@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.
A friendly reminder that we need owner's review here. It will be great to have this patch merged to M57.
lgtm with comments applied. Sorry for the delay :( https://codereview.chromium.org/2622273003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp (right): https://codereview.chromium.org/2622273003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:90: // true for that method. Can we nuke the method? I think it was created for that purpose. Any reason why you think it should be removed? https://codereview.chromium.org/2622273003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:750: if (event->type() == EventTypeNames::mousedown) { Should `pointerdown` be added? https://codereview.chromium.org/2622273003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:756: if (event->type() == EventTypeNames::mouseup) { ditto for `pointerup`? https://codereview.chromium.org/2622273003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:764: if (event->type() == EventTypeNames::input) { maybe an early return? https://codereview.chromium.org/2622273003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:825: if (event->type() == EventTypeNames::mousedown) `pointerdown`/`pointerup`?
https://codereview.chromium.org/2622273003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp (right): https://codereview.chromium.org/2622273003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:90: // true for that method. Can we nuke the method? On 2017/01/24 02:06:51, mlamouri wrote: > I think it was created for that purpose. Any reason why you think it should be > removed? Removed the TODO. I wasn't sure if the special handling for media elements (vs all event targets) is intentional. https://codereview.chromium.org/2622273003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:750: if (event->type() == EventTypeNames::mousedown) { On 2017/01/24 02:06:51, mlamouri wrote: > Should `pointerdown` be added? This may cause double handling. Because each "physical mouse down" fires a pointerdown followed by a mousedown, an UI element should handle either pointer* or mouse* events. Handling both is bad in general, e.g. consider a checkbox that toggles with these events. Ideally all mouse events sent to media controls should be filtered earlier so the conditional handling in the rest of the code is for pointer events only. We should address this separately from this bug fix. The TODO I added in MediaControlElementTypes.h covers this. Please let me know if you want more details there. https://codereview.chromium.org/2622273003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:756: if (event->type() == EventTypeNames::mouseup) { On 2017/01/24 02:06:51, mlamouri wrote: > ditto for `pointerup`? Same as above: want to avoid double-handling. https://codereview.chromium.org/2622273003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp:764: if (event->type() == EventTypeNames::input) { On 2017/01/24 02:06:51, mlamouri wrote: > maybe an early return? Done.
mustaq@, just as a reminder, I think this can land now.
The CQ bit was checked by mustaq@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/01/25 05:23:36, mlamouri (slow) wrote: > mustaq@, just as a reminder, I think this can land now. Just realized that I didn't upload my new patch last night. Here it is, committing now.
The CQ bit was checked by mustaq@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from chcunningham@chromium.org, mlamouri@chromium.org Link to the patchset: https://codereview.chromium.org/2622273003/#ps80001 (title: "mlamouri's comments.")
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": 80001, "attempt_start_ts": 1485356394512660,
"parent_rev": "76b2a485c54fb25c8a0fe271033715899c41a83c", "commit_rev":
"74fce5949bdf05a92c2bc0bd98e6e3e977c55376"}
Message was sent while issue was closed.
Description was changed from ========== Fixed volume slider element event handling MediaControlVolumeSliderElement::defaultEventHandler has making redundant calls to setVolume() & setMuted() on mouse activity. E.g. if a mouse click changed the slider position, the above calls were made 4 times, once for each of these events: mousedown, input, mouseup, DOMActive, click. This crack got exposed when PointerEvents are enabled by default on M55, adding pointermove, pointerdown & pointerup to the list. This CL fixes the code to trigger the calls to setVolume() & setMuted() only when the slider position is changed. Also added pointer events to certain lists of mouse events in the code. BUG=677900 ========== to ========== Fixed volume slider element event handling MediaControlVolumeSliderElement::defaultEventHandler has making redundant calls to setVolume() & setMuted() on mouse activity. E.g. if a mouse click changed the slider position, the above calls were made 4 times, once for each of these events: mousedown, input, mouseup, DOMActive, click. This crack got exposed when PointerEvents are enabled by default on M55, adding pointermove, pointerdown & pointerup to the list. This CL fixes the code to trigger the calls to setVolume() & setMuted() only when the slider position is changed. Also added pointer events to certain lists of mouse events in the code. BUG=677900 Review-Url: https://codereview.chromium.org/2622273003 Cr-Commit-Position: refs/heads/master@{#446032} Committed: https://chromium.googlesource.com/chromium/src/+/74fce5949bdf05a92c2bc0bd98e6... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/74fce5949bdf05a92c2bc0bd98e6... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
