Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
914043002:
Tap on the video element toggles controls visibility (Closed)
|
Created:
5 years, 10 months ago by Tima Vaisburd Modified:
4 years, 2 months ago CC:
blink-reviews, nessy, gasubic, fs, eric.carlson_apple.com, feature-media-reviews_chromium.org, dglazkov+blink, blink-reviews-html_chromium.org, vcarbune.chromium Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionTap on the video element toggles controls visibility
We would like to match YouTube where a tap turns controls
on and off.
This CL tries to decouple tap gesture from mouse events
and process them separately.
BUG=436592
Patch Set 1 #
Total comments: 6
Patch Set 2 : Reset the hiding timer while touching controls, skip input events etc. for m_wasLastEventTouch #
Total comments: 5
Patch Set 3 : Rebased, process mouse only for non-synthetic events. #
Total comments: 15
Messages
Total messages: 32 (4 generated)
timav@chromium.org changed reviewers: + qinmin@chromium.org
Min, Could you, please, verify that this aproach makes sense? If yes I will think of the tests.
philipj@opera.com changed reviewers: + fs@opera.com, philipj@opera.com
This is great, we need to solve this. However, there's one small adjustment to the behavior I would like, namely that when the controls are hidden, it should be enough to simply touch the screen (touchstart) in order to show the controls. Both YouTube and Android's built-in "Video player" app that the Gallery app uses behave like this and I quite like it. https://codereview.chromium.org/914043002/diff/1/Source/core/html/shadow/Medi... File Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/914043002/diff/1/Source/core/html/shadow/Medi... Source/core/html/shadow/MediaControls.cpp:460: // Do not generate mouse events from this tap Is this necessary? How about simply listening to click events and ignoring those where fromTouch() is false? https://codereview.chromium.org/914043002/diff/1/Source/core/html/shadow/Medi... Source/core/html/shadow/MediaControls.cpp:466: // Prevent immediate hiding by playbackProgressed() This seems like a bit of a hack, I think it would be better to try to distinguish between real and synthetic mouse events when setting m_isMouseOverControls instead.
On 2015/02/11 06:57:38, philipj_UTC7 wrote: > There's one small adjustment to > the behavior I would like, namely that when the controls are hidden, it should > be enough to simply touch the screen (touchstart) in order to show the controls. > Both YouTube and Android's built-in "Video player" app that the Gallery app uses > behave like this and I quite like it. > Hi Philip, In the full screen mode this seems to already behave like this (after this CL applied): no matter where I touch the hidden controls appear for me. In the non-fullscreen mode hitting some area outside the media element will often follow the link and switch the content, and I think this is the right thing to do. YouTube behaves the same way. Could you, please, clarify your point? I will try to address the other things you mentioned and comment on them later.
https://codereview.chromium.org/914043002/diff/1/Source/core/html/shadow/Medi... File Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/914043002/diff/1/Source/core/html/shadow/Medi... Source/core/html/shadow/MediaControls.cpp:461: event->preventDefault(); What if there is a onclick eventhandler on the MediaElement?
https://codereview.chromium.org/914043002/diff/1/Source/core/html/shadow/Medi... File Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/914043002/diff/1/Source/core/html/shadow/Medi... Source/core/html/shadow/MediaControls.cpp:461: event->preventDefault(); On 2015/02/11 22:29:12, qinmin wrote: > What if there is a onclick eventhandler on the MediaElement? In this case the solution won't work. We can't simply remove this preventDefault() though: in this case an innocent tap to bring the controls might unexpectedly trigger some action like pause. I tried that.
On 2015/02/11 19:47:11, Tima Vaisburd wrote: > On 2015/02/11 06:57:38, philipj_UTC7 wrote: > > There's one small adjustment to > > the behavior I would like, namely that when the controls are hidden, it should > > be enough to simply touch the screen (touchstart) in order to show the > controls. > > Both YouTube and Android's built-in "Video player" app that the Gallery app > uses > > behave like this and I quite like it. > > > > Hi Philip, > In the full screen mode this seems to already behave like this (after this CL > applied): > no matter where I touch the hidden controls appear for me. In the non-fullscreen > mode > hitting some area outside the media element will often follow the link and > switch the content, > and I think this is the right thing to do. YouTube behaves the same way. > Could you, please, clarify your point? I mean that the controls should be shown immediately when the finger touches the screen (touchstart), not after a full tap (touchstart+touchend). Nothing in your CL seems to change behavior in fullscreen, what does "In the non-fullscreen mode hitting some area outside the media element will often follow the link and switch the content" mean? Obviously if you're not touching the media element then this code will do nothing, I'm just saying that when you are touching the media element, the behavior should be the same regardless of fullscreen.
https://codereview.chromium.org/914043002/diff/1/Source/core/html/shadow/Medi... File Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/914043002/diff/1/Source/core/html/shadow/Medi... Source/core/html/shadow/MediaControls.cpp:461: event->preventDefault(); On 2015/02/11 22:48:20, Tima Vaisburd wrote: > On 2015/02/11 22:29:12, qinmin wrote: > > What if there is a onclick eventhandler on the MediaElement? > > In this case the solution won't work. > We can't simply remove this preventDefault() though: in this case an innocent > tap to bring the controls might unexpectedly trigger some action like pause. I > tried that. What causes the pause? Maybe you were testing while my activation behavior was in there, that's been reverted now. If the page has a click event handler I think that should simply be run. Some events are prevented from leaving the media controls, but I don't think it makes sense to capture them when nothing observable happens to the video element, we're just making the controls visible here...
On 2015/02/12 02:29:01, philipj_UTC7 wrote: > I mean that the controls should be shown immediately when the finger touches the > screen (touchstart), not after a full tap (touchstart+touchend). I got it. I tried Youtube, the full screen behavior is quite nice: show on touchstart, remove on second touchend. The non-fullscreen behavior is on full cycle though. We can try to have it the nice way and consistent. > Obviously if you're not touching the media element then this code will do nothing, Yes, please ignore what I wrote there.
On 2015/02/12 02:31:21, philipj_UTC7 wrote: > > We can't simply remove this preventDefault() though: in this case an innocent > > tap to bring the controls might unexpectedly trigger some action like pause. > > What causes the pause? Imagine the situation when controls are hidden and the video is playing. You tap at the bottom of the screen. You do not see any controls yet, obviously, so the position of you tap will be quite random. If your tap happened to be at the lower left corner, it might go at the position of the hidden Pause/Play button. When I did not call preventDefault() I saw that the button received the click from the tap. Similarly, if you tap at the bottom center instead I saw the seek happened. I found it quite confusing for the user.
In this new patch I tried to fix 2 bugs: 1. Controls disappear "under my fingers" because the timer was not extended when we touch the knobs, 2. Sometimes the controls do not hide. This happens when m_panel->hovered() is true but a subsequent event erased the m_wasLastEventTouch that was introduced just for this purpose. https://codereview.chromium.org/914043002/diff/1/Source/core/html/shadow/Medi... File Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/914043002/diff/1/Source/core/html/shadow/Medi... Source/core/html/shadow/MediaControls.cpp:466: // Prevent immediate hiding by playbackProgressed() On 2015/02/11 06:57:38, philipj_UTC7 wrote: > This seems like a bit of a hack, I think it would be better to try to > distinguish between real and synthetic mouse events when setting > m_isMouseOverControls instead. Yes, I agree, but so far I haven;t come up with a better way. I'd appreciate an advise. https://codereview.chromium.org/914043002/diff/20001/Source/core/html/shadow/... File Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/914043002/diff/20001/Source/core/html/shadow/... Source/core/html/shadow/MediaControls.cpp:464: m_wasLastEventTouch = event->isTouchEvent() || event->isGestureEvent() This is used to prevent sticking the controls forever because the touch event in the control area might leave a synthetic mouse there. But non-mouse events like input delete this flag. Can we have a more drastic approach and say if at least one touch happened then we should ignore panel->hover()? https://codereview.chromium.org/914043002/diff/20001/Source/core/html/shadow/... Source/core/html/shadow/MediaControls.cpp:494: if (event->type() == EventTypeNames::mouseover) { I can add && toMouseEvent(event)->fromTouch() in these 3 cases.
On 2015/02/12 02:51:26, Tima Vaisburd wrote: > On 2015/02/12 02:31:21, philipj_UTC7 wrote: > > > > We can't simply remove this preventDefault() though: in this case an > innocent > > > tap to bring the controls might unexpectedly trigger some action like pause. > > > > What causes the pause? > > Imagine the situation when controls are hidden and the video is playing. > You tap at the bottom of the screen. You do not see any controls yet, obviously, > so the position of you tap will be quite random. > > If your tap happened to be at the lower left corner, it might go at the position > of the hidden Pause/Play button. When I did not call preventDefault() I saw that > the button received the click from the tap. Similarly, if you tap at the bottom > center > instead I saw the seek happened. I found it quite confusing for the user. Oh... so the touchstart causes the controls to be shown, and that same touchstart + later touchend triggers a synthetic click event which does things with the now visible controls. That is definitely bad! I will have to apply your CL and try some things. Ideally we could keep track of "the touchstart which led to the click event caused the controls become visible" and do nothing for that case, but doing that for each control element separately would be pretty terrible.
https://codereview.chromium.org/914043002/diff/20001/Source/core/html/shadow/... File Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/914043002/diff/20001/Source/core/html/shadow/... Source/core/html/shadow/MediaControls.cpp:464: m_wasLastEventTouch = event->isTouchEvent() || event->isGestureEvent() On 2015/02/12 03:18:02, Tima Vaisburd wrote: > This is used to prevent sticking the controls forever because the touch event in > the control area might leave a synthetic mouse there. But non-mouse events like > input delete this flag. Can we have a more drastic approach and say if at least > one touch happened then we should ignore panel->hover()? As you suggested yourself, I would first investigate if we can't simply ignore synthetic mouseover, mouseout and mousemove events. They are currently needed, but won't be if touch events are handled separately. See also https://codereview.chromium.org/917043003/ https://codereview.chromium.org/914043002/diff/20001/Source/core/html/shadow/... Source/core/html/shadow/MediaControls.cpp:494: if (event->type() == EventTypeNames::mouseover) { On 2015/02/12 03:18:02, Tima Vaisburd wrote: > I can add && toMouseEvent(event)->fromTouch() in these 3 cases. Yes, this sounds worth trying! If it doesn't work investigate if fromTouch() actually works for these events, it's easy to imagine it only working for click events or something.
On 2015/02/12 04:02:03, philipj_UTC7 wrote: > I will have to apply your CL and try some things. Please do! After playing with it a little more I understood why we need to react to the touchdown: now a quick tap toggles controls, but a long one or touch-and-drag does not. Feels weird. > Ideally we could keep track of > "the touchstart which led to the click event caused the controls become visible" > and do nothing for that case, but doing that for each control element separately > would be pretty terrible. I'm not sure how can we do that, can we attach some mark to an event that would survive touch -> mouse transformations? I have to switch to something else for a couple of days.
https://codereview.chromium.org/914043002/diff/20001/Source/core/html/shadow/... File Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/914043002/diff/20001/Source/core/html/shadow/... Source/core/html/shadow/MediaControls.cpp:494: if (event->type() == EventTypeNames::mouseover) { On 2015/02/12 04:26:28, philipj_UTC7 wrote: > On 2015/02/12 03:18:02, Tima Vaisburd wrote: > > I can add && toMouseEvent(event)->fromTouch() in these 3 cases. > > Yes, this sounds worth trying! If it doesn't work investigate if fromTouch() > actually works for these events, it's easy to imagine it only working for click > events or something. I added this condition, it seems to have to effect on both Linux and Android version (that is, seems to not hurt).
On 2015/02/12 18:52:17, Tima Vaisburd wrote: > On 2015/02/12 04:02:03, philipj_UTC7 wrote: > > > I will have to apply your CL and try some things. > > Please do! After playing with it a little more I understood why we need to react > to the touchdown: now a quick tap toggles controls, but a long one or > touch-and-drag does not. > Feels weird. > > > Ideally we could keep track of > > "the touchstart which led to the click event caused the controls become > visible" > > and do nothing for that case, but doing that for each control element > separately > > would be pretty terrible. > > I'm not sure how can we do that, can we attach some mark to an event that would > survive touch -> mouse transformations? > > I have to switch to something else for a couple of days. I'll get back to this in Monday, I'm going on a little weekend trip now.
It's Monday! https://codereview.chromium.org/914043002/diff/40001/Source/core/html/shadow/... File Source/core/html/shadow/MediaControlElements.cpp (right): https://codereview.chromium.org/914043002/diff/40001/Source/core/html/shadow/... Source/core/html/shadow/MediaControlElements.cpp:381: mediaControls().timelineKnobDragged(); Isn't this fixing an unrelated problem, perhaps the same as the recently fixed https://code.google.com/p/chromium/issues/detail?id=454304 ? https://codereview.chromium.org/914043002/diff/40001/Source/core/html/shadow/... File Source/core/html/shadow/MediaControlElements.h (right): https://codereview.chromium.org/914043002/diff/40001/Source/core/html/shadow/... Source/core/html/shadow/MediaControlElements.h:48: bool isOpaque() const { return m_opaque; } What about m_isDisplayed? I assume you want to check "are the controls currently visible to the user" and unfortunately MediaControlPanelElement::makeOpaque() involves two state bits. https://codereview.chromium.org/914043002/diff/40001/Source/core/html/shadow/... File Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/914043002/diff/40001/Source/core/html/shadow/... Source/core/html/shadow/MediaControls.cpp:443: bool MediaControls::isInactiveElement(Node* node) const If isn't mate moot by other changes, would willRespondToMouseClickEvents() serve the same purpose? https://codereview.chromium.org/914043002/diff/40001/Source/core/html/shadow/... Source/core/html/shadow/MediaControls.cpp:462: bool mouseIsSynthetic = event->isMouseEvent() && toMouseEvent(event)->fromTouch(); Perhaps isMouseEventFromTouch will make sound less like there's a virtual mouse or something involved. https://codereview.chromium.org/914043002/diff/40001/Source/core/html/shadow/... Source/core/html/shadow/MediaControls.cpp:463: bool wasLastEventTouch = event->isTouchEvent() || event->isGestureEvent() I just renamed this, but actually isTouchEvent would be more accurate now. https://codereview.chromium.org/914043002/diff/40001/Source/core/html/shadow/... Source/core/html/shadow/MediaControls.cpp:470: if (event->type() == EventTypeNames::gesturetap) { Did you first try using the touchstart and click events for showing and hiding respectively? GestureEvent was added to WebKit in 2012 <https://trac.webkit.org/changeset/123799> with the comment "This change does not expose the gesture events to JavaScript, since there is no spec for that at the moment." There's still no spec and AFAICT the only other place where this is used internally is HTMLSelectElement::listBoxDefaultEventHandler, so if there's any way at all of avoiding GestureEvent, that seems worthwhile. https://codereview.chromium.org/914043002/diff/40001/Source/core/html/shadow/... Source/core/html/shadow/MediaControls.cpp:476: event->preventDefault(); This will be observable to Web content. An alternative could be to ignore clicks where the target has changed: Keep a (weak?) reference to the event target in the (touchstart?) event that caused the controls to be shown. In from-touch click events, if the event target is not the same, do nothing. If it's a weak ref there should be no need to worry about when to clear it. https://codereview.chromium.org/914043002/diff/40001/Source/core/html/shadow/... Source/core/html/shadow/MediaControls.cpp:482: m_isMouseOverControls = true; This seems like an abuse of this state variable, as the mouse may not be over the controls at this point.
Thank you for the thorough review! https://codereview.chromium.org/914043002/diff/40001/Source/core/html/shadow/... File Source/core/html/shadow/MediaControlElements.cpp (right): https://codereview.chromium.org/914043002/diff/40001/Source/core/html/shadow/... Source/core/html/shadow/MediaControlElements.cpp:381: mediaControls().timelineKnobDragged(); On 2015/02/16 10:45:18, philipj_UTC7 wrote: > Isn't this fixing an unrelated problem, perhaps the same as the recently fixed > https://code.google.com/p/chromium/issues/detail?id=454304 ? I might also say "related, but different problem". Yes, maybe it does not belong in here. I saw your change that you refer in here. As far as I can see your change does not fix that particular scenario (i.e. a user presses and drags back and forth without stopping). https://codereview.chromium.org/914043002/diff/40001/Source/core/html/shadow/... File Source/core/html/shadow/MediaControlElements.h (right): https://codereview.chromium.org/914043002/diff/40001/Source/core/html/shadow/... Source/core/html/shadow/MediaControlElements.h:48: bool isOpaque() const { return m_opaque; } On 2015/02/16 10:45:18, philipj_UTC7 wrote: > What about m_isDisplayed? I assume you want to check "are the controls currently > visible to the user" and unfortunately MediaControlPanelElement::makeOpaque() > involves two state bits. Thank you, I did not pay enough attention to this. But, wouldn't it change the behavior only for those taps that came in the middle of fading? https://codereview.chromium.org/914043002/diff/40001/Source/core/html/shadow/... File Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/914043002/diff/40001/Source/core/html/shadow/... Source/core/html/shadow/MediaControls.cpp:470: if (event->type() == EventTypeNames::gesturetap) { On 2015/02/16 10:45:19, philipj_UTC7 wrote: > Did you first try using the touchstart and click events for showing and hiding > respectively? GestureEvent was added to WebKit in 2012 > <https://trac.webkit.org/changeset/123799> with the comment "This change does > not expose the gesture events to JavaScript, since there is no spec for that at > the moment." > > There's still no spec and AFAICT the only other place where this is used > internally is HTMLSelectElement::listBoxDefaultEventHandler, so if there's any > way at all of avoiding GestureEvent, that seems worthwhile. No, I must admit I haven't tried the touchstart event at all. Moreover, only recently did I realize that touchstart, touchend, touchmove and touchcancel events do not propagate to the renderer process right now. We need to add a TouchEvent handler to the EventHandlerRegistry for that. On the other hand processing these events seems necessary for the implementation of YouTube-like behavior that you mentioned earlier. I guess this consideration invalidates the current approach completely.
https://codereview.chromium.org/914043002/diff/40001/Source/core/html/shadow/... File Source/core/html/shadow/MediaControlElements.cpp (right): https://codereview.chromium.org/914043002/diff/40001/Source/core/html/shadow/... Source/core/html/shadow/MediaControlElements.cpp:381: mediaControls().timelineKnobDragged(); On 2015/02/16 20:32:11, Tima Vaisburd wrote: > On 2015/02/16 10:45:18, philipj_UTC7 wrote: > > Isn't this fixing an unrelated problem, perhaps the same as the recently fixed > > https://code.google.com/p/chromium/issues/detail?id=454304 ? > > I might also say "related, but different problem". Yes, maybe it does not belong > in here. > I saw your change that you refer in here. As far as I can see your change does > not fix that particular scenario (i.e. a user presses and drags back and forth > without stopping). My change was just cleanup, https://codereview.chromium.org/892963003 was the actual fix. Anyway, unless this problem only surfaces because of the "touch to show/hide" changes, I suggest a separate CL and test for it. https://codereview.chromium.org/914043002/diff/40001/Source/core/html/shadow/... File Source/core/html/shadow/MediaControlElements.h (right): https://codereview.chromium.org/914043002/diff/40001/Source/core/html/shadow/... Source/core/html/shadow/MediaControlElements.h:48: bool isOpaque() const { return m_opaque; } On 2015/02/16 20:32:11, Tima Vaisburd wrote: > On 2015/02/16 10:45:18, philipj_UTC7 wrote: > > What about m_isDisplayed? I assume you want to check "are the controls > currently > > visible to the user" and unfortunately MediaControlPanelElement::makeOpaque() > > involves two state bits. > > Thank you, I did not pay enough attention to this. But, wouldn't it change the > behavior only for those taps that came in the middle of fading? http://trac.webkit.org/changeset/115125 is where m_isDisplayed was added. I think the idea is that makeOpaque can be called unconditionally wherever something should trigger the controls to become visible, but unless MediaControls::show() has been called it doesn't matter. This is somewhat confusing and I could be wrong, but I think m_opaque && m_isDisplayed ought to mean that it's really visible. Maybe just inspecting CSSPropertyDisplay and CSSPropertyOpacity would be easier. https://codereview.chromium.org/914043002/diff/40001/Source/core/html/shadow/... File Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/914043002/diff/40001/Source/core/html/shadow/... Source/core/html/shadow/MediaControls.cpp:470: if (event->type() == EventTypeNames::gesturetap) { On 2015/02/16 20:32:11, Tima Vaisburd wrote: > On 2015/02/16 10:45:19, philipj_UTC7 wrote: > > Did you first try using the touchstart and click events for showing and hiding > > respectively? GestureEvent was added to WebKit in 2012 > > <https://trac.webkit.org/changeset/123799> with the comment "This change does > > not expose the gesture events to JavaScript, since there is no spec for that > at > > the moment." > > > > There's still no spec and AFAICT the only other place where this is used > > internally is HTMLSelectElement::listBoxDefaultEventHandler, so if there's any > > way at all of avoiding GestureEvent, that seems worthwhile. > > No, I must admit I haven't tried the touchstart event at all. Moreover, only > recently did I realize that touchstart, touchend, touchmove and touchcancel > events do not propagate to the renderer process right now. We need to add a > TouchEvent handler to the EventHandlerRegistry for that. > On the other hand processing these events seems necessary for the implementation > of YouTube-like behavior that you mentioned earlier. I guess this consideration > invalidates the current approach completely. Overriding Node::willRespondToTouchEvents() ought to do it, but I'm not sure. I just found that MediaControlOverlayEnclosureElement::preDispatchEventHandler is doing something very similar to this CL, but it doesn't override willRespondToTouchEvents() so either it doesn't work or I don't understand why it works. Using preDispatchEventHandler is probably a good idea, though.
philipj@opera.com changed reviewers: + aberent@chromium.org
aberent@, can you comment on the MediaControlOverlayEnclosureElement::preDispatchEventHandler case? Does the touchstart case actually work? I feel like we're doing very similar things here, so I'm wondering if it would make sense to have the code for mouseover/mouseout/mousemove/touchstart/click at a higher level and show/hide the controls panel, overlay play button and cast button under the exact same conditions?
On 2015/02/17 at 02:41:25, philipj wrote: > aberent@, can you comment on the MediaControlOverlayEnclosureElement::preDispatchEventHandler case? Does the touchstart case actually work? Yes, I am fairly sure that touchstart does work (or did when I added the code a few months ago). As far as I remember I discovered the need to test for touchstart in addition to click from some real sites, and adding testing for touchstart solved the problem. Unfortunately I don't have a record of which sites these were, so I can't easily retest this, although I should possibly add a layout test to test for this particular case. > > I feel like we're doing very similar things here, so I'm wondering if it would make sense to have the code for mouseover/mouseout/mousemove/touchstart/click at a higher level and show/hide the controls panel, overlay play button and cast button under the exact same conditions? Sounds sensible.
I saw the comments, I can come back to this next week only. https://codereview.chromium.org/914043002/diff/40001/Source/core/html/shadow/... File Source/core/html/shadow/MediaControls.cpp (right): https://codereview.chromium.org/914043002/diff/40001/Source/core/html/shadow/... Source/core/html/shadow/MediaControls.cpp:470: if (event->type() == EventTypeNames::gesturetap) { On 2015/02/17 02:37:24, philipj_UTC7 wrote: > On 2015/02/16 20:32:11, Tima Vaisburd wrote: > > touchstart, touchend, touchmove and touchcancel > > events do not propagate to the renderer process > Overriding Node::willRespondToTouchEvents() ought to do it I was wrong: whether touch events propagate or not depends on the TouchEvent registration (https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...). HTMLInputEvents does this already, and this explains why touches come to the derived elements (timeline etc.), but not to MediaControlOverlayEnclosureElement. Adding the registration where we need it seems easy.
On 2015/02/19 19:32:29, Tima Vaisburd wrote: > I saw the comments, I can come back to this next week only. > > https://codereview.chromium.org/914043002/diff/40001/Source/core/html/shadow/... > File Source/core/html/shadow/MediaControls.cpp (right): > > https://codereview.chromium.org/914043002/diff/40001/Source/core/html/shadow/... > Source/core/html/shadow/MediaControls.cpp:470: if (event->type() == > EventTypeNames::gesturetap) { > On 2015/02/17 02:37:24, philipj_UTC7 wrote: > > On 2015/02/16 20:32:11, Tima Vaisburd wrote: > > > touchstart, touchend, touchmove and touchcancel > > > events do not propagate to the renderer process > > Overriding Node::willRespondToTouchEvents() ought to do it > > I was wrong: whether touch events propagate or not depends on the TouchEvent > registration > (https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...). > HTMLInputEvents does this already, and this explains why touches come to the > derived elements (timeline etc.), but not to > MediaControlOverlayEnclosureElement. Adding the registration where we need it > seems easy. I would have guessed that overriding Node::willRespondToTouchEvents() would be enough, but I haven't tested it.
On 2015/02/20 04:22:19, philipj wrote: > On 2015/02/19 19:32:29, Tima Vaisburd wrote: > > I saw the comments, I can come back to this next week only. > > > > > https://codereview.chromium.org/914043002/diff/40001/Source/core/html/shadow/... > > File Source/core/html/shadow/MediaControls.cpp (right): > > > > > https://codereview.chromium.org/914043002/diff/40001/Source/core/html/shadow/... > > Source/core/html/shadow/MediaControls.cpp:470: if (event->type() == > > EventTypeNames::gesturetap) { > > On 2015/02/17 02:37:24, philipj_UTC7 wrote: > > > On 2015/02/16 20:32:11, Tima Vaisburd wrote: > > > > touchstart, touchend, touchmove and touchcancel > > > > events do not propagate to the renderer process > > > Overriding Node::willRespondToTouchEvents() ought to do it > > > > I was wrong: whether touch events propagate or not depends on the TouchEvent > > registration > > > (https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...). > > HTMLInputEvents does this already, and this explains why touches come to the > > derived elements (timeline etc.), but not to > > MediaControlOverlayEnclosureElement. Adding the registration where we need it > > seems easy. > > I would have guessed that overriding Node::willRespondToTouchEvents() would be > enough, but I haven't tested it. Is this still current? If not please close.
On 2015/06/29 09:56:34, aberent wrote: > On 2015/02/20 04:22:19, philipj wrote: > > On 2015/02/19 19:32:29, Tima Vaisburd wrote: > > > I saw the comments, I can come back to this next week only. > > > > > > > > > https://codereview.chromium.org/914043002/diff/40001/Source/core/html/shadow/... > > > File Source/core/html/shadow/MediaControls.cpp (right): > > > > > > > > > https://codereview.chromium.org/914043002/diff/40001/Source/core/html/shadow/... > > > Source/core/html/shadow/MediaControls.cpp:470: if (event->type() == > > > EventTypeNames::gesturetap) { > > > On 2015/02/17 02:37:24, philipj_UTC7 wrote: > > > > On 2015/02/16 20:32:11, Tima Vaisburd wrote: > > > > > touchstart, touchend, touchmove and touchcancel > > > > > events do not propagate to the renderer process > > > > Overriding Node::willRespondToTouchEvents() ought to do it > > > > > > I was wrong: whether touch events propagate or not depends on the TouchEvent > > > registration > > > > > > (https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...). > > > HTMLInputEvents does this already, and this explains why touches come to the > > > derived elements (timeline etc.), but not to > > > MediaControlOverlayEnclosureElement. Adding the registration where we need > it > > > seems easy. > > > > I would have guessed that overriding Node::willRespondToTouchEvents() would be > > enough, but I haven't tested it. > > Is this still current? If not please close. Philip, I'm not working on the issue currently. Do you work on a newer/better version so this CL can be closed?
On 2015/07/08 19:09:07, Tima Vaisburd wrote: > On 2015/06/29 09:56:34, aberent wrote: > > On 2015/02/20 04:22:19, philipj wrote: > > > On 2015/02/19 19:32:29, Tima Vaisburd wrote: > > > > I saw the comments, I can come back to this next week only. > > > > > > > > > > > > > > https://codereview.chromium.org/914043002/diff/40001/Source/core/html/shadow/... > > > > File Source/core/html/shadow/MediaControls.cpp (right): > > > > > > > > > > > > > > https://codereview.chromium.org/914043002/diff/40001/Source/core/html/shadow/... > > > > Source/core/html/shadow/MediaControls.cpp:470: if (event->type() == > > > > EventTypeNames::gesturetap) { > > > > On 2015/02/17 02:37:24, philipj_UTC7 wrote: > > > > > On 2015/02/16 20:32:11, Tima Vaisburd wrote: > > > > > > touchstart, touchend, touchmove and touchcancel > > > > > > events do not propagate to the renderer process > > > > > Overriding Node::willRespondToTouchEvents() ought to do it > > > > > > > > I was wrong: whether touch events propagate or not depends on the > TouchEvent > > > > registration > > > > > > > > > > (https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...). > > > > HTMLInputEvents does this already, and this explains why touches come to > the > > > > derived elements (timeline etc.), but not to > > > > MediaControlOverlayEnclosureElement. Adding the registration where we need > > it > > > > seems easy. > > > > > > I would have guessed that overriding Node::willRespondToTouchEvents() would > be > > > enough, but I haven't tested it. > > > > Is this still current? If not please close. > > Philip, I'm not working on the issue currently. Do you work on a newer/better > version so this CL can be closed? This will need some rework after the media controls rewrite: https://codereview.chromium.org/1156993013/ I don't have a replacement for this ready, though.
Is this CL still live? If not then please close it.
On 2016/10/04 08:43:21, aberent wrote: > Is this CL still live? If not then please close it. Closing since no interest has been shown after I stopped working on this.
Description was changed from ========== Tap on the video element toggles controls visibility We would like to match YouTube where a tap turns controls on and off. This CL tries to decouple tap gesture from mouse events and process them separately. BUG=436592 ========== to ========== Tap on the video element toggles controls visibility We would like to match YouTube where a tap turns controls on and off. This CL tries to decouple tap gesture from mouse events and process them separately. BUG=436592 ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
