Closed Bug 1087058 Opened 6 years ago Closed 6 years ago
Gallery should not display split photo when language is set to Arabic
Flame, 35.0a1, build ID 20141008040203. * Set language to Arabic. * Take a screenshot of a single email in the email app (for example). * Go to Gallery. * Click on your screenshot in Gallery to view it full size. * Observe: Only half the image is shown. Expected: Full image should be shown, as usual.
I was also able to reproduce this issue on Flame 2.1. Result: The fullscreen mode displays only half of the image when the language is set to Arabic. Device: Flame 2.1 (319mb)(Kitkat Base)(Full Flash) BuildID: 20141104001202 Gaia: 8b0cf889ae0d48a9eb7ecdcb9b67590de45cc5e5 Gecko: 388b03efe92d Gonk: 48835395daa6a49b281db62c50805bd6ca24077e Version: 34.0 (2.1) Firmware: V188 User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.
Investigated and changing dx translate value to negative fixes the issue in rtl mode https://github.com/mozilla-b2g/gaia/blob/master/shared/js/media/media_frame.js#L494 Looking into how to best handle above update in media frames.
Hi David Asking for your review as attached patched is touching shared/js/media/media_frame.js, this patch also fixes the issue reported in bug 1088407. Thanks!
Attachment #8521534 - Flags: review?(dflanagan)
Comment on attachment 8521534 [details] [review] PR with fix of Bug 1087058 This patch solves the bug, but I don't really understand why. I set out to investigate and found some alternatives that I prefer. One is in gallery.css in the .frames style, we can just add direction:ltr so that the container that holds the image is always ltr and not affected by the locale. But that also seems heavy handed, and I don't like that either. Then I noticed that when I converted media frames to use background-image on a <div> instead of using a regular <img> element, I missed making some changes to gallery's css. So this bug is a regression caused by bug 1068986. gallery.css still has a line '.frames > img' that sets the image to be absolutely positioned. And the setPosition() function that you're patching relies on that because it positions the image relative to the upper left. But since we're not using an <img> anymore, those styles don't apply and we end up with a regularly positioned element. With an Arabic locale, we end up positioning it relative to the upper right instead of teh upper left, I guess and that is why you found you needed to switch the translation direction (or something like that.) In any case, I think a better fix for this bug is to change '.frames > img' to '.frames > .image-view'. And while you're at it, please remove the '.frames > img.swapping' styles below that since they are not needed anymore. The same change is going to be needed for the view activity in open.css. And also in Camera in apps/camera/style/preview-gallery.css (It is easy to verify that this same bug appears there.) Sadly bug 1068986 was uplifted all the way to 2.0. I'm going to nominate this as a 2.1 blocker so we can fix the regression at least in that release.
[Blocking Requested - why for this release]: I caused this bug with my patch for bug 1068986 because I changed MediaFrame to use a different element, but did not change the CSS of the apps that use MediaFrame. So far the only symptom we've seen of this regression is this one that affects RTL, but I can't be certain that it won't cause other issues. Bug 1068986 was also uplifted to 2.0 and 2.0M, so when we have a patch we might want to suggest to the maintainers of those branches that they consider taking the patch.
blocking-b2g: --- → 2.1?
I've confirmed that this bug also affects the Gallery view activity and the Camera preview function.
Comment on attachment 8521534 [details] [review] PR with fix of Bug 1087058 r- not because this patch is bad, but because in the process of the review I figured out a better patch. See above.
Attachment #8521534 - Flags: review?(dflanagan) → review-
Comment on attachment 8521534 [details] [review] PR with fix of Bug 1087058 Hi David I have updated patch to use image-view css to fix this issue in camera preview, gallery view activity and fullscreen view. I have tested and we don't need to explicitly handle .frame > img for view poster images as we are positioning videoPoster inside videoplayer.css https://github.com/mozilla-b2g/gaia/blob/master/apps/camera/style/VideoPlayer.css#L9 https://github.com/mozilla-b2g/gaia/blob/master/apps/gallery/style/VideoPlayer.css#L2 Thanks!
Attachment #8521534 - Flags: review- → review?(dflanagan)
i think we are not shipping any RTL locales in 2.1 so this is fine to get fixed in 2.2 :djf if there is an impact outside of RTL , please re-nom.
blocking-b2g: 2.1? → -
Comment on attachment 8521534 [details] [review] PR with fix of Bug 1087058 The code looks good to me. I'm assuming that you've tested carefully, especially with the camera app. When we talked you had some concerns that the old style (with "> img") also had an effect on video poster images, and that making this change would alter those. Have you tested (including in Arabic) for changes in video poster display in both gallery and camera? If you're confident in your testing then go ahead and land.
Attachment #8521534 - Flags: review?(dflanagan) → review+
Punam: Sorry I didn't read comment 9 before writing my review comment. You've answered my question. Bhavana made the right call not to make this a 2.1 blocker, I think. But should we request uplift to 2.1? Have you discovered, or can you think of, any possible symptoms of the regression I caused other than breaking RTL? If so, we should nominate the patch for uplift.
(In reply to David Flanagan [:djf] from comment #12) > Punam: Sorry I didn't read comment 9 before writing my review comment. > You've answered my question. > > Bhavana made the right call not to make this a 2.1 blocker, I think. But > should we request uplift to 2.1? Have you discovered, or can you think of, > any possible symptoms of the regression I caused other than breaking RTL? If > so, we should nominate the patch for uplift. I have seen symptoms of the regression only in RTL, I checked 2.1 and in LTR fullscreen view and flows around it works good.
Gaia-try run Gb test failure is unrelated to the css fixes in this patch Patch landed on master https://github.com/mozilla-b2g/gaia/commit/10d5580d7600dcfe721f3cdc799aa4abfc862ef8
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Seeing this on 2.2 again. Attached is a photo of how a screenshot looks on my Flame in the Gallery. The screenshot itself is fine: when I send the screenshot to my laptop via Bluetooth, the full image comes through, but in the Gallery it appears split in half. Also note that the Gallery >< icons overlap each other in the upper right corner.
Setting this to block 2.2 since it's a regression.
blocking-b2g: - → 2.2+
(In reply to Stephany Wilkes from comment #15) > Created attachment 8570703 [details] > photo(1).JPG > > Seeing this on 2.2 again. Attached is a photo of how a screenshot looks on > my Flame in the Gallery. The screenshot itself is fine: when I send the > screenshot to my laptop via Bluetooth, the full image comes through, but in > the Gallery it appears split in half. Also note that the Gallery >< icons > overlap each other in the upper right corner. I tried STR from #comment 0 with latest 2.2 BuildId:20150227002521 and screen shot opens fine in gallery. Can you please provide more details such as BuildId and STR? Thanks!
Test case has been added in moztrap: https://moztrap.mozilla.org/manage/case/15913/
Can we update the target milestone? Thanks.
Punam, was your device set to Arabic when you looked at the Gallery? Our build IDs match.
(In reply to Stephany Wilkes from comment #20) > Punam, was your device set to Arabic when you looked at the Gallery? Our > build IDs match. Adding punam for Stephany's question
(In reply to Stephany Wilkes from comment #20) > Punam, was your device set to Arabic when you looked at the Gallery? Our > build IDs match. Hi Stephany The language was set to Arabic, here's the video link to experience seen on my flame http://youtu.be/jBWon5oKS0w Build Details Gaia-Rev eb6a5ac9081d3962198e0f4520b0743d716d7a27 Gecko-Rev https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/c8a38dcfbebc Build-ID 20150227002521 Version 37.0 The screen shot attached in #comment 15, has the footer icons reversed (starts with delete icon and ends with camera icon ), we have this experience in 2.1. It will be great if you can provide more info to reproduce this bug. Thanks
I couldn't reproduce this on latest master either with email app screenshot Gaia-Rev f34ce82a840ad3c0aed3bfff18517b3f6a0eb37f Gecko-Rev https://hg.mozilla.org/mozilla-central/rev/eea6188b9b05 Build-ID 20150302010223 Version 39.0a1
Hema and Punam, that's great. Although: why are the footer icons reversed? Per the bidi spec, the footer shouldn't change. Did it change in 2.2 for another reason?
(In reply to Stephany Wilkes from comment #24) > Hema and Punam, that's great. Although: why are the footer icons reversed? > Per the bidi spec, the footer shouldn't change. Did it change in 2.2 for > another reason? Footer icons shows reversed in RTL mode for version 2.1 and before. In 2.2 and master, RTL mode footer doesn't change and show as per bidi spec. I am marking bug resolved works for me. Please reopen the bug if you see the issue. Thanks!
Status: REOPENED → RESOLVED
Closed: 6 years ago → 6 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.