HTML5 <video controls> "click to play/pause" doesn't respect event.preventDefault standard

RESOLVED FIXED in mozilla12

Status

()

defect
RESOLVED FIXED
8 years ago
11 months ago

People

(Reporter: rwaldron, Assigned: dao)

Tracking

({html5})

Trunk
mozilla12
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [popcorn.js], )

Attachments

(1 attachment, 4 obsolete attachments)

No description provided.
Sorry,  this submitted before I was done (as a result of trying to avoid the suggested ticket box).

Demonstrated here: http://jsfiddle.net/rwaldron/8fYYZ/show/light

The new feature described here: http://msujaws.wordpress.com/2011/10/07/enhancements-for-html5-videos-in-firefox/

"Clicking on the video surface will toggle between play and pause*"

Works as expected, but cannot be prevented with preventDefault()
Keywords: html5
This _will_ prevent you from interacting with the normal controls
Component: General → Video/Audio
Product: Firefox → Core
QA Contact: general → video.audio
Severity: major → normal
OS: Mac OS X → All
Version: 10 Branch → Trunk
Jared, what do you think?
This feature is only available when the |controls| attribute is set. We currently don't allow users to configure which media controls are available to their users when |controls| is set, and this would be an precedent of enabling that control.

If |controls| is removed, then this issue goes away. We could spend time fixing this bug, but it doesn't seem trivial as the <video> element also includes the overlaid controls, and we would have to determine what native-anonymous content was the target to determine if |event.preventDefault| should be respected.

While I think the demonstrated behavior with event.preventDefault is unfortunate, a web developer opts in to the browsers controls and if they would not like the click to play/pause behavior, then they can simply remove the |controls| attribute.
Summary: Non-standard video play/pause on click cannot be prevented → HTML5 controls video play/pause on click cannot be prevented with event.preventDefault
So what you're saying is "too bad"? 

This response is entirely unacceptable. The behaviour that has been implemented is non-standard and will interfere with efforts to create programmable video experiences where both the controls AND clickable UI are valued, eg. ads embedded into video, clickable player regions... all of these become impossible. 

