Closed Bug 1095832 Opened 5 years ago Closed 5 years ago
[RTL][Video] Progress slider position moves opposite from slider progress
When the language is Arabic, the slider position element moves to the right as the slider progress bar moves to the left. See Attached.
Comment on attachment 8522595 [details] [review] Github PR: https://github.com/mozilla-b2g/gaia/pull/26127 This looks good to me, but: 1) consider dropping the ready() code. I think it is just as fast to use navigator.mozL10n.language.direction and you don't need to cache changes to that in a global variable. (You would want to do that if you had to check document.documentElement.dir, probably) 2) Do you need to make the same or a similar change in view.js? If so, I'd suggest you roll that into this patch. Optional: 3) You're probably going to be coming back to this play head to fix things like margins for RTL. So if you're going to have to start messing around with the playHead CSS, Here's another way you might get the play head to move how you want it.... Instead of using absolute positioning and setting the left or right property, you could make the play head part of the regular flow (or maybe use relative) and move it by setting -moz-margin-start. That should move it right in ltr locales and move it left in rtl locales, and your JS wouldn't have to care about the direction. Or if elapsedTime and playHead were laid out with normal flow in the slider wrapper, (and playHead had a slightly negative -moz-margin-start), then setting the width of elapsedTime would also move the play head. (Though maybe that would cause more reflows than we want...)
I dropped the ready() code, I agree it doesn't add anything useful. I also updated the patch to include the view.js change (based on the change to video.js).
feature-b2g: --- → 2.2+
Target Milestone: --- → 2.2 S1 (5dec)
Comment on attachment 8522595 [details] [review] Github PR: https://github.com/mozilla-b2g/gaia/pull/26127 Converting my review to r- because I took another look at this and realized that you haven't updated the code for dragging the slider. In LTR this just works, but in RTL, the time bar moves in the opposite direction of my finger and the thumb itself doesn't move at all as I drag. And just tapping on the progress bar moves the thumb to the opposite position of what I tapped. It seems to me that those issues should be fixed in this patch since they are so closely related. If you'd prefer to file a new bug to fix them, however, I'm okay with that. Also, in RTL the margins for the elapsed time and duration are wrong. The elapsed time is on the right of the slider as it should be, but there is too much space to the left of the time. And the total time is on the left as it should be, but there is not enough space on the right of the duration and the thumb overlaps it when we reach the end of the playback. You can file a new bug for this if you want. But it might be simplest to just fix it here. I'm guessing it will be a matter of replacing margin-left with -moz-margin-start and margin-right with -moz-margin-end.
Attachment #8522595 - Flags: review+ → review-
Ah. I just saw that you've got a patch in bug 1102611 for the margin issue. I still think you should handle the dragging and tapping issue in this bug, though.
Comment on attachment 8522595 [details] [review] Github PR: https://github.com/mozilla-b2g/gaia/pull/26127 I've updated the PR to address moving the slider in RTL locales (thanks for catching that).
Attachment #8522595 - Flags: review- → review?(dflanagan)
Comment on attachment 8522595 [details] [review] Github PR: https://github.com/mozilla-b2g/gaia/pull/26127 Update to view.js is in progress...
Comment on attachment 8522595 [details] [review] Github PR: https://github.com/mozilla-b2g/gaia/pull/26127 Stylistically, I don't care for the use of the ?: operator in this patch, especially not inside of square brackets. When a ?: expression is too long to fit on one line, I think it becomes too hard to read, and think that just using an if statement would be simpler. But that is a matter of style, and it is okay to just get this patch landed like this if you want to.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Test case has been added in moztrap: https://moztrap.mozilla.org/manage/case/15348/
This issue is verified pass on latest flame 2.2&3.0 and Nexus 5 2.2&3.0 build. The STR is as follow: Precondition:One or more videos in device. 1.Set your phone language to Arabic. 2.Launch Video and select a video to play. 3.Observe the progress bar. **Slider position element and slider progress bar move to left at the same time. See attachment:Verify1_Flame 2.2&3.0_N5 2.2&3.0_Pass.mp4 Reproducing rate:0/10 Device:Flame 2.2 build (Pass) Build ID 20150525162504 Gaia Revision 144673a413586f98b5e2c27b781c1a539611f754 Gaia Date 2015-05-25 02:01:14 Gecko Revision https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/115112d51e08 Gecko Version 37.0 Device Name flame Firmware(Release) 4.4.2 Firmware(Incremental) eng.cltbld.20150525.202102 Firmware Date Mon May 25 20:21:12 EDT 2015 Bootloader L1TC000118D0 Device:Flame 3.0 build (Pass) Build ID 20150525160205 Gaia Revision 5bcc08a732163087999251b523e3643db397412c Gaia Date 2015-05-24 14:44:40 Gecko Revision https://hg.mozilla.org/mozilla-central/rev/b6623a27fa64 Gecko Version 41.0a1 Device Name flame Firmware(Release) 4.4.2 Firmware(Incremental) eng.cltbld.20150525.192755 Firmware Date Mon May 25 19:28:07 EDT 2015 Bootloader L1TC000118D0 Device:Nexus 5 2.2 build (Pass) Build ID 20150525002504 Gaia Revision 144673a413586f98b5e2c27b781c1a539611f754 Gaia Date 2015-05-25 02:01:14 Gecko Revision https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/c4db2af40b1b Gecko Version 37.0 Device Name hammerhead Firmware(Release) 5.1 Firmware(Incremental) eng.cltbld.20150525.041303 Firmware Date Mon May 25 04:13:19 EDT 2015 Bootloader HHZ12f Device: Nexus 5 3.0 build (Pass) Build ID 20150525160205 Gaia Revision 5bcc08a732163087999251b523e3643db397412c Gaia Date 2015-05-24 14:44:40 Gecko Revision https://hg.mozilla.org/mozilla-central/rev/b6623a27fa64 Gecko Version 41.0a1 Device Name hammerhead Firmware(Release) 5.1 Firmware(Incremental) eng.cltbld.20150525.192207 Firmware Date Mon May 25 19:22:24 EDT 2015 Bootloader HHZ12f
Status: RESOLVED → VERIFIED
QA Whiteboard: [MGSEI-Triage+]
You need to log in before you can comment on or make changes to this bug.