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)
Firefox OS Graveyard
Gaia::Video
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)
9.90 KB,
image/png
|
Details | |
373.44 KB,
image/png
|
Details | |
487.37 KB,
image/png
|
Details | |
263.96 KB,
image/png
|
Details | |
375.93 KB,
image/png
|
Details | |
225.25 KB,
image/png
|
Details | |
46 bytes,
text/x-github-pull-request
|
djf
:
review+
|
Details | Review |
234.06 KB,
image/png
|
Details | |
233.63 KB,
image/png
|
Details | |
5.22 MB,
video/mp4
|
Details |
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•10 years ago
|
Whiteboard: video-rtl
Assignee | ||
Comment 1•10 years ago
|
||
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•10 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•10 years ago
|
||
David, do you have any insight into issue 1) in comment 2?
Flags: needinfo?(dflanagan)
Assignee | ||
Comment 4•10 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•10 years ago
|
||
Russ, you're correct. They should not be mirrored for 2.2.
Flags: needinfo?(swilkes)
Assignee | ||
Comment 6•10 years ago
|
||
Assignee | ||
Comment 7•10 years ago
|
||
Assignee | ||
Comment 8•10 years ago
|
||
Attachment #8526394 -
Attachment is obsolete: true
Assignee | ||
Comment 9•10 years ago
|
||
Assignee | ||
Comment 10•10 years ago
|
||
Assignee | ||
Comment 11•10 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•10 years ago
|
Assignee: nobody → rnicoletti
Updated•10 years ago
|
feature-b2g: --- → 2.2+
Whiteboard: video-rtl → video-rtl [ft:media]
Target Milestone: --- → 2.2 S1 (5dec)
Comment 13•10 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)
Assignee | ||
Comment 14•10 years ago
|
||
Attachment #8528535 -
Flags: review?(dflanagan)
Comment 15•10 years ago
|
||
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-
Comment 16•10 years ago
|
||
This screenshot shows why it is not okay to force ltr on the title. The justification of the second title is incorrect.
Comment 17•10 years ago
|
||
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•10 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 19•10 years ago
|
||
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•10 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 21•9 years ago
|
||
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•9 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 23•9 years ago
|
||
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+
Updated•9 years ago
|
feature-b2g: 2.2+ → ---
Assignee | ||
Comment 24•9 years ago
|
||
Master: https://github.com/mozilla-b2g/gaia/commit/86d950990a99274e78e5761076bf2d0681d66952
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Comment 25•9 years ago
|
||
Test case has been added in moztrap: https://moztrap.mozilla.org/manage/case/15194/
Flags: in-moztrap+
Comment 26•9 years ago
|
||
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!
Updated•9 years ago
|
Status: RESOLVED → VERIFIED
QA Whiteboard: [MGSEI-Triage+]
Updated•9 years ago
|
Flags: needinfo?(nefzaoui)
You need to log in
before you can comment on or make changes to this bug.
Description
•