601881 - video and audio should never play automatically in editor

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

<video autoplay> and <audio autoplay> should never autoplay in the editor or Midas. Suppose you need to insert a <video autoplay loop> in your markup... Editing becomes hell.

Comment 1

[(no longer active)]

Indeed!  I'll take this, but I'm afraid that I may not get to it before Firefox 4, given the number of remaining blockers...

Comment 2

[Matthew Gregan [:kinetik]]

Does mozAutoplayEnabled (added in bug 478982) help with this?

Comment 3

[(no longer active)]

(In reply to comment #2)
> Does mozAutoplayEnabled (added in bug 478982) help with this?

Possibly.  I think what we want to do is to modify nsHTMLMediaElement::GetMozAutoplayEnabled and always return false if the node is editable...  In fact, I may write a patch to that effect!  :-)

Comment 4

[(no longer active)]

Attachment: Patch (v1)

Comment 5

[(no longer active)]

Comment on attachment 481128 [details] [diff] [review]
Patch (v1)

Boris, is this something which you can review?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 481128 [details] [diff] [review]
Patch (v1)

Looks good.

Comment 7

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/9002f89cf423

Comment 8

[Nochum Sossonko [:Natch]]

Comment on attachment 481128 [details] [diff] [review]
Patch (v1)

This seems wrong. GetMozAutoplayEnabled isn't what decides whether a video is played or not, it just reflects the mAutoplayEnabled boolean. In BindToTree GetMozAutoplayEnabled is never called. Seems to me like this check really belongs in IsAutoplayEnabled or something, although technically that may be wrong too since autoplay is really enabled, but the video won't autoplay for other reasons.

My .02.

Comment 9

[Nochum Sossonko [:Natch]]

The check should probably go in the same place as (!aDocument || !aDocument->IsStaticDocument()) in BindToTree. Ftr, the test won't catch this since it doesn't check if the video is playing or not (since there is no source anyhow)...

Comment 10

[Boris Zbarsky [:bzbarsky]]

Ah, I thought the xbl code was what started the autoplay.  Do we have some other code actually doing it?

Comment 11

[Nochum Sossonko [:Natch]]

(In reply to comment #10)
Nope. Seems like the deciding place is here: http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsHTMLMediaElement.cpp?mark=2179-2192#2171

NotifyAutoplayDataReady is called from different locations (sometimes the decoder) once the video data is ready, and then it is played.

Comment 12

[Nochum Sossonko [:Natch]]

See the url for a testcase. mozAutoplayEnabled will report false, but the video will still play.

Comment 13

[(no longer active)]

Attachment: WIP

Hmm, I'd expect this approach to work, but I'm obviously wrong, as the test fails, and I actually see that the media elements are playing on the screen...

Matthew, do you have any idea what I'm missing here?

Comment 14

[Chris Pearce [:cpearce (Not reading bugmail)]]

In nsHTMLMediaElement::BindToTree() when we set mAutoplayEnabled, IsEditable() is returning PR_FALSE because the video element itself isn't contentEditable, and it's not in the designMode document yet because we call IsEditable() *before* we call to nsGenericHTMLElement::BindToTree() to do the binding.

If you set mAutoplayEnabled after the call to nsGenericHTMLElement::BindToTree(), the video element's in the document, and it seems to work.

You're also calling video.play() in your testcase, that's probably not helping! ;)

Comment 15

[Nochum Sossonko [:Natch]]

