[RTL][Video] Thumbnail images do not appear

VERIFIED FIXED in 2.2 S1 (5dec)

Status

Firefox OS
Gaia::Video
VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: russn, Assigned: russn)

Tracking

unspecified
2.2 S1 (5dec)
Bug Flags:
in-moztrap +

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(10 attachments, 1 obsolete attachment)

(Assignee)

Description

4 years ago
Created attachment 8519315 [details]
No thumbnail image/information

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
(Assignee)

Updated

4 years ago
Whiteboard: video-rtl
(Assignee)

Updated

4 years ago
Blocks: 1088976
(Assignee)

Comment 1

4 years ago
Created attachment 8523228 [details]
landscape thumbnail view

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

Comment 2

4 years ago
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
(Assignee)

Comment 3

4 years ago
David, do you have any insight into issue 1) in comment 2?
Flags: needinfo?(dflanagan)
(Assignee)

Comment 4

4 years ago
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)

Comment 5

4 years ago
Russ, you're correct. They should not be mirrored for 2.2.
Flags: needinfo?(swilkes)
(Assignee)

Comment 6

4 years ago
Created attachment 8526393 [details]
POC mirrored image/details -- portrait, no wrap
(Assignee)

Comment 7

4 years ago
Created attachment 8526394 [details]
POC mirroed image/details -- portrait, title wrapped
(Assignee)

Comment 8

4 years ago
Created attachment 8526396 [details]
POC mirrored image/details -- portrait, title wrapped
Attachment #8526394 - Attachment is obsolete: true
(Assignee)

Comment 9

4 years ago
Created attachment 8526397 [details]
POC mirrored image/details -- landscape, no wrap
(Assignee)

Comment 10

4 years ago
Created attachment 8526399 [details]
POC mirrored image/details -- landscape, title wrapped
(Assignee)

Comment 11

4 years ago
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)

Updated

4 years ago
Assignee: nobody → rnicoletti
(Assignee)

Updated

4 years ago
Duplicate of this bug: 1011183

Updated

4 years ago
feature-b2g: --- → 2.2+
Whiteboard: video-rtl → video-rtl [ft:media]
Target Milestone: --- → 2.2 S1 (5dec)

Comment 13

4 years ago
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-
Created attachment 8528616 [details]
ltr title in rtl locale has wrong justification

This screenshot shows why it is not okay to force ltr on the title. The justification of the second title is incorrect.
Created attachment 8528649 [details]
ellipsis on the wrong side of the rtl title

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

Comment 18

4 years ago
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-
(Assignee)

Comment 20

4 years ago
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-
(Assignee)

Comment 22

4 years ago
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+ → ---
(Assignee)

Comment 24

4 years ago
Master: 

https://github.com/mozilla-b2g/gaia/commit/86d950990a99274e78e5761076bf2d0681d66952
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Test case has been added in moztrap:
https://moztrap.mozilla.org/manage/case/15194/
Flags: in-moztrap+
Created attachment 8610337 [details]
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.