Closed Bug 489631 Opened 15 years ago Closed 15 years ago

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

Categories

(Toolkit :: Video/Audio Controls, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: tomer, Assigned: ehsan.akhgari)

References

(Blocks 1 open bug, )

Details

(Keywords: fixed1.9.1, intl, rtl)

Attachments

(7 files, 2 obsolete files)

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
Blocks: video
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.
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.
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.
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
Attached patch Patch (v1) (obsolete) — Splinter Review
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)
(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.
Attached patch Patch (v2) (obsolete) — Splinter Review
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)
I think this should block, the controls look really messed up and do not function properly.
Flags: blocking1.9.1?
(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. :)
Attached patch Patch (v3)Splinter Review
(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)
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.
Attachment #378273 - Flags: review?(mconnor) → review+
Comment on attachment 378273 [details] [diff] [review]
Patch (v3)

r=me, I trust dolske on the tests.
http://hg.mozilla.org/mozilla-central/rev/b16fcc7f2031
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
(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+
Depends on: 501754
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.