(In reply to comment #14)
> If you set mAutoplayEnabled after the call to
> nsGenericHTMLElement::BindToTree(), the video element's in the document, and it
> seems to work.

The only problem with that is whether the video controls will read the property in time. Video controls is the current consumer for mozAutoplayEnabled.

Comment 16

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to comment #15)
> (In reply to comment #14)
> > If you set mAutoplayEnabled after the call to
> > nsGenericHTMLElement::BindToTree(), the video element's in the document, and it
> > seems to work.
> 
> The only problem with that is whether the video controls will read the property
> in time. Video controls is the current consumer for mozAutoplayEnabled.

Ok, when setting mAutoplayEnabled as well as checking !IsEditable() we could also check if !aDocument->HasFlag(NODE_IS_EDITABLE) then?

With or without this patch, I still don't see video controls being displayed in designMode documents.

Comment 17

[(no longer active)]

(In reply to comment #14)
> In nsHTMLMediaElement::BindToTree() when we set mAutoplayEnabled, IsEditable()
> is returning PR_FALSE because the video element itself isn't contentEditable,
> and it's not in the designMode document yet because we call IsEditable()
> *before* we call to nsGenericHTMLElement::BindToTree() to do the binding.

Ah, right!

> If you set mAutoplayEnabled after the call to
> nsGenericHTMLElement::BindToTree(), the video element's in the document, and it
> seems to work.

Yep!

> You're also calling video.play() in your testcase, that's probably not helping!
> ;)

Indeed, it's not helping at all!  So much for hg copy&&paste I guess!

Comment 18

[(no longer active)]

(In reply to comment #16)
> (In reply to comment #15)
> > (In reply to comment #14)
> > > If you set mAutoplayEnabled after the call to
> > > nsGenericHTMLElement::BindToTree(), the video element's in the document, and it
> > > seems to work.
> > 
> > The only problem with that is whether the video controls will read the property
> > in time. Video controls is the current consumer for mozAutoplayEnabled.
> 
> Ok, when setting mAutoplayEnabled as well as checking !IsEditable() we could
> also check if !aDocument->HasFlag(NODE_IS_EDITABLE) then?

Sounds reasonable.

> With or without this patch, I still don't see video controls being displayed in
> designMode documents.

Hmm, does the same thing happen if <body> is contenteditable, for example?

Comment 19

[(no longer active)]

Attachment: Patch (v2)

Comment 20

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to comment #18)
> > With or without this patch, I still don't see video controls being displayed in
> > designMode documents.
> 
> Hmm, does the same thing happen if <body> is contenteditable, for example?

The behaviour I see with your patch applied is:

1. Setting document.designMode="on" *before* the video element gets parsed and added to page - controls do not appear.
2. Seting document.designMode="on" *after* the video element gets parsed and added to page - controls do not appear.
3. contentEditable <body> - controls appear.

In all of those cases I see lots of assertion failures when I type, such as:

WARNING: NS_ENSURE_TRUE(outNode && inNode) failed: file c:/Users/cpearce/src/orange/objdir/editor/li
beditor/html/../../../../editor/libeditor/html/nsHTMLEditor.cpp, line 4602
0[1dc680]: WARNING: NS_ENSURE_TRUE(node) failed: file c:/Users/cpearce/src/orange/objdir/editor/libe
ditor/html/../../../../editor/libeditor/html/nsHTMLEditor.cpp, line 4687
WARNING: NS_ENSURE_TRUE(node) failed: file c:/Users/cpearce/src/orange/objdir/editor/libeditor/html/
../../../../editor/libeditor/html/nsHTMLEditor.cpp, line 4687
0[1dc680]: WARNING: NS_ENSURE_TRUE(mListenerEnabled) failed: file c:/Users/cpearce/src/orange/objdir
/editor/libeditor/html/../../../../editor/libeditor/html/nsHTMLEditRules.cpp, line 8518
WARNING: NS_ENSURE_TRUE(mListenerEnabled) failed: file c:/Users/cpearce/src/orange/objdir/editor/lib
editor/html/../../../../editor/libeditor/html/nsHTMLEditRules.cpp, line 8518
0[1dc680]: WARNING: NS_ENSURE_TRUE(elem) failed: file c:/Users/cpearce/src/orange/objdir/editor/libe
ditor/html/../../../../editor/libeditor/html/nsHTMLEditUtils.cpp, line 396
WARNING: NS_ENSURE_TRUE(elem) failed: file c:/Users/cpearce/src/orange/objdir/editor/libeditor/html/
../../../../editor/libeditor/html/nsHTMLEditUtils.cpp, line 396
0[1dc680]: WARNING: NS_ENSURE_TRUE(elem) failed: file c:/Users/cpearce/src/orange/objdir/editor/libe
ditor/html/../../../../editor/libeditor/html/nsHTMLEditUtils.cpp, line 396
WARNING: NS_ENSURE_TRUE(elem) failed: file c:/Users/cpearce/src/orange/objdir/editor/libeditor/html/
../../../../editor/libeditor/html/nsHTMLEditUtils.cpp, line 396
0[1dc680]: WARNING: NS_ENSURE_TRUE(elem) failed: file c:/Users/cpearce/src/orange/objdir/editor/libe
ditor/html/../../../../editor/libeditor/html/nsHTMLEditUtils.cpp, line 396
WARNING: NS_ENSURE_TRUE(elem) failed: file c:/Users/cpearce/src/orange/objdir/editor/libeditor/html/
../../../../editor/libeditor/html/nsHTMLEditUtils.cpp, line 396
0[1dc680]: WARNING: NS_ENSURE_TRUE(mLastNBSPNode) failed: file c:/Users/cpearce/src/orange/objdir/ed
itor/libeditor/html/../../../../editor/libeditor/html/nsWSRunObject.cpp, line 663
WARNING: NS_ENSURE_TRUE(mLastNBSPNode) failed: file c:/Users/cpearce/src/orange/objdir/editor/libedi
tor/html/../../../../editor/libeditor/html/nsWSRunObject.cpp, line 663
0[1dc680]: WARNING: NS_ENSURE_TRUE(mLastNBSPNode) failed: file c:/Users/cpearce/src/orange/objdir/ed
itor/libeditor/html/../../../../editor/libeditor/html/nsWSRunObject.cpp, line 663
WARNING: NS_ENSURE_TRUE(mLastNBSPNode) failed: file c:/Users/cpearce/src/orange/objdir/editor/libedi
tor/html/../../../../editor/libeditor/html/nsWSRunObject.cpp, line 663
0[1dc680]: WARNING: NS_ENSURE_TRUE(*outIsEmptyNode) failed: file c:/Users/cpearce/src/orange/objdir/
editor/libeditor/html/../../../../editor/libeditor/html/nsHTMLEditor.cpp, line 5095
WARNING: NS_ENSURE_TRUE(*outIsEmptyNode) failed: file c:/Users/cpearce/src/orange/objdir/editor/libe
ditor/html/../../../../editor/libeditor/html/nsHTMLEditor.cpp, line 5095
0[1dc680]: WARNING: NS_ENSURE_TRUE(aNode) failed: file c:/Users/cpearce/src/orange/objdir/editor/lib
editor/base/../../../../editor/libeditor/base/nsEditor.cpp, line 3599
WARNING: NS_ENSURE_TRUE(aNode) failed: file c:/Users/cpearce/src/orange/objdir/editor/libeditor/base
/../../../../editor/libeditor/base/nsEditor.cpp, line 3599

Comment 21

[Boris Zbarsky [:bzbarsky]]

That patch doesn't look right for cases where the document is not editable but the new parent (and hence our node, but only after we finish the bind) is....

Why can't we just do this check after calling the superclass BindToTree?

Comment 22

[(no longer active)]

Attachment: Patch (v3)

(In reply to comment #21)
> That patch doesn't look right for cases where the document is not editable but
> the new parent (and hence our node, but only after we finish the bind) is....

Right.

> Why can't we just do this check after calling the superclass BindToTree?

This version of the patch does that.

Comment 23

[Boris Zbarsky [:bzbarsky]]

If you're already bound, IsEditable() takes aDocument into account.

Why do you need the two separate sets of mAutoplayEnabled?  Why not just set after the call to the superclass?

And is it me, or is mIsBindingToTree write-only?

All of which is to say, it looks like that whole block can just move past the superclass bind call....

Comment 24

[(no longer active)]

Attachment: Patch (v4)

(In reply to comment #23)
> If you're already bound, IsEditable() takes aDocument into account.

True.

> Why do you need the two separate sets of mAutoplayEnabled?  Why not just set
> after the call to the superclass?

Well, I wasn't sure if it's safe to do that... But I guess nsGenericHTMLElement::BindToTree doesn't touch mAutoplayEnabled... ;-)

> And is it me, or is mIsBindingToTree write-only?

Seems like a superfluous variable.  Removed it in this version of the patch.

> All of which is to say, it looks like that whole block can just move past the
> superclass bind call....

Done.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Hmm.  Why is the change to CanActivateAutoplay needed?

Comment 26

[(no longer active)]

In case autoplay can get triggered before a node has been bound to a document tree.  I;m nit sure if that can happen in practice though.

Comment 27

[Boris Zbarsky [:bzbarsky]]

It'd be good if someone who understands autoplay can check that part over.

The rest of the patch looks fine to me.

Comment 28

[(no longer active)]

Comment on attachment 483578 [details] [diff] [review]
Patch (v4)

(In reply to comment #27)
> It'd be good if someone who understands autoplay can check that part over.
> 
> The rest of the patch looks fine to me.

Requesting review from Chris on that.

Comment 29

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 483578 [details] [diff] [review]
Patch (v4)

Looks good to me.

It's a shame we need to rely on a timeout for the test, but we don't have a "I didn't play" event. :/

(In reply to comment #26)
> In case autoplay can get triggered before a node has been bound to a document
> tree.  I;m nit sure if that can happen in practice though.

According to the description of HAVE_ENOUGH_DATA media readyState, we should only autoplay when a media element is in a document:
http://www.whatwg.org/specs/web-apps/current-work/multipage/video.html#the-ready-states

However Firefox, Webkit and Opera do autoplay when not in a document (contrary to the spec). IE9beta does not, but I'm not convinced that's intentional... I've sent a message to the WhatWG list to see if we want to change the spec to match implemented behaviour.

In the meantime, your change to CanActivateAutoplay is needed.

Comment 30

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/9edc2a7cd46d