1095832 - [RTL][Video] Progress slider position moves opposite from slider progress

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::Video, defect)

Description

[Russ Nicoletti [:russn]]

Attachment: Slider position moves opposite 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 1

[Russ Nicoletti [:russn]]

Attachment: Github PR: https://github.com/mozilla-b2g/gaia/pull/26127

Comment 2

[David Flanagan [:djf]]

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...)

Comment 3

[Russ Nicoletti [:russn]]

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).

Comment 4

[David Flanagan [:djf]]

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.

Comment 5

[David Flanagan [:djf]]

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 6

[Russ Nicoletti [:russn]]

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).

Comment 7

[Russ Nicoletti [:russn]]

Comment on attachment 8522595 [details] [review]
Github PR: https://github.com/mozilla-b2g/gaia/pull/26127

Update to view.js is in progress...

Comment 8

[David Flanagan [:djf]]

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.

Comment 9

[Russ Nicoletti [:russn]]

Master:

https://github.com/mozilla-b2g/gaia/commit/ad6da527cf923722c2dba3378c0f6b2438c6579c

Comment 10

[Lancy(Leave from Mozilla)]

Test case has been added in moztrap:
https://moztrap.mozilla.org/manage/case/15348/

Comment 11

[Lancy(Leave from Mozilla)]

Attachment: Verify1_Flame 2.2&3.0_N5 2.2&3.0_Pass.mp4

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