The standard controls can be prevented with preventDefault, so you're argument doesn't hold.
ahem "your"
Summary: HTML5 controls video play/pause on click cannot be prevented with event.preventDefault → Non-standard "click to play/pause" in video player cannot be prevented with event.preventDefault
(In reply to Rick Waldron from comment #5)
> So what you're saying is "too bad"? 
> 
> This response is entirely unacceptable. The behaviour that has been
> implemented is non-standard (...)
Not only it is non-standard, but both latest Chrome and Opera implement the standard behavior.
Either the standard has to change (which i would disagree with based on Rick's arguments) or Firefox implementation has to be fixed. All middle ground is unacceptable, I agree.

Maybe the bug is not trivial to fix, maybe it will take time (some bugs are over 10yo and still open), but the web devs expect at least Firefox to be willing to fix it, I think.
Summary: Non-standard "click to play/pause" in video player cannot be prevented with event.preventDefault → HTML5 controls video play/pause on click cannot be prevented with event.preventDefault
Sorry for the summury change. Our changes collapsed.
Summary: HTML5 controls video play/pause on click cannot be prevented with event.preventDefault → Non-standard "click to play/pause" in video player cannot be prevented with event.preventDefault
To clarify, this ticket does NOT pertain to the controls that appear when the |control| attribute is given, it is specifically targeted at the "new" feature in Nightly that allows clicking "anywhere" to play/pause a video
I understand why this is hard to fix. Can we consider disabling this feature for now?
Summary: Non-standard "click to play/pause" in video player cannot be prevented with event.preventDefault → Non-standard "click to play/pause" in video player cannot be prevented with event.preventDefault when |controls| is set
Upon further thinking I think we may be able to fix this by using a xul:button instead of a xul:spacer. I'll investigate this tomorrow.
First, a technical nit: there is no "standard" behavior for HTML5 media controls. In general, the HTML specs do not impose UI requirements on the UA. HTML5 media, in particular, is a new and tricky area.

Anyway...

What's your use-case for wanting to prevent the default action? IE, what are you actually trying to do? I mostly ask to better understand, but also because I wonder if there will still be problems with mobile UAs (eg, Fennec uses a tap (click) to bring up the default controls.

I think part of the issue, at least with the current code, is that our listener is being called before your event listener. I added some logging to our click handler to verify (and of course event.defaultPrevented is false).

I don't think this is directly avoidable -- the element our click handler is bound to is in the anonymous XBL content descending down from the <video> in the DOM. Both handlers are added as non-capturing handlers (3rd addEventListener() arg is |false|). We're on the actual (anonymous) target, and will see the bubbling event before the parent <video> does. I'm not sure if it's possible to add a "default" handler in JS. Hooking it up to a <xul:button>'s oncommand is the closest I can think of. XBL <handlers> can do some obscure |group="system"| stuff, but I don't think that's useful here. Might want to talk with Enn or one of the content guys about how to better handle this.

Another solution would be to (1) add a check to .defaultPrevented in our code, and (2) change your code to use a capturing listener for calling preventDefault(). I just did a quick test and this works fine.
(In reply to Justin Dolske [:Dolske] from comment #12)
> First, a technical nit: there is no "standard" behavior for HTML5 media
> controls. In general, the HTML specs do not impose UI requirements on the
> UA. HTML5 media, in particular, is a new and tricky area.
I think that the standard Rick refers to is DOM Events 3 which defines .preventDefault() (http://www.w3.org/TR/DOM-Level-3-Events/#events-event-type-preventDefault) "When this method is invoked, (...) any default actions normally taken by the implementation as a result of the event must not occur".
So, regardless of the UI requirements being standard or not, it should be canceled by preventDefault as long as the event is cancelable (which is the case for a click as in the use case).

If the bug cannot be fixed easily, one idea could be to temporarily set cancelable to "false" for this particular case (but no other case). It would be non-standard too, but webdevs would have a way to know that they cannot prevent the default behavior from JavaScript code.
Thanks for taking the time to review and respond. I feel like there is a misunderstanding of "what" part of the video player I'm talking about, so I've put together this illustration to clarify: http://i.imm.io/admC.png
(In reply to Justin Dolske [:Dolske] from comment #12)
> What's your use-case for wanting to prevent the default action? IE, what are
> you actually trying to do? 

As I noted in a previous comment, the use case centers around any functionality a developer might want to create that involves clicking _anywhere_ in the video player's display area. This may include, but is not limited to: clicking on a mapped region to expose additional information to the viewer or change the state of the displaying web page - the "use case" is largely immaterial.
(In reply to Rick Waldron from comment #15)
> (In reply to Justin Dolske [:Dolske] from comment #12)
> > What's your use-case for wanting to prevent the default action? IE, what are
> > you actually trying to do? 
> 
> As I noted in a previous comment, the use case centers around any
> functionality a developer might want to create that involves clicking
> _anywhere_ in the video player's display area. This may include, but is not
> limited to: clicking on a mapped region to expose additional information to
> the viewer or change the state of the displaying web page - the "use case"
> is largely immaterial.

(In reply to Rick Waldron from comment #5) 
> The standard controls can be prevented with preventDefault, so you're
> argument doesn't hold.

What's confusing to me is that even if this bug was fixed, using event.preventDefault on <video controls/> also removes functionality of the play/pause button and mute button, so the immersive case with clicking on mapped regions will also introduce broken playback controls.
The streams are getting crossed here...  The problem lies solely in the newly added feature, described here http://msujaws.wordpress.com/2011/10/07/enhancements-for-html5-videos-in-firefox/ as: "Clicking on the video surface will toggle between play and pause*". 

Currently, there is no way for this behaviour to be prevented unless |controls| is removed - this is the bug. I'm trying to find a compromise. If this can be prevented by calling preventDefault, then clicks on the video surface can be delegated. 


The alternative is to shut off the feature entirely, which would be unfortunate.



I only pointed out that event.preventDefault on <video controls/> worked correctly as a means of illustrating the newly added, broken behaviour.
I was just informed that the image i linked to earlier may not be working properly, please refer to this instead: http://gyazo.com/5440ece37979e39142247b8e13f8913c.png
Posted patch Patch for bug 693014 (obsolete) — Splinter Review
This patch switches the video controls to use xul:button instead of xul:spacer.

Marco: We already provide an accessible play/pause button, and this new button is not visible. Ideally, screen readers would actually not mention this new play/pause button because another one already exists. Can you confirm that with this patch there is only one play or pause button?
Assignee: nobody → jwein
Status: NEW → ASSIGNED
Attachment #566356 - Flags: review?(dolske)
Attachment #566356 - Flags: feedback?(marco.zehe)
After reading through https://bugzilla.mozilla.org/show_bug.cgi?id=518008 I'm left wondering why this was ever implemented at all? This kind of interaction with dom elements should be left to the end developer to implement if they desire the behaviour.
Comment on attachment 566356 [details] [diff] [review]
Patch for bug 693014

First, thank you very much for the ping!

Unfortunately, there is an extra button now exposed via the accessibility APIs. Looks like either it is not truly hidden by -moz-appearance: none; or it is still being exposed via the accessibility APIs because we don't specifically account for this style to filter out the accessible. Setting f- for now since the desired effect isn't there, however I'm also gonna talk to surkov and davidb about whether we should not create accessibles for XUL elements or anonymous XBL elements that have the -moz-appearance: none; style.
Attachment #566356 - Flags: feedback?(marco.zehe) → feedback-
davidb, surkov, please see my comment/question in comment #21.
(In reply to Marco Zehe (:MarcoZ) from comment #21)
> I'm also gonna talk to
> surkov and davidb about whether we should not create accessibles for XUL
> elements or anonymous XBL elements that have the -moz-appearance: none;
> style.

I don't think we'd want to remove accessibility from elements that have -moz-appearance: none, since the current controls are implemented that way as well as many other buttons that we want to customize in their styling. Perhaps there is a different property that can be set to tell the accessibility APIs to ignore this element?
There is one, an attribute called role with a value of "presentation". Unfortunately, on focusable elements, this is ignored so that accessibles for focusable elements always get created.
Posted patch Patch for bug 693014 v2 (obsolete) — Splinter Review
Marco: Thank you for the feedback. I have added the role="presentation" attribute, as well as -moz-user-focus: none, to the button. I also installed NVDA locally to try to test this, but I was unable to notice any difference (probably because I'm using NVDA incorrectly). Can you try this again with these changes?
Attachment #566356 - Attachment is obsolete: true
Attachment #566356 - Flags: review?(dolske)
Attachment #566641 - Flags: feedback?(marco.zehe)
Comment on attachment 566641 [details] [diff] [review]
Patch for bug 693014 v2

This looks the same to me as the previous one.
Attachment #566641 - Flags: feedback?(marco.zehe) → feedback-
Jared, fyi, accessibility relies on nsIContent::IsFocusable(). If this method returns true for the button having -moz-user-focus: none then accessible is created for it.
I'm curious to know if there has been an update on this issue, thanks.
I was thinking of this bug again during the Mozilla Festival this past weekend.  A number of use cases involved interaction with the video element while it's playing, making hot-regions, where the user can interact with areas.  For example, clicking on a person in a video to get more info about them.

I think Rick's point about needing a way to prevent the event and override is good.  Hopefully we can get there with these patches.
Whiteboard: [popcorn.js]
Couldn't this be worked around by placing a <div> over the video and/or using custom controls? I'm still not sure why this blocks people.

I'm going to unassign myself for now as I'm not actively working on this.
Assignee: jwein → nobody
Status: ASSIGNED → NEW
Battling top/left, absolute/relative/fixed positioning with CSS seems like an unfair burden to place on developers - especially when the problem doesn't exist in other browsers.
(In reply to Jared Wein [:jwein and :jaws] from comment #30)
> Couldn't this be worked around by placing a <div> over the video and/or
> using custom controls? I'm still not sure why this blocks people.
Finding a workaround is not the question here. Webdevs have found workaround for years for all sorts of bugs and uniplemented features. The question here is not whether Rick can implement his particular use case. I'm sure he already has found a workaround for his work. It is not absolutely blocking.

The question here is whether Firefox correctly implement standards.

The Mozilla Foundation has an ideal of the web defined in its Manifesto. One of the tools to achieve this ideal is supporting and writing open standards. And obviously Firefox implements these standards as a proof of feasability.
(In reply to David Bruant from comment #32)
> (In reply to Jared Wein [:jwein and :jaws] from comment #30)
> > Couldn't this be worked around by placing a <div> over the video and/or
> > using custom controls? I'm still not sure why this blocks people.
> Finding a workaround is not the question here. Webdevs have found workaround
> for years for all sorts of bugs and uniplemented features. The question here
> is not whether Rick can implement his particular use case. I'm sure he
> already has found a workaround for his work. It is not absolutely blocking.
> 
> The question here is whether Firefox correctly implement standards.
> 
> The Mozilla Foundation has an ideal of the web defined in its Manifesto. One
> of the tools to achieve this ideal is supporting and writing open standards.
> And obviously Firefox implements these standards as a proof of feasability.

I can't find where in the standards that it says if an author uses the |controls| attribute then the user agent *must* abide by some rules. Can you show me where it says this?
This is the section on |controls| that I have referenced, and I don't see where it conflicts with click-to-play.

http://www.whatwg.org/specs/web-apps/current-work/multipage/video.html#attr-media-controls
The standard in question is DOM Event Level 3 (http://www.w3.org/TR/DOM-Level-3-Events/#events-event-type-preventDefault):
Event.preventDefault: "When this method is invoked, the event must be canceled, meaning any default actions normally taken by the implementation as a result of the event must not occur"

In the case in question, clicking on the video element for play/pause is the default action. This is what the event.preventDefault method should cancel.

This is completely unrelated to whether the |controls| attribute is set or not. It just turns out that this is the circumstance that makes the bug appear.
(In reply to David Bruant from comment #35)
> The standard in question is DOM Event Level 3
> (http://www.w3.org/TR/DOM-Level-3-Events/#events-event-type-preventDefault):
> Event.preventDefault: "When this method is invoked, the event must be
> canceled, meaning any default actions normally taken by the implementation
> as a result of the event must not occur"
> 
> In the case in question, clicking on the video element for play/pause is the
> default action. This is what the event.preventDefault method should cancel.
> 
> This is completely unrelated to whether the |controls| attribute is set or
> not. It just turns out that this is the circumstance that makes the bug
> appear.

I'm sorry for derailing the bug comments. This was clearly already discussed in earlier comments and I forgot.
Of course the goal would be to preserve the "click the surface to play/pause" feature, but add the ability to event.preventDefault() when click events are observed on the video surface.
Posted patch Patch for bug 693014 v2.1 (obsolete) — Splinter Review
Marco, I haven't been able to figure out how to remove the accessibility text from this button. I've used role="presentation" and -moz-user-focus: ignore.

Instead of trying to remove the accessibility text, I have added the correct accessibility text to the button.

Is this OK? Would you be happy if we shipped this change? I have noticed that NVDA currently says "blank" when HTML5 videos are given focus. This would change that to say "clickable play button" or "clickable pause button" depending on the state.
Attachment #566641 - Attachment is obsolete: true
Attachment #577452 - Flags: feedback?(marco.zehe)
Attachment #577452 - Attachment description: Patch for bug 693014 v2.1 → WIP for bug 693014 v2.1
Comment on attachment 577452 [details] [diff] [review]
Patch for bug 693014 v2.1

Preemptive feedback on this patch is that if we keep the accessibility text, |role="presentation"| should be removed. I'll also move the |-moz-user-focus: ignore| up a few lines to be with the other buttons that share the same property.
Updating bug name so it is clear that the standard being broken is the preventDefault event.

http://www.w3.org/TR/2001/WD-DOM-Level-3-Events-20010823/events.html#Events-Event-preventDefault
Summary: Non-standard "click to play/pause" in video player cannot be prevented with event.preventDefault when |controls| is set → HTML5 <video controls> "click to play/pause" doesn't respect event.preventDefault standard
I'm worried that this title change creates an ambiguity - The _expected_ play/pause button that is displayed as a result of declaring the boolean |controls| attribute (ie. <video controls>) is fully capable of listening for click events and respecting calls `event.preventDefault()`. 

The issue is very specifically the result of additionally functionality being added to the video element, as illustrated here: http://gyazo.com/5440ece37979e39142247b8e13f8913c.png
Rick, I think that the discussion the bug suffices.
Comment on attachment 577452 [details] [diff] [review]
Patch for bug 693014 v2.1

David or Marco: Can one of you provide accessibility feedback on this patch?

I am looking to see if it is an acceptable change. In my testing with NVDA, giving focus to the video previously said "blank" and now it says "clickable play button".
Attachment #577452 - Flags: feedback?(bolterbugz)
Comment on attachment 577452 [details] [diff] [review]
Patch for bug 693014 v2.1

f=me since this is actually another UI element here. And having an actual name is much much better than having no name for this.
Attachment #577452 - Flags: feedback?(marco.zehe) → feedback+
Comment on attachment 577452 [details] [diff] [review]
Patch for bug 693014 v2.1

Considering that this is an a11y improvement, I think we should go ahead and take it. It doesn't regress the previous feature, and provides a win for web developers.

Since I've switched the <spacer> to being a <button>, I've changed the className from |.controlsSpacer| to |.playToggleButton|, respectively. I'm open to suggestions on a different className here.

|role="presentation"| should be removed from the final patch. As stated earlier, I'll move the |-moz-user-focus: ignore| up a few lines to be with the other buttons that share the same property.
Attachment #577452 - Flags: feedback?(bolterbugz) → review?(dolske)
Assignee: nobody → jwein
Status: NEW → ASSIGNED
Attachment #577452 - Attachment description: WIP for bug 693014 v2.1 → Patch for bug 693014 v2.1
(In reply to Marco Zehe (:MarcoZ) from comment #44)
> Comment on attachment 577452 [details] [diff] [review] [diff] [details] [review]
> Patch for bug 693014 v2.1
> 
> f=me since this is actually another UI element here. And having an actual
> name is much much better than having no name for this.

My understanding is that it shouldn't be exposed since the element is redundant...
Comment on attachment 577452 [details] [diff] [review]
Patch for bug 693014 v2.1

>--- a/toolkit/content/widgets/videocontrols.css
>+++ b/toolkit/content/widgets/videocontrols.css

>+.playToggleButton {
>+  -moz-appearance: none;
>+  margin: 0;
>+  background: transparent;
>+  border: none;
>+  -moz-user-focus: ignore;
>+}

This belongs in the theme files, since you're overriding the theme-provided button.css. Of course, it would be preferable if you didn't use a button in the first place.
(In reply to Dão Gottwald [:dao] from comment #47)
> Comment on attachment 577452 [details] [diff] [review] [diff] [details] [review]
> Patch for bug 693014 v2.1
> 
> Of course, it would be preferable if you didn't use a button in
> the first place.

Yeah... it seems awkward, but I don't know a different way to keep the present behavior but also respect event.preventDefault. Do you have a different idea in mind?
This is somewhat nasty but it works...
Blocks: 518008
No longer depends on: 518008
apparently getPreventDefault is deprecated
Attachment #578950 - Attachment is obsolete: true
Attachment #578951 - Flags: review?(dolske)
Attachment #578951 - Flags: feedback?(jwein)
Component: Video/Audio → Video/Audio Controls
Product: Core → Toolkit
QA Contact: video.audio → video.audio
Comment on attachment 578951 [details] [diff] [review]
read defaultPrevented and toggle pause asynchronously

Review of attachment 578951 [details] [diff] [review]:
-----------------------------------------------------------------

Interesting approach. I tested it out and it works fine. Maybe include a comment that links to this bug so it will be clear to others why this approach was used?
Attachment #578951 - Flags: feedback?(jwein) → feedback+
Comment on attachment 577452 [details] [diff] [review]
Patch for bug 693014 v2.1

Dao's solution is superior than my patch in that his patch doesn't cause us to change our accessibility approach.
Attachment #577452 - Flags: review?(dolske)
Attachment #578951 - Flags: review?(dolske) → review+
http://hg.mozilla.org/integration/mozilla-inbound/rev/3f4c460bb95e
Assignee: jwein → dao
Target Milestone: --- → mozilla12
(In reply to Dão Gottwald [:dao] from comment #53)
> http://hg.mozilla.org/integration/mozilla-inbound/rev/3f4c460bb95e

Just a random question:
In web content, setTimeout(f, 0) is clamped to 4ms (1sec in background tab). Does this apply to chrome code as well or does it really mean "execute asynchronously asap" in chrome code?
(In reply to David Bruant from comment #54)
> In web content, setTimeout(f, 0) is clamped to 4ms (1sec in background tab).
> Does this apply to chrome code as well or does it really mean "execute
> asynchronously asap" in chrome code?

I don't think it really matters here, but AFAIK only nested timeouts should be clamped (bug 512645).
https://hg.mozilla.org/mozilla-central/rev/3f4c460bb95e
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
This "bug" is in Firefox 61.0.2.
Reading through the bug history it seems like it was actually fixed at some point. Is this a regression? 

For an example, on the issue, you can look at http://jsbin.com/xagixudidu/edit?html,output 

Click event works as expected when the controls attribute is not present in the video element.
Flags: needinfo?(dao+bmo)
Correct link to an example showing the issue http://jsbin.com/duluyej/1/edit?html,output
(In reply to Octavio Rodríguez from comment #57)
> This "bug" is in Firefox 61.0.2.
> Reading through the bug history it seems like it was actually fixed at some
> point. Is this a regression?

There might well be a regression. Please file a new bug.
Flags: needinfo?(dao+bmo)
You need to log in before you can comment on or make changes to this bug.