Closed Bug 1095826 Opened 10 years ago Closed 9 years ago

[RTL][Video] Thumbnail images do not appear

Categories

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

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED
2.2 S1 (5dec)

People

(Reporter: rnicoletti, Assigned: rnicoletti)

References

Details

(Whiteboard: video-rtl [ft:media])

Attachments

(10 files, 1 obsolete file)

When the language is Arabic, the video thumbnails do not appear, nor does the video title, duration, etc. The videos can be played by clicking where the thumbnails should appear, but the image and information does not appear.

Device: Flame
BuildID: 20141107073659
Version: 36.0a1
Gaia: 779f05fead3d009f6e7fe713ad0fea16b6f2fb31
B2G: b62ccf3228ba
Whiteboard: video-rtl
Blocks: video-rtl
It is instructive to see how the thumbnail list appears in landscape view, when language is rtl. The thumbnail element is too narrow and the thumbnail image appears with padding on the left.
It appears there are a couple of factors contributing to the behavior of the thumbnail list view in RTL:

1) The size of the thumbnail_group_container when lang is RTL is 50 pixels narrower than when the lang is LTR -- LTR 320x483 (portrait), 570x483 (landscape), RTL 270x483 (portrait), 520x483 (landscape). The result when lang is RTL is there is an empty space to the right of the thumbnail details and when the orientation is portrait the details overlap the image.
2) video.css [1] has to take into account orientation when defining padding of the thumbnail list elements. In portrait, padding should be '-moz-padding-start: 5em;'. In landscape, '-moz-padding-start: 17.5'

