Closed Bug 1327097 Opened 8 years ago Closed 7 years ago

Video doesn't seek if page prevented mousedown event or mouseup event

Categories

(Toolkit :: Video/Audio Controls, defect)

Product:
Toolkit
Component:
Video/Audio Controls
Version:
53 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox50 --- unaffected
firefox51 --- unaffected
firefox52 --- unaffected
firefox-esr52 --- unaffected
firefox57 --- wontfix
firefox58 --- wontfix
firefox59 --- fixed

People

(Reporter: arni2033, Assigned: timdream)

References

Details

(Keywords: regression)

Attachments

(4 files, 1 obsolete file)

Bug 1327097 - Part II, Allow a11y test harness to listen event on the DOM element
59 bytes, text/x-review-board-request
surkov
: review+
Details
Bug 1327097, Part I, Revert bug 1338961,
59 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1327097 - Part III, Trap mouse/touch/pointer events in audio/video element,
59 bytes, text/x-review-board-request
smaug
: review+
Details
Video controls.png
487.78 KB, image/png
Details
>>> My Info: Win7_64, Nightly 53, 32bit, ID 20161119030204 (2016-11-19) STR_1: (testcase) 1. Open https://www.iandevlin.com/html5test/webvtt/html5-video-webvtt-sample.html 2. Add attribute [onmousedown="return false"] to <body> 3. Click on the middle of timeline in video controls AR: No visible action ER: Video should seek to the middle in a normal way, just like on GoogleChrome STR_2: (original) 1. Open any video in comment on funnyjunk.com 2. Click on the middle of timeline in video controls AR: No visible action ER: Video should seek to the middle, just like on GoogleChrome STR_3: (testcase) 1. Open https://www.iandevlin.com/html5test/webvtt/html5-video-webvtt-sample.html 2. Add attribute [onmouseup="return false"] to <body> 3. Click on the middle of timeline in video controls 4. Move mouse a bit AR: Step 3 - video seeks to the middle. Step 4 - video goes back to the same place in timeline where it was before Step 3 ER: Video should seek to the middle in a normal way, just like on GoogleChrome
Component: Untriaged → Video/Audio Controls
Product: Firefox → Toolkit
No longer blocks: 1277113
I have managed to reproduce this issue following the steps from STR_1, below is the result: Narrowed inbound regression window from [28e2a6dd, 6186126f] (3 revisions) to [46127b3a, 6186126f] (2 revisions) (~1 steps left) Oh noes, no (more) inbound revisions :( Last good revision: 46127b3a981bceb0413c8199849f4e47afc949da First bad revision: 6186126f502ba47e4fb2b6f4d971ea6fd3e66a02 Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=46127b3a981bceb0413c8199849f4e47afc949da&tochange=6186126f502ba47e4fb2b6f4d971ea6fd3e66a02
status-firefox50: --- → unaffected
status-firefox51: --- → unaffected
status-firefox52: --- → unaffected
status-firefox53: --- → affected
Keywords: regressionwindow-wanted
Version: Trunk → 53 Branch
I did some tests on slider(input[type="range"]) and noticed that not only the slider inside video control is broken, but actually all input[type="range"] are affected. As we converted all xul element to HTML in bug 1271765, it is expectable that slider suffered from the same issue. Jared, do you know who are the right person to this problem? Thanks :)
Flags: needinfo?(jaws)
Redirecting to :stone as that is who helped me with bug 1295719.
Flags: needinfo?(jaws) → needinfo?(sshih)
Assignee: nobody → sshih
Flags: needinfo?(sshih)
Return false in mousedown's listener stops the default behavior [1]. That stops updating the value of range element and no input event is fired. The listener in [2] is not triggered either. Tested on Edge Microsoft Edge 38.14393.0.0 return false doesn't stop dragging and seeking video. call preventDefault stops dragging and seeking video. Tested on Chrome 57.0.2975.0 return false doesn't stop dragging and seeking video. call preventDefault doesn't stop dragging and seeking video. [1] http://searchfox.org/mozilla-central/rev/0f254a30d684796bcc8b6e2a102a0095d25842bb/dom/events/JSEventHandler.cpp#224 [2] https://hg.mozilla.org/mozilla-central/rev/083604641e50#l2.1090
Corrected testing results on Edge Tested on Microsoft Edge 38.14393.0.0 UA fires mousedown event to body element when clicking on the video control Added attribute onmousedown="return false" to body element stops dragging and seeking video. call preventDefault stops dragging and seeking video. Tested on Safari 9.1.1 (11601.6.17) behaves the same as Edge Tested on Firefox 53.0a1 (2017-01-09) (64-bit) behaves the same as Edge Tested on Chrome 57.0.2975.0 UA doesn't fire mousedown event to body element when clicking on the video control Added attribute onmousedown="return false" to body element doesn't stop dragging and seeking video. call preventDefault doesn't stop dragging and seeking video.
Tested with audio control (http://www.html5tutorial.info/html5-audio.php) Tested on Microsoft Edge 38.14393.0.0 Click on audio progress bar fires mousedown Added attribute onmousedown="return false" stops dragging and seeking audio progress. Call preventDefault stops dragging and seeking audio progress. Click on audio volume bar fires mousedown Added attribute onmousedown="return false" stops dragging audio volume bar. Call preventDefault stops dragging audio volume bar. Tested on Safari 9.1.1 (11601.6.17) No audio progress control Audio volume behaves the same as Edge Tested on Firefox 53.0a1 (2017-01-09) (64-bit) Doesn't fire mousedown Drags audio progress and volume bar Tested on Chrome 57.0.2975.0 Behaves the same as Firefox
Tested with <select> tag (http://examples.quackit.com/preview/html_editor_result.cfm?contentFile=/html_5/tags/_inc/inc_html_select_tag.cfm) Tested on Microsoft Edge 38.14393.0.0 Fires mousedown PreventDefault or add attribute onmousedown="return false" stop its behavior Tested on Safari 9.1.1 (11601.6.17) Behaves the same as Edge Tested on Firefox 53.0a1 (2017-01-09) (64-bit) Behaves the same as Edge Tested on Chrome 57.0.2975.0 Behaves the same as Edge
In summary Video Control Audio Control Select Edge 1 1 1 Safari 1 1 1 Firefox 1 2 1 Chrome 2 2 1 1: fires mousedown event, prevent default or add attribute onmousedown="return false" stops its action 2: doesn't fire mousedown event, prevent default and add attribute onmousedown "return false" doesn't stop its action
What you mean with 2, "doesn't fire mousedown event"? Do you not get event at all, or not in bubble phase or what?
That particular code would be consistent with the rest of the videocontrol handling if { mozSystemGroup: true } was passed to addEventListener as 3rd param.
The audio testing results are updated because I misused a website with iframe. Video Control Audio Control Select Edge 1 1 1 Safari 1 1 1 Firefox 1 1 1 Chrome 2 2 1 1: fires mousedown event, prevent default or add attribute onmousedown="return false" stops its action 2: doesn't fire mousedown event (checked via listeners in capturing and bubbling phase), prevent default and add attribute onmousedown "return false" doesn't stop its action Note: Edge stops the actions of all controls when click on it with preventDefault is called. Safari only stops progress seeking and volume adjustment. Play/suspend and mute/unmute is still working. Firefox only stops progress seeking and volume adjustment. Play/suspend and mute/unmute is still working.
On Firefox, preventDefault on click can stop play/resume control's behavior. Prevent default on mousedown, mouseup, click doesn't stop mute/unmute control's behavior.
Thanks for looking into this bug, :stone. As we discussed offline, another slider issue(bug 1327238) might related to input[type="range"] as well.
(In reply to Olli Pettay [:smaug] from comment #10) > FWIW, > http://searchfox.org/mozilla-central/rev/ > 225ab0637ed51b8b3f9f4ee2f9c339a37a65b626/toolkit/content/widgets/ > videocontrols.xml#1454-1455 is rather suspicious looking code. Why is that > calling stopPropagation. The stopPropagation is for texttrack button which overlay video content area. It's intended to avoid triggering video play/pause while the button being selecting.
Ray, could the listener use { mozSystemGroup: true } and then possibly preventDefault()? { mozSystemGroup: true } makes this not visible to the web content.
Hi Olli, I think the listener of slider uses mozSystemGroup already[0]. IIRC, mozSystemGroup has its own procedure rather than web content event, so I had a hard time figure out the reason how slider being affected. Let me try preventDefault(), and I'll update the result soon later :) Thanks. [0] https://dxr.mozilla.org/mozilla-central/rev/13603af3862d9583ed2feefb06e0988c2d7fed8c/toolkit/content/widgets/videocontrols.xml#1720-1724,1746-1747
The relevant code I linked doesn't use mozSystemGroup, but it probably should.
Sorry for the late reply. yeh, all event listeners in anonymous content should use `mozSystemGroup`, thanks. ---- However, the problem is still, and I wonder is this behavior happened on typical websites or not? It seems likely a sort of bad intention by website author, and I don't see an urgent necessary to fix(support) it right away. Also, per survey by :stone, our implementation is aligned with Safari and even a tiny bit better than Edge. Should we consider removing this bug from blocker list of bug 1271765? Hani, what do you think about this? Thanks.
Flags: needinfo?(hani.yacoub)
I think it's OK not to be fixed in Nightly and focus on the bugs that have higher impact on this feature, but this bug should be fixed in aurora.
Flags: needinfo?(hani.yacoub)
If you land a fix after next week's merge, could you be sure to request uplift to aurora? Thanks.
tracking-firefox53: --- → +
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #22) > If you land a fix after next week's merge, could you be sure to request > uplift to aurora? Thanks. Sorry for replying late. We behave the same as Edge and Safari. I think it's hard to say current behavior is incorrect at this moment. There is a spec issue fired in [1] to be discussed but I have no idea sure whether we can get the conclusion soon. Please ni me if need other actions or information. Thanks. [1] https://github.com/whatwg/html/issues/2258
Flags: needinfo?(lhenry)
OK. Thanks! Hani, by "should be fixed in aurora" do you mean it is fixed already in aurora 52, or that it still needs to be fixed in 53 once 53 moves to aurora? I realize on second look that your meaning is unclear.
Flags: needinfo?(lhenry) → needinfo?(hani.yacoub)
What I meant was that it's still need to be fix in Firefox Nightly 54 and then uplifted to Aurora 53.
Flags: needinfo?(hani.yacoub)
Any word on a fix here, or a resolution of the spec issue?
Flags: needinfo?(sshih)
This is still considered as a bug after discussions with ralin, timdream, and chsiang. This bug will be handover to ralin and he plans to create another bug to request the flexibility to prevent the default behavior of anonymous elements be impacted by the content. Maybe ralin could update more details about next action.
Flags: needinfo?(sshih) → needinfo?(ralin)
Depends on: 1338961
After lengthy offline discussion, we have a proposal to fix this in bug 1338961. This bug has nothing to do with mozSystemGroup, just to be clear.
Flags: needinfo?(ralin)
We will need to look at bug 1338961 to see if we could get this fixed in fx53. It's unfortunate, but we will not be able to fix this in JS without that feature. Without waiting for a resolution from the spec, the assumption in Fx Front-end and Product is that we should try to keep the original behavior (i.e. fix this regression) whatever the underlining markup we are using to construct the UI.
Assignee: sshih → ralin
Status: NEW → ASSIGNED
tracking-firefox53: + → ?
So while discussing bug 1338961, I checked the Chrome video control implementation. Their video control is hosted inside a user agent shadow DOM. It seems that they avoid this bug by never dispatch the event interacting with video controls (<div pseudo="-webkit-media-controls-enclosure">) to the document. No-op mousedown events on the non-control part of the video element does get dispatched, though. Our event dispatcher does not allow XBL script to do that, since system event group receives the event *after* capturing and bubbling phase ( http://searchfox.org/mozilla-central/rev/ac8a72f2d5204ced2004c93a9c193430c63a6a71/dom/events/EventDispatcher.cpp#419-529 ). That's something we could consider supporting in order not to implement the hack in bug 1338961.
Chrome's behavior sounds like a bug to me. Why is it doing that? Rick, perhaps you know, or you know who to ask?
Flags: needinfo?(rbyers)
You don't even see the mousedown event on the document in the capture phase? That's not what I'm seeing in a quick test on youtube.com - mousedown on the slider and I see the mousedown reach the document both during capture and bubbling phases. Regardless mlamouri@chromium.org is the expert, and he's told me our DOM events system really should adopt Gecko's mechanism for dealing with internal controls (though I can't find that thread right now - was there a bug?).
Flags: needinfo?(rbyers) → needinfo?(mounir)
(In reply to Rick Byers from comment #32) > You don't even see the mousedown event on the document in the capture phase? > That's not what I'm seeing in a quick test on youtube.com - mousedown on the > slider and I see the mousedown reach the document both during capture and > bubbling phases. We are talking about native video control here. YouTube implements it's own control so you won't see the behavior here. There is my test: 1. Go to http://camendesign.com/code/video_for_everybody/test.html 2. Open up DevTools, inspect the <body> element and do the following $0.addEventListener('mousedown', (evt) => console.log(evt, 'capture'), true) $0.addEventListener('mousedown', (evt) => console.log(evt, 'bubble')) 3. Click on the video control (play/pause button, or seeker, anything). You can verify DevTools works by click on other part of the page, or the video itself.
(In reply to Rick Byers from comment #32) > You don't even see the mousedown event on the document in the capture phase? > That's not what I'm seeing in a quick test on youtube.com - mousedown on the > slider and I see the mousedown reach the document both during capture and > bubbling phases. > > Regardless mlamouri@chromium.org is the expert, and he's told me our DOM > events system really should adopt Gecko's mechanism for dealing with > internal controls (though I can't find that thread right now - was there a > bug?). I'm not sure who is that mlamouri@chromium.org but I'm no expert :) Media controls in Blink are a nice mess and because we don't have a system group like you have in Gecko, we do weird things in order to make sure we receive the events. In this case, I think the patch that created this weird behaviour is https://codereview.chromium.org/406213002 which is linked to these two bugs: https://bugs.chromium.org/p/chromium/issues/detail?id=388738 https://bugs.chromium.org/p/chromium/issues/detail?id=269454 It seems that one of the reasons why this change was created is https://github.com/whatwg/html/commit/ef57900202d092434b3e4bfef29481e7767d4a1f which suggests that UA should not propagate events from the native controls to the page. At least, that's my understand.
Flags: needinfo?(mounir)
wow, that spec is just buggy, super vague. Is mousemove user interaction event? Anyhow, if we want to mimic not-dispatching-events-at-all, we could easily just stop propagation in to <video> and <audio> by having HTMLMediaElement::GetEventTargetParent method and set aVisitor.mCanHandle = false; there. Probably need to do that only for some events (which ones? The spec is totally unclear here), and only trusted events, and only when "native" controls are used.
I can try to deliver a fix proposed in comment 35, but probably not in the fx53 cycle. Let's land the localized hack in bug 1338961 first...
Assignee: ralin → timdream
.(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #37) > nit: not the entire <video> or <audio> element, just the video control > within it. It would be a bit hard to figure out that part of the UI in > native code (the binding host <xul:videocontrol> also occupied the full > video area). ... and this is not correct because Firefox do respond to mouse click on non-control area of the <video> frame. This means by definition of the spec, the "user interface exposed to the user" occupied the entire <video> frame and we must not fire any events of mouse/touch/keyboard from it. I wonder if this is acceptable...
Note that Blink is looking into implementing keyboard controls on the video element and that will make the problem similar to what you have in Gecko.
My current patch implements what's said in comment 35 but disregards event dispatching of keyboard event. The same approach won't work with keyboard event because the binding also listens for them on the <video> itself -- setting mCanHandle on the element will break the video control too. I think this is right balance because this is how Firefox used to work with XUL video control (allow content to listen to events but ignore it's defaultPrevented flag). Obviously spec can be more clearer about key events (see comment 42) and we can address that on another bug when it does.
Comment on attachment 8846508 [details] Bug 1327097 - Part II, Allow a11y test harness to listen event on the DOM element https://reviewboard.mozilla.org/r/119570/#review121484 I guess the other patch explains why we can revert this :)
Attachment #8846508 - Flags: review?(bugs) → review+
Comment on attachment 8846509 [details] Bug 1327097 - Part II, Trap mouse/touch/pointer events in audio/video element, https://reviewboard.mozilla.org/r/119572/#review121486 ::: dom/html/HTMLMediaElement.cpp:4078 (Diff revision 3) > } > > +nsresult > +HTMLMediaElement::GetEventTargetParent(EventChainPreVisitor& aVisitor) > +{ > + RefPtr<HTMLMediaElement> self(this); Why do you need this RefPtr? It just slows down this possibly hot code. ::: dom/html/HTMLMediaElement.cpp:4081 (Diff revision 3) > +HTMLMediaElement::GetEventTargetParent(EventChainPreVisitor& aVisitor) > +{ > + RefPtr<HTMLMediaElement> self(this); > + > + if (!self->Controls()) { > + aVisitor.mCanHandle = true; No need to set mCanHandle = true, since the parent classes will do that. ::: dom/html/HTMLMediaElement.cpp:4086 (Diff revision 3) > + aVisitor.mCanHandle = true; > + return nsGenericHTMLElement::GetEventTargetParent(aVisitor); > + } > + > + if (!aVisitor.mEvent->mFlags.mIsTrusted) { > + aVisitor.mCanHandle = true; ditto. And I would just merge the two ifs. if (!Controls() || !aVisitor.mEvent->mFlags.mIsTrusted) { return return nsGenericHTMLElement::GetEventTargetParent(aVisitor); } ::: dom/html/HTMLMediaElement.cpp:4092 (Diff revision 3) > + return nsGenericHTMLElement::GetEventTargetParent(aVisitor); > + } > + > + // We will need to trap mouse events within the media element, > + // preventing the content from handling them. > + switch (aVisitor.mEvent->mMessage) { Do you need to handle also pointer events here? What does blink do with those? It would be odd to get pointerdown/up, but not mousedown/up. What about touch events? What happens if one does mousedown on some other element and then mouseup on the video? How does blink behave then? (you may need to call preventDefault on the mousedown or mousemove to prevent drag session to be initialized) ::: dom/html/HTMLMediaElement.cpp:4100 (Diff revision 3) > + case eMouseDoubleClick: > + case eMouseUp: > + aVisitor.mCanHandle = false; > + return NS_OK; > + default: > + aVisitor.mCanHandle = true; No need to set mCanHandle to true
Attachment #8846509 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] from comment #49) > ::: dom/html/HTMLMediaElement.cpp:4092 > (Diff revision 3) > > + return nsGenericHTMLElement::GetEventTargetParent(aVisitor); > > + } > > + > > + // We will need to trap mouse events within the media element, > > + // preventing the content from handling them. > > + switch (aVisitor.mEvent->mMessage) { > > Do you need to handle also pointer events here? > What does blink do with those? > It would be odd to get pointerdown/up, but not mousedown/up. > What about touch events? > Blink contains touch, keyboard, and mouse events and does not handle pointer events: https://codereview.chromium.org/406213002/patch/120001/130009 https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/shadow/MediaControlElements.cpp?l=80&rcl=a15fea4b2a0062f98fa2c7b446dfa4035e7231c9 Does call preventDefault on pointer event also prevent the touch/mouse event from firing? Will test tomorrow. > What happens if one does mousedown on some other element and then mouseup on > the video? > How does blink behave then? > (you may need to call preventDefault on the mousedown or mousemove to > prevent drag session to be initialized) Interesting. Will test this out tomorrow too.
The code for slider seems to dealing with pointerevents these days.
Hi Yura, Quick question for you. My patch here will contain click/mouse/touch/pointer events into the <video> element itself. The change breaks ally test on media controls [1] because it binds the event listener on the outer document [2], not the element itself. [1] https://treeherder.mozilla.org/#/jobs?repo=try&author=timdream@gmail.com&selectedJob=83338794 [2] http://searchfox.org/mozilla-central/rev/a5c2b278897272497e14a8481513fee34bbc7e2c/accessible/tests/mochitest/events.js#782 Can I change the object in eventSeq (created by checkerOfActionInvoker()) such that events.js will bind the listener on the DOM node itself on this case? I will ask you for review for that. Thanks.
Flags: needinfo?(yzenevich)
Comment on attachment 8846509 [details] Bug 1327097 - Part II, Trap mouse/touch/pointer events in audio/video element, https://reviewboard.mozilla.org/r/119572/#review121486 > Do you need to handle also pointer events here? > What does blink do with those? > It would be odd to get pointerdown/up, but not mousedown/up. > What about touch events? > > What happens if one does mousedown on some other element and then mouseup on the video? > How does blink behave then? > (you may need to call preventDefault on the mousedown or mousemove to prevent drag session to be initialized) The new patch now handles pointer & touch events, because they can also be used to cancel slider drag per my tests. If the user does mousedown on some other element and then mouseup on the video, the document will only hear mousedown. This is consistent with what Blink is doing and just like if the user mousedown on the document and mouseup outside of the window.
Blink's behavior is horrible. Can we do anything better. Mounir, you might know why blink is doing what it is doing. Why it even initially started to have that not-compatible-with-the-rest-of-the-DOM behavior?
Flags: needinfo?(mounir)
Not sure what the question is exactly. If this is the general behaviour, I believe I gave some links above and that's the only thing I know. I can dig deeper if it can help. If the question is about the pointer events, it's a bug and we should handle them. Feel free to ping me on IRC if it can help.
Flags: needinfo?(mounir)
Blink doesn't seem to trap mousemoves when over video, if one isn't dragging some slider. I think we definitely want at least that behavior, so that not all mousemoves are broken on top of video.
Comment on attachment 8846509 [details] Bug 1327097 - Part II, Trap mouse/touch/pointer events in audio/video element, https://reviewboard.mozilla.org/r/119572/#review121996 See the previous comment in bugzilla. I think we need different handling for *move events. Also, does blink trap all these events even when on top of the video area, or only when on top of control area?
Attachment #8846509 - Flags: review?(bugs) → review-
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #52) > Hi Yura, > > Quick question for you. My patch here will contain click/mouse/touch/pointer > events into the <video> element itself. The change breaks ally test on media > controls [1] because it binds the event listener on the outer document [2], > not the element itself. > > [1] > https://treeherder.mozilla.org/#/jobs?repo=try&author=timdream@gmail. > com&selectedJob=83338794 > [2] > http://searchfox.org/mozilla-central/rev/ > a5c2b278897272497e14a8481513fee34bbc7e2c/accessible/tests/mochitest/events. > js#782 > > Can I change the object in eventSeq (created by checkerOfActionInvoker()) > such that events.js will bind the listener on the DOM node itself on this > case? I will ask you for review for that. Thanks. Moving this to Alex as he has more context about the mochitests and their harness.
Flags: needinfo?(yzenevich) → needinfo?(surkov.alexander)
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #52) > Hi Yura, > > Quick question for you. My patch here will contain click/mouse/touch/pointer > events into the <video> element itself. Could you elaborate please, I'm not following what the change is. > The change breaks ally test on media > controls [1] because it binds the event listener on the outer document [2], > not the element itself. events.js adds an event listener on a DOM document of a DOM node. What is the outer document you refer to? > Can I change the object in eventSeq (created by checkerOfActionInvoker()) > such that events.js will bind the listener on the DOM node itself on this > case? Listening an event right on a DOM node will probably work overall. I didn't catch though how exactly you want to handle this case separately from all cases.
Flags: needinfo?(surkov.alexander)
Attachment #8846509 - Attachment is obsolete: true
Oops, somehow pushing only the 3rd part will mark the first two parts obsolete. MozReview is fun. (I guess that won't happen if there is also a MozReview-Commit-ID on Part I) Anyway, I will need sometime to figure out how I can achieve comment 57. It's a bit over my C++ skill (which is no more than copy-pasting I guess). Will get some crash courses in the office ...
Comment on attachment 8846508 [details] Bug 1327097 - Part II, Allow a11y test harness to listen event on the DOM element https://reviewboard.mozilla.org/r/119570/#review122536 the patch looks good, I'm not quite following though, why those clicks events cannot be captured or don't bubble up to their document? r=me with the question answered
Attachment #8846508 - Flags: review?(surkov.alexander) → review+
(In reply to alexander :surkov from comment #63) > the patch looks good, I'm not quite following though, why those clicks > events cannot be captured or don't bubble up to their document? r=me with > the question answered See comment 35. We will need to contain these events in the subtree of the video element (simply, the DOM of video control HTML UIs).
(Nothing changed; just restoring the old reviewed commits and upload my broken WIP from 8 months ago)
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #68) > (Nothing changed; just restoring the old reviewed commits and upload my > broken WIP from 8 months ago) Appreciate your help :D (I've been struggling with those event handling including recent regression...)
Comment on attachment 8925819 [details] Bug 1327097, Part I, Revert bug 1338961, https://reviewboard.mozilla.org/r/197006/#review204022 This has been reviewed before in bug 1327097 comment 48 but the flag was lost. Would you mind set it again? Thanks.
Comment on attachment 8925820 [details] Bug 1327097 - Part III, Trap mouse/touch/pointer events in audio/video element, This patch finally does what comment 57 asks, only block mousemove events when the range input is being dragged. I have to manage to QueryInterface my way from EventTargets to nsIDOMHTMLInputElement to check the drag state. I am not sure I am using the best approach here (and I am told nsI* is going away), so please let me know if there are better approaches.
Attachment #8925820 - Flags: feedback?(bugs)
Comment on attachment 8925820 [details] Bug 1327097 - Part III, Trap mouse/touch/pointer events in audio/video element, https://reviewboard.mozilla.org/r/197008/#review204150 ::: dom/html/HTMLInputElement.cpp:1566 (Diff revision 2) > NS_IMPL_ACTION_ATTR(HTMLInputElement, FormAction, formaction) > NS_IMPL_STRING_ATTR(HTMLInputElement, Name, name) > NS_IMPL_BOOL_ATTR(HTMLInputElement, ReadOnly, readonly) > > NS_IMETHODIMP > +HTMLInputElement::GetIsDraggingRange(bool* aValue) { Nit, { to its own line ::: dom/html/HTMLMediaElement.cpp:4416 (Diff revision 2) > + case eTouchStart: > + case eMouseClick: > + case eMouseDoubleClick: > + case eMouseDown: > + case eMouseUp: > + aVisitor.mCanHandle = false; really? What if mousedown happened on a different element and mouseup then here. What do other browsers do in such case? ::: dom/html/HTMLMediaElement.cpp:4431 (Diff revision 2) > + case eMouseOver: > + el = do_QueryInterface(aVisitor.mEvent->mOriginalTarget); > + if (!el) { > + // See if the node is children of <input range> > + node = do_QueryInterface(aVisitor.mEvent->mOriginalTarget); > + el = do_QueryInterface(node->GetParentNode()); Why not use nsINode::IsHTMLElement and then static_cast to HTMLInputElement. We'd like to get rid of nsIDOM*Element interfaces.
Comment on attachment 8925820 [details] Bug 1327097 - Part III, Trap mouse/touch/pointer events in audio/video element, Looks possible solution, assuming other browsers do this too.
Attachment #8925820 - Flags: feedback?(bugs) → feedback+
Comment on attachment 8925820 [details] Bug 1327097 - Part III, Trap mouse/touch/pointer events in audio/video element, https://reviewboard.mozilla.org/r/197008/#review204150 > really? What if mousedown happened on a different element and mouseup then here. What do other browsers do in such case? As explained in comment 54, the document will only hear mousedown in your example. This is in consistent of what Blink does, though their video control does not cover the entire video element.
Comment on attachment 8925820 [details] Bug 1327097 - Part III, Trap mouse/touch/pointer events in audio/video element, https://reviewboard.mozilla.org/r/197008/#review204512 ::: dom/html/HTMLMediaElement.cpp:4421 (Diff revision 4) > + aVisitor.mCanHandle = false; > + return NS_OK; > + > + // The *move events however are only comsumed when the range input is being > + // dragged. > + case ePointerMove: What happens to mouse/pointerenter/leave events? Remember that separate events are dispatched to the ancestors. How does that all work in blink? ::: dom/html/HTMLMediaElement.cpp:4427 (Diff revision 4) > + case ePointerOut: > + case ePointerOver: > + case eMouseMove: > + case eMouseOut: > + case eMouseOver: > + node = do_QueryInterface(aVisitor.mEvent->mOriginalTarget); First check that node is in native anonymous content ::: dom/html/HTMLMediaElement.cpp:4431 (Diff revision 4) > + case eMouseOver: > + node = do_QueryInterface(aVisitor.mEvent->mOriginalTarget); > + if (node->IsHTMLElement(nsGkAtoms::input)) { > + // The node is a <input type="range"> > + el = static_cast<HTMLInputElement*>(node.get()); > + } else if (node->GetParentNode()->IsHTMLElement(nsGkAtoms::input)) { Null check GetParentNode
Attachment #8925820 - Flags: review?(bugs) → review-
Comment on attachment 8925820 [details] Bug 1327097 - Part III, Trap mouse/touch/pointer events in audio/video element, https://reviewboard.mozilla.org/r/197008/#review204512 > What happens to mouse/pointerenter/leave events? > Remember that separate events are dispatched to the ancestors. How does that all work in blink? I was wrong about mouse{enter/leave/over/out} events. Thank you for catching that. They work as if `<video>` is one element without any children on Blink, and we do that already without any special handling here. I have removed them from switch case.
Did you check enter/leave handling by adding capturing listeners, not bubbling? I mean, adding listeners to all the ancestor nodes of a <video> separately.
I guess that all works normally in blink if out/over work too
Comment on attachment 8925820 [details] Bug 1327097 - Part III, Trap mouse/touch/pointer events in audio/video element, https://reviewboard.mozilla.org/r/197008/#review204866 ::: dom/html/HTMLMediaElement.cpp:4410 (Diff revision 5) > + // and preventing the content from handling them. > + switch (aVisitor.mEvent->mMessage) { > + case ePointerDown: > + case ePointerUp: > + case eTouchEnd: > + case eTouchMove: Really, eTouchMove would be blocked always? ::: dom/html/HTMLMediaElement.cpp:4414 (Diff revision 5) > + case eTouchEnd: > + case eTouchMove: > + case eTouchStart: > + case eMouseClick: > + case eMouseDoubleClick: > + case eMouseDown: So this is not the behavior I get on Chrome. If there is listener for mousedown and I click on video, mousedown fires normally. Only when I click on top of the controls there is no mousedown. ::: dom/html/HTMLMediaElement.cpp:4428 (Diff revision 5) > + node = do_QueryInterface(aVisitor.mEvent->mOriginalTarget); > + if (node->IsInNativeAnonymousSubtree()) { > + if (node->IsHTMLElement(nsGkAtoms::input)) { > + // The node is a <input type="range"> > + el = static_cast<HTMLInputElement*>(node.get()); > + } else if (node->GetParentNode() && Is there extra space there
Attachment #8925820 - Flags: review?(bugs) → review-
Comment on attachment 8925820 [details] Bug 1327097 - Part III, Trap mouse/touch/pointer events in audio/video element, https://reviewboard.mozilla.org/r/197008/#review204866 > Really, eTouchMove would be blocked always? The touchmove event will be captured on the video element. Given that we will always block touchstart here, it doesn't make sense to only dispatch touchmove sometimes - document will hear touchmove without hearing touchstart. > So this is not the behavior I get on Chrome. > If there is listener for mousedown and I click on video, mousedown fires normally. Only when I click on top of the controls there is no mousedown. As I explained in comment 54 and again in comment 79, our video control covers the entire video element while their controls only occupies the small strip at the bottom. Video play/pause if the user click the area outside of the visible control, and full screen toggles if the user double clicks. If you disagree such design result compat issue, I can loop UX designer and we can talk.
Attached image Video controls.png —
In case I didn't make myself clear in English, I made this graphic to explain the difference b/t our controls and their controls. Let me know if you agree with the assessment or not, or if we want to change the behavior of the control. If been unproductive for us to exchange one message per day over Bugzilla, so please let's set a baseline expectation here and I can refine the switch cases definitely. Thanks.
Flags: needinfo?(bugs)
Comment on attachment 8925820 [details] Bug 1327097 - Part III, Trap mouse/touch/pointer events in audio/video element, https://reviewboard.mozilla.org/r/197008/#review205222 ::: dom/html/HTMLMediaElement.cpp:4405 (Diff revision 6) > + // and preventing the content from handling them. > + switch (aVisitor.mEvent->mMessage) { > + case ePointerDown: > + case ePointerUp: > + case eTouchEnd: > + case eTouchMove: Could you add a comment why touchmove is handled differently to mousemove and pointermove. Basically something what you explained in comment 88.
Attachment #8925820 - Flags: review?(bugs) → review+
Flags: needinfo?(bugs)
Pushed by timdream@gmail.com: https://hg.mozilla.org/integration/autoland/rev/82a6e4dc5da7 Part I, Revert bug 1338961, r=smaug https://hg.mozilla.org/integration/autoland/rev/c0c5ee67b3ef Part II, Allow a11y test harness to listen event on the DOM element r=smaug,surkov https://hg.mozilla.org/integration/autoland/rev/09c0a4c56c78 Part III, Trap mouse/touch/pointer events in audio/video element, r=smaug
Backout by rgurzau@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/2fab4b8b8f7c Backed out 3 changesets for eslint failures toolkit/content/tests/widgets/test_videocontrols.html:228 r=backout on a CLOSED TREE
Backed out 3 changesets (bug 1327097) for eslint failures toolkit/content/tests/widgets/test_videocontrols.html:228 r=backout on a CLOSED TREE Backout: https://hg.mozilla.org/integration/autoland/rev/2fab4b8b8f7c8e56d200d04801c9e015086a541d Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=09c0a4c56c78e66f3584e765e2a5cf816fb423cf Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=145267999&repo=autoland&lineNumber=238 [task 2017-11-16T10:25:42.421Z] Error processing command. Ignoring because optional. (optional:packages.txt:comm/build/virtualenv_packages.txt) [task 2017-11-16T10:33:18.782Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/toolkit/content/tests/widgets/test_videocontrols.html:228:4 | Unnecessary semicolon. (no-extra-semi) [taskcluster 2017-11-16 10:33:19.503Z] === Task Finished === [taskcluster 2017-11-16 10:33:19.504Z] Unsuccessful task run with exit code: 1 completed in 696.605 seconds
Flags: needinfo?(timdream)
This is embarassing... thanks for backing that out. Anyway, let's verify this on Try again before landing. https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab10890717c4d5e4e7c1e0544414e7cc375d47ff
Flags: needinfo?(timdream)
Pushed by timdream@gmail.com: https://hg.mozilla.org/integration/autoland/rev/400bffaca364 Part I, Revert bug 1338961, r=smaug https://hg.mozilla.org/integration/autoland/rev/45520c6ace4e Part II, Allow a11y test harness to listen event on the DOM element r=smaug,surkov https://hg.mozilla.org/integration/autoland/rev/194be07f3d4b Part III, Trap mouse/touch/pointer events in audio/video element, r=smaug
https://hg.mozilla.org/mozilla-central/rev/45520c6ace4e https://hg.mozilla.org/mozilla-central/rev/194be07f3d4b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Can this ride the 59 train or should we request Beta approval on it for 58?
status-firefox57: --- → wontfix
status-firefox58: --- → affected
status-firefox-esr52: --- → unaffected
Flags: needinfo?(timdream)
Let's ride the train. Thanks.
status-firefox58: affected → wontfix
Flags: needinfo?(timdream)
Please don't remove the ability to listen for events on video elements with controls. This will break a lot of sites and browser extensions, in particular ones that use clicking or dragging on videos to close videos opened inline. The alternative is to add a small button or link to close a video, which is much less convenient for users, or to add a lot of complexity by implementing controls in Javascript. A video element takes up a large amount of screen real estate, and developers should be able to interact with this space. Some examples. (warning: I've tried to avoid linking to anything pornographic, but these are sites with user submitted content so such things could be added at any time.) 1. https://meguca.org/cr/2896968#p3357938 Clicking the thumbnail opens the video; clicking it again closes the video. Interacting with the controls doesn't close the video because they're ignoring clicks in a small rectangular strip at the bottom. Not firing events on the control bar would be beneficial here as it would no longer have to guess the size of the bar. Not firing events anywhere on the video is a breaking change. 2. https://8ch.net/tg/res/352415.html This is similar. Clicking the first thumbnail opens a video. Here there is a small button to close the video, but you can also close the video by dragging it to the left, which is admittedly not obvious, but much more convenient than the little button if you're aware of it. Not firing events on videos with controls breaks this feature. There are a number of boards using the same or similar board software as (1) and (2) which are likely to be affected: https://pastebin.com/L8gndqMk For my part, 4chan X, a userscript I maintain which adds various enhancements to 4chan.org, will be affected, although 4chan itself will not be affected because the unmodified site uses a small link to close videos.
I also don't think it's clear this change is what's intended by the spec. >If the user agent exposes a user interface to the user by displaying controls over the media element, then the user agent should suppress any user interaction events while the user agent is interacting with this interface. Clicking the media element to play/pause is a user interface, but it's not an interface exposed by displaying controls over the media element.
I just wanted to add that with the recent change that eliminated the capture phase mouse events on video elements with controls, it has become impossible to execute mouse gestures on such video elements. I'm definitely in favor of keeping the events when the mouse is not explicity over the controls bar.
Depends on: 1531774
See Also: → 1667125
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: