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

Status: RESOLVED FIXED

(Toolkit :: Video/Audio Controls, defect)

Comment 1

[Rick Waldron [:rwaldron]]

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()

Comment 2

[Rick Waldron [:rwaldron]]

This _will_ prevent you from interacting with the normal controls

Comment 3

[Paul Rouget [:paul]]

Jared, what do you think?

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 5

[Rick Waldron [:rwaldron]]

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.

Comment 6

[Rick Waldron [:rwaldron]]

ahem "your"

Comment 7

[David Bruant]

(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.

Comment 8

[David Bruant]

Sorry for the summury change. Our changes collapsed.

Comment 9

[Rick Waldron [:rwaldron]]

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

Comment 10

[Paul Rouget [:paul]]

I understand why this is hard to fix. Can we consider disabling this feature for now?

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 12

[Justin Dolske [:Dolske]]

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.

Comment 13

[David Bruant]

(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.

Comment 14

[Rick Waldron [:rwaldron]]

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

Comment 15

[Rick Waldron [:rwaldron]]

(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.

Comment 16

[Jared Wein [:jaws] (please needinfo? me)]

(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.

Comment 17

[Rick Waldron [:rwaldron]]

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.

Comment 18

[Rick Waldron [:rwaldron]]

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

Comment 19

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for bug 693014

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?

Comment 20

[Rick Waldron [:rwaldron]]

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 21

[Marco Zehe (:MarcoZ)]

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.

Comment 22

[Marco Zehe (:MarcoZ)]

davidb, surkov, please see my comment/question in comment #21.

Comment 23

[Jared Wein [:jaws] (please needinfo? me)]

(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?

Comment 24

[Marco Zehe (:MarcoZ)]

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.

Comment 25

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for bug 693014 v2

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?

Comment 26

[Marco Zehe (:MarcoZ)]

Comment on attachment 566641 [details] [diff] [review]
Patch for bug 693014 v2

This looks the same to me as the previous one.

Comment 27

[alexander :surkov (:asurkov)]

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.

Comment 28

[Rick Waldron [:rwaldron]]

I'm curious to know if there has been an update on this issue, thanks.

Comment 29

[David Humphrey (:humph)]

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.

Comment 30

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 31

[Rick Waldron [:rwaldron]]

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.

Comment 32

[David Bruant]

(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.

Comment 33

[Jared Wein [:jaws] (please needinfo? me)]

(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?

Comment 34

[Jared Wein [:jaws] (please needinfo? me)]

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

Comment 35

[David Bruant]

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.

Comment 36

[Jared Wein [:jaws] (please needinfo? me)]

(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.

Comment 37

[Rick Waldron [:rwaldron]]

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.

Comment 38

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for bug 693014 v2.1

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.

Comment 39

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 40

[Jared Wein [:jaws] (please needinfo? me)]

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

Comment 41

[Rick Waldron [:rwaldron]]

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

Comment 42

[David Humphrey (:humph)]

Rick, I think that the discussion the bug suffices.

Comment 43

[Jared Wein [:jaws] (please needinfo? me)]

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".

Comment 44

[Marco Zehe (:MarcoZ)]

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.

Comment 45

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 46

[Dão Gottwald [::dao]]

(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 47

[Dão Gottwald [::dao]]

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.

Comment 48

[Jared Wein [:jaws] (please needinfo? me)]

(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?

Comment 49

[Dão Gottwald [::dao]]

Attachment: read getPreventDefault() and toggle pause asynchronously

This is somewhat nasty but it works...

Comment 50

[Dão Gottwald [::dao]]

Attachment: read defaultPrevented and toggle pause asynchronously

apparently getPreventDefault is deprecated

Comment 51

[Jared Wein [:jaws] (please needinfo? me)]

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?

Comment 52

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 53

[Dão Gottwald [::dao]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/3f4c460bb95e

Comment 54

[David Bruant]

(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?

Comment 55

[Dão Gottwald [::dao]]

(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).

Comment 56

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3f4c460bb95e

Comment 57

[Octavio Rodríguez]

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.

Comment 58

[Octavio Rodríguez]

Correct link to an example showing the issue http://jsbin.com/duluyej/1/edit?html,output

Comment 59

[Dão Gottwald [::dao]]

(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.