[1]: https://github.com/mozilla-b2g/gaia/blob/master/apps/video/style/video.css#L319-L321
David, do you have any insight into issue 1) in comment 2?
Flags: needinfo?(dflanagan)
Re-reading the BiDi guidelines, I'm not sure if the thumbnail images and details should be mirrored in RTL languages. The closest comparable screen in the BiDi guidelines I can find is the contacts index where the image and name are mirrored. Stephany, what is your guidance here?
Flags: needinfo?(swilkes)
Russ, you're correct. They should not be mirrored for 2.2.
Flags: needinfo?(swilkes)
Attachment #8526394 - Attachment is obsolete: true
Stephany, I have implemented the change to mirror the thumbnail image and details in RTL languages. It is a "POC" because I would like input from you regarding the layout of the title. I have made it ltr as that seems to make the most sense (and I didn't see any specific guidelines in the spec). Can you take a look at the "POC" screenshots attached and give me your feedback? Thanks.
Flags: needinfo?(dflanagan) → needinfo?(swilkes)
Assignee: nobody → rnicoletti
feature-b2g: --- → 2.2+
Whiteboard: video-rtl → video-rtl [ft:media]
Target Milestone: --- → 2.2 S1 (5dec)
Sorry for the delay, Russ, I have been really sick and behind on some things. These look good to me. Flagging Ahmed to get his input as well.
Flags: needinfo?(swilkes) → needinfo?(nefzaoui.ahmed)
Comment on attachment 8528535 [details] [review]
Github PR: https://github.com/mozilla-b2g/gaia/pull/26456

Giving r- here because I'm almost positive it is wrong to force the title to be left-to-right. For titles that go onto two lines but do not get truncated, they end up being left justified, which is wrong. I'll attach some screenshots below with more on this.

Also, I really don't understand why this patch works. It changes the position of the details, but doesn't do anything to change the position of the image or the "unwatched" indicator. It does work, and you haven't made the CSS any more confusing than it already is, so we could land it like this. But if you can understand the current layout, I'd prefer a patch that adds RTL support and simplifies things at the same time. I left a comment on github about the weird CSS. I spent a few minutes trying to simplify it myself and got nowhere.
Attachment #8528535 - Flags: review?(dflanagan) → review-
This screenshot shows why it is not okay to force ltr on the title. The justification of the second title is incorrect.
This screenshot shows what goes wrong with really long video titles when you format them rtl, and I assume that this is the reason you added the ltr override.  I'm guessing that the bidi algorithm is failing here, and is treating the '...' as a switch back to rtl.

Having the ellipsis on the left here actually looks good visually, but seems logically wrong since it ends up in the middle of the string.  I'm not sure how to fix it. Maybe we should just not fix it.

If we do want to fix it, I've found that putting the Unicode character \u202d (LEFT-TO-RIGHT OVERRIDE) right before the "..." of the ellipsis, or at the start of the video title will force the ellipsis to be on the right. But that seems like it will break if we every actually parse video metadata and have a title that is actually in arabic or hebrew.

Ideally, there would be a way to say "this ellipsis is attached to the text that preceeds it and should have the same direction as that text". I would have thought that there would be an easy way to acheive that, but I can't find one.

So I think I'd recommend just not worrying about the ellipsis and landing this without your ltr override. We can file a followup bug for the ellipsis if Nefzaoui tells us that it matters.
Comment on attachment 8528535 [details] [review]
Github PR: https://github.com/mozilla-b2g/gaia/pull/26456

I'm using a flexbox now for the thumbnail elements (unwatched, img, details). With the flexbox, there are no RTL-specific css statements (with the addition of a -moz-margin-start for the 'details'). With this approach, the title wrapping has the issue mentioned in comment 17 -- the ellipse is on the wrong side of the title. I would like to take the approach you recommended which is to land this patch without addressing the ellipse issue and create a bug for that if and when Nefzaoui says it matters.
Attachment #8528535 - Flags: review- → review?(dflanagan)
Comment on attachment 8528535 [details] [review]
Github PR: https://github.com/mozilla-b2g/gaia/pull/26456

I like the flexbox approach. r- because I think there are a few more changes to be made, including incorrect calculation of margins. See github for my comments.
Attachment #8528535 - Flags: review?(dflanagan) → review-
Comment on attachment 8528535 [details] [review]
Github PR: https://github.com/mozilla-b2g/gaia/pull/26456

I've updated the patch and the PR with my comments in preparation for another review.
Attachment #8528535 - Flags: review- → review?(dflanagan)
Comment on attachment 8528535 [details] [review]
Github PR: https://github.com/mozilla-b2g/gaia/pull/26456

Russ: I'm sure this patch works okay, but I've got one more round of comments on github. We've got a lot of incomprehensible CSS in our codebase, and since you're taking the time to clean up this CSS, there are a couple more things I think you should address.
Attachment #8528535 - Flags: review?(dflanagan) → review-
Comment on attachment 8528535 [details] [review]
Github PR: https://github.com/mozilla-b2g/gaia/pull/26456

David, I addressed the issue you raised about the width of the details element along with the margins/padding being inconsistent with the width of the image. I left a comment in the PR but I'm not seeing it now so I'm going to explain here. I've subtracted '23rem' to get the width of the details and added 1rem margin left and right. I'm using left/right margin instead of padding because I want to be consistent and not use margin for top/bottom and padding for left/right (the CSS was already using margin for top/bottom). With the other changes I made, the CSS is significantly cleaner now (and the display better with the consistent left/right margins). And in case you're wondering, I tested with Arabic and the rtl flow is also correct.

I commented in the PR on the other issue you raised.
Attachment #8528535 - Flags: review- → review?(dflanagan)
Comment on attachment 8528535 [details] [review]
Github PR: https://github.com/mozilla-b2g/gaia/pull/26456

r+. Thanks for making all the tweaks. 

I wonder why inline didn't work at all. Did you add the height back in for the image element?

I still think that it would be cool to use the flex property instead of width:calc(), but this way works, and there are probably better things for me to obsess over :-)
Attachment #8528535 - Flags: review?(dflanagan) → review+
feature-b2g: 2.2+ → ---
Master: 

https://github.com/mozilla-b2g/gaia/commit/86d950990a99274e78e5761076bf2d0681d66952
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Test case has been added in moztrap:
https://moztrap.mozilla.org/manage/case/15194/
Flags: in-moztrap+
Attached video Verify_Pass.mp4
This problem is verified pass on latest build of Flame 2.2, Flame 3.0, N5 2.2 and N5 3.0.
The STR is as follow:
Precondition: There's a video with a long title in device storage.
1. Launch Video app.
2. Observe the video title in video detail information.
3. Switch device to landscape mode.
4. Observe the video title in video detail information.
Actual result: In portrait mode and landscape mode, the thumbnails now on the right corner, the information on the left. The video title texts are right aligned. And a reasonable space between pictures and descriptions. 
See attachment: Verify_Pass.mp4
Rate: 0/5

Device: Flame 2.2 (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 (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 (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 (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

Thanks!
Status: RESOLVED → VERIFIED
QA Whiteboard: [MGSEI-Triage+]
Flags: needinfo?(nefzaoui)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: