video and audio should never play automatically in editor

RESOLVED FIXED in mozilla2.0b8

Status

()

Core
Editor
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: glazou, Assigned: Ehsan)

Tracking

Trunk
mozilla2.0b8
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

7 years ago
<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.
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...
Component: Video/Audio → Editor
QA Contact: video.audio → editor
Assignee: nobody → ehsan
Does mozAutoplayEnabled (added in bug 478982) help with this?
(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!  :-)
Created attachment 481128 [details] [diff] [review]
Patch (v1)
Attachment #481128 - Flags: review?(jst)
Attachment #481128 - Flags: approval2.0?
Status: NEW → ASSIGNED
Whiteboard: [has patch][needs review jst]
Comment on attachment 481128 [details] [diff] [review]
Patch (v1)

Boris, is this something which you can review?
Attachment #481128 - Flags: review?(jst) → review?(bzbarsky)
Comment on attachment 481128 [details] [diff] [review]
Patch (v1)

Looks good.
Attachment #481128 - Flags: review?(bzbarsky)
Attachment #481128 - Flags: review+
Attachment #481128 - Flags: approval2.0?
Attachment #481128 - Flags: approval2.0+
Whiteboard: [has patch][needs review jst] → [needs landing]
http://hg.mozilla.org/mozilla-central/rev/9002f89cf423
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs landing]
Target Milestone: --- → mozilla2.0b8
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.
Attachment #481128 - Flags: review-
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)...
Ah, I thought the xbl code was what started the autoplay.  Do we have some other code actually doing it?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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.
See the url for a testcase. mozAutoplayEnabled will report false, but the video will still play.
Status: REOPENED → ASSIGNED
Created attachment 483228 [details] [diff] [review]
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?
Attachment #481128 - Attachment is obsolete: true
Attachment #483228 - Flags: feedback?(kinetik)
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! ;)
(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.
(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.
(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!
(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?
Created attachment 483352 [details] [diff] [review]
Patch (v2)
Attachment #483228 - Attachment is obsolete: true
Attachment #483352 - Flags: review?(bzbarsky)
Attachment #483352 - Flags: approval2.0?
Attachment #483228 - Flags: feedback?(kinetik)
(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
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?
Created attachment 483546 [details] [diff] [review]
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.
Attachment #483352 - Attachment is obsolete: true
Attachment #483546 - Flags: review?(bzbarsky)
Attachment #483546 - Flags: approval2.0?
Attachment #483352 - Flags: review?(bzbarsky)
Attachment #483352 - Flags: approval2.0?
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....
Created attachment 483578 [details] [diff] [review]
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.
Attachment #483546 - Attachment is obsolete: true
Attachment #483578 - Flags: review?(bzbarsky)
Attachment #483578 - Flags: approval2.0?
Attachment #483546 - Flags: review?(bzbarsky)
Attachment #483546 - Flags: approval2.0?
Hmm.  Why is the change to CanActivateAutoplay needed?
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.
It'd be good if someone who understands autoplay can check that part over.

The rest of the patch looks fine to me.
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.
Attachment #483578 - Flags: review?(chris)
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.
Attachment #483578 - Flags: review?(chris) → review+
Attachment #483578 - Flags: approval2.0? → approval2.0+
Whiteboard: [needs landing]
http://hg.mozilla.org/mozilla-central/rev/9edc2a7cd46d
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing]
Attachment #483578 - Flags: review?(bzbarsky)
You need to log in before you can comment on or make changes to this bug.