HTML5 <video>/<audio> elements should not inherit document direction

RESOLVED FIXED in mozilla1.9.2a1

Status

()

Toolkit
Video/Audio Controls
RESOLVED FIXED
9 years ago
a year ago

People

(Reporter: tomer, Assigned: Ehsan)

Tracking

(Blocks: 1 bug, {fixed1.9.1, intl, rtl})

unspecified
mozilla1.9.2a1
fixed1.9.1, intl, rtl
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(7 attachments, 2 obsolete attachments)

(Reporter)

Description

9 years ago
Created attachment 374107 [details]
html5 video player in Firefox 3.5b3 on RTL document

I have linked an <video> to my site, which is in Hebrew. Back in August we held a mail conversation and decided to keep the <video> interface the same for RTL locales, but it appears that the document is able to control the player direction.

Steps to reproduce:
Enter the following URL with Firefox 3.5b3 - http://tomercohen.com/2009/04/22/firefox-35-thank-you-video/ 

Notice the player controls are mirrored if comparing to the regular interface, and the progress bar is not synced with the seek button.


After adding dir=ltr to the <video> element I can get back to the default player display, but dir=ltr is not officially supported by HTML5 video[1] as far as I know, and I don't think we should specify this for every video we publish on pages with <body dir=rtl>. 

[1] https://developer.mozilla.org/En/HTML/Element/Video
(Reporter)

Comment 1

9 years ago
Created attachment 374108 [details]
html5 video player in Firefox 3.5b3 on RTL document after adding dir=ltr
(Reporter)

Updated

9 years ago
Blocks: 382267
Yes, the conclusion from the email thread was that the controls should remain the same (ltr vs rtl). Thus I didn't need to make the controls reversed for rtl, but I didn't realize I would have to do something to *not* make them reversed. Sigh.

Hmm, I'm not quite sure how to fix this (although it's easy to reproduce)...

Setting |dir="ltr"| on the <hbox class="controlBar"> fixes the problem, but isn't quite right. I think we want to affect the whole binding, not sprinkle |dir| attributes around here and there (because it's messy, and ends up being a whack-a-mole problem).

But setting |dir="ltr"| on the XBL <content> didn't help (nor did |chromedir="ltr"|, not sure what the difference is). Also tried setting |dir="ltr"| on the topmost XUL element in the binding (the <stack>), and that didn't help either.
(Reporter)

Comment 3

9 years ago
Created attachment 374145 [details]
Testcase: The body element has dir=rtl which cause    the document to be RTL and the video player controls to appear in the wrong order.
(Reporter)

Updated

9 years ago
Attachment #374145 - Attachment description: The body element has dir=rtl which cause the document to be RTL and the video player controls to appear in the wrong order. → Testcase: The body element has dir=rtl which cause the document to be RTL and the video player controls to appear in the wrong order.
(Reporter)

Comment 4

9 years ago
Created attachment 374148 [details]
Testcase 2: The html element has direction:rtl which    cause the document to be RTL and the video player controls to appear in the wrong order.
(Reporter)

Comment 5

9 years ago
Created attachment 374149 [details]
Testcase 3: The video element has dir=rtl which cause    the controls to be RTL. Since <video> don't have official 'dir' parameter, we don't know what should happen when this parameter is being used.

Please note that the third testcase has no expected behavior by the spec as far as I know. I think the best would be to fix the progress bar issue and keep it possible to add dir=rtl in case the author know what he is doing.
chromedir is a made-up attribute (similar to anonid) that doesn't affect the actual node, but can (and is the de-facto attribute to) be used in css to fix rtl<->ltr problems.
(Assignee)

Comment 7

9 years ago
I have a patch, I'll post it here once I whip up some reftests as well.
Assignee: nobody → ehsan.akhgari
Status: NEW → ASSIGNED
Version: unspecified → Trunk
(Assignee)

Comment 8

9 years ago
Created attachment 374268 [details] [diff] [review]
Patch (v1)

Make all video controls LTR except in the case where |dir="rtl"| has been specified on the audio/video element.

Requesting review from Justin on the videocontrols part, and from Roc on the reftests.
Attachment #374268 - Flags: superreview?(roc)
Attachment #374268 - Flags: review?(roc)
Attachment #374268 - Flags: review?(dolske)
(Assignee)

Comment 9

9 years ago
(In reply to comment #8)
> Created an attachment (id=374268) [details]
> Patch (v1)

Err, forgot to sort ogg-video/reftest.list alphabetically, will do upon landing.  :-)
Comment on attachment 374268 [details] [diff] [review]
Patch (v1)

2 things:

* The conclusion from the aforementioned email thread (which you were on) was that the video controls shouldn't differ in RTL. So we don't support that (and it looks like crap, now that bug 475318 has landed). This isn't the right bug to revisit that. So, remove the special-casing for allowing <video dir="rtl">.

* I suspect your tests are not testing the right thing, or might fail randomly. When an error condition occurs, the controls fade out. So, depending on the timing of the reftests, the controls could be in different fade-out states, or could be fully faded out in both (in which case you're basically reftesting an empty frame). There are some existing ogg files in the tree, just reference those and you should be good. As long as |autoplay| isn't set, the controls will be shown by default. Oh, hmm, except that we show a progress bar for loading the media, so that could be another source of randomness. You'll probably need to make these deferred reftests (with class="reftest-wait", see examples in tree) that waits until onload. Which might also run afoul of the reduced media cache size (roc added a workaround for that in test_videocontrols.html) but if you're just using layout/reftests/ogg-video/black140x100.ogv that should be small enough to not trigger issues.
Attachment #374268 - Flags: review?(dolske) → review-
add <script src="use_large_cache.js">, don't rely on the video fitting in the cache.
(Assignee)

Comment 12

9 years ago
Created attachment 374493 [details] [diff] [review]
Patch (v2)

Removed the dir=rtl attribute handling, and reworked the tests based on the review comments.
Attachment #374268 - Attachment is obsolete: true
Attachment #374493 - Flags: review?(dolske)
Attachment #374268 - Flags: superreview?(roc)
Attachment #374268 - Flags: review?(roc)
Comment on attachment 374493 [details] [diff] [review]
Patch (v2)

>+++ b/toolkit/content/tests/widgets/test_videocontrols_direction.html
...
>+  <video id="v" src="black.ogv" style="display: none;"></video>
...
>+<script type="text/javascript" src="use_large_cache.js"></script>

I suspect this needs to come before the <video>, or else the video's load will be kicked off with the potentially-smaller cache. But, really, I don't think you need the <video> in the top-level test at all. The iframe's load won't fire until the iframe's video has loaded.


>+++ b/toolkit/content/tests/widgets/videocontrols_direction-1-ref.html
...
>+<video controls mozNoDynamicControls id="av" src="black.ogv"></video>

Do you actually need mozNoDynamicControls here? As long as it's not |autoplay|, it should be fine without it... I added this before we made the controls appear by default (w/o needing a mouseover), so I'm actually hoping to remove it.

Are the various flavors of setting rtl on <html> or <body> with dir/style really all that different, or are you just being exhaustive? That's fine, just wondering.


>+++ b/toolkit/content/tests/widgets/videocontrols_direction-2-ref.html
...
>+<audio controls mozNoDynamicControls id="av" style="width: 150px"></audio>

None of the <audio> tests have a |src| attribute set.


>+++ b/toolkit/content/tests/widgets/videocontrols_direction-2e.html
...
>+<audio controls mozNoDynamicControls id="av" style="width: 150px" style="direction: rtl;"></audio>

You have 2 style attributes here... Does that work?


>+++ b/toolkit/content/widgets/videocontrols.xml
...
>-    <xbl:content xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
>+    <xbl:content xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
>+                 class="frame">

"frame" doesn't strike me as a particularly descriptive name, but I don't have a compelling alternative for this bikeshed.


r+ if you set src on the <audio> tests, and verify the other things are ok (fix/cleanup as needed).
Attachment #374493 - Flags: review?(dolske) → review+
You'll technically need to get a review from a real toolkit peer too, like Enn.
Attachment #374493 - Flags: review?(enndeakin)
Comment on attachment 374493 [details] [diff] [review]
Patch (v2)

Oh, I guess an updated patch is still needed here, before sending it off to Enn for review...

Ehsan?
Attachment #374493 - Flags: review?(enndeakin)
Created attachment 377008 [details]
Screenshot of current (unpatched) behaviour
I think this should block, the controls look really messed up and do not function properly.
Flags: blocking1.9.1?
(Assignee)

Comment 18

9 years ago
(In reply to comment #15)
> (From update of attachment 374493 [details] [diff] [review])
> Oh, I guess an updated patch is still needed here, before sending it off to Enn
> for review...
> 
> Ehsan?

Yeah, this needs a new patch.  I've been buried in other stuff, but I'll try to get to this bug soon.
Whiteboard: [needs new patch]
Ehsan: Ping again. :)
(Assignee)

Comment 20

9 years ago
Created attachment 378273 [details] [diff] [review]
Patch (v3)

(In reply to comment #13)
> (From update of attachment 374493 [details] [diff] [review])
> >+++ b/toolkit/content/tests/widgets/test_videocontrols_direction.html
> ...
> >+  <video id="v" src="black.ogv" style="display: none;"></video>
> ...
> >+<script type="text/javascript" src="use_large_cache.js"></script>
> 
> I suspect this needs to come before the <video>, or else the video's load will
> be kicked off with the potentially-smaller cache. But, really, I don't think
> you need the <video> in the top-level test at all. The iframe's load won't fire
> until the iframe's video has loaded.

Fixed based on our irc discussion.

> >+++ b/toolkit/content/tests/widgets/videocontrols_direction-1-ref.html
> ...
> >+<video controls mozNoDynamicControls id="av" src="black.ogv"></video>
> 
> Do you actually need mozNoDynamicControls here? As long as it's not |autoplay|,
> it should be fine without it... I added this before we made the controls appear
> by default (w/o needing a mouseover), so I'm actually hoping to remove it.

I did that out of the fear that without mozNoDynamicControls, the tests could fail if during the test run the mouse is moved over the videos, but after some testing without mozNoDynamicControls, this is not the case, so I've removed those.

> Are the various flavors of setting rtl on <html> or <body> with dir/style
> really all that different, or are you just being exhaustive? That's fine, just
> wondering.

No, I'm just being exhaustive.

BTW, dir/style is actually different (not that it matters here, of course):

<http://mxr.mozilla.org/mozilla-central/source/layout/style/html.css#43>

> >+++ b/toolkit/content/tests/widgets/videocontrols_direction-2-ref.html
> ...
> >+<audio controls mozNoDynamicControls id="av" style="width: 150px"></audio>
> 
> None of the <audio> tests have a |src| attribute set.

I didn't realize that it was necessary.  Originally I was setting the src attribute on audio elements as well, but eliminated them before submitting the patch...  I've added them back again.

> >+++ b/toolkit/content/tests/widgets/videocontrols_direction-2e.html
> ...
> >+<audio controls mozNoDynamicControls id="av" style="width: 150px" style="direction: rtl;"></audio>
> 
> You have 2 style attributes here... Does that work?

Yes, apparently, but it's embarrassing... I fixed that.

> >+++ b/toolkit/content/widgets/videocontrols.xml
> ...
> >-    <xbl:content xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
> >+    <xbl:content xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
> >+                 class="frame">
> 
> "frame" doesn't strike me as a particularly descriptive name, but I don't have
> a compelling alternative for this bikeshed.

How about mediaControlsFrame?


I also splitted the tests into two files so that they won't time out on slow machines running debug builds, etc.
Attachment #374493 - Attachment is obsolete: true
Attachment #378273 - Flags: review?(enndeakin)
Attachment #378273 - Flags: review?(dolske)
(Assignee)

Updated

9 years ago
Whiteboard: [needs new patch]
(In reply to comment #20)

> > None of the <audio> tests have a |src| attribute set.
> 
> I didn't realize that it was necessary.

Without a source, we currently show the error overlay and hide the controls. So, if the controls are actually messed up due to some RTL issue, the test wouldn't have caught that.
Attachment #378273 - Flags: review?(mconnor)
Attachment #378273 - Flags: review?(enndeakin)
Attachment #378273 - Flags: review?(dolske)
Attachment #378273 - Flags: review+
Comment on attachment 378273 [details] [diff] [review]
Patch (v3)

r+, Enn is away this week so I'll bounce it to mconnor.

Updated

9 years ago
Attachment #378273 - Flags: review?(mconnor) → review+
Comment on attachment 378273 [details] [diff] [review]
Patch (v3)

r=me, I trust dolske on the tests.
(Assignee)

Comment 24

9 years ago
http://hg.mozilla.org/mozilla-central/rev/b16fcc7f2031
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
(Reporter)

Comment 25

9 years ago
(In reply to comment #24)
>           What    |Removed                     |Added
>----------------------------------------------------------------------------
>            Status|ASSIGNED                    |RESOLVED
>        Resolution|                            |FIXED
>  Target Milestone|---                         |mozilla1.9.2a1
>              Flag|                            |in-testsuite+


Does it means this fix won't make it into 1.9.1? 

(hint: this bug is 'blocking?' for 1.9.1)
We can and should definitely still land this for 1.9.1.
Flags: blocking1.9.1? → blocking1.9.1+
(Assignee)

Updated

9 years ago
Duplicate of this bug: 496707
Component: Video/Audio → Video/Audio Controls
Product: Core → Toolkit
QA Contact: video.audio → video.audio
Target Milestone: mozilla1.9.2a1 → ---
Version: Trunk → unspecified
Target Milestone: --- → mozilla1.9.2a1
You need to log in before you can comment on or make changes to this bug.