Gallery should not display split photo when language is set to Arabic.

RESOLVED WORKSFORME

Status

defect
P1
normal
RESOLVED WORKSFORME
5 years ago
4 years ago

People

(Reporter: swilkes, Assigned: pdahiya)

Tracking

({regression})

unspecified
2.1 S9 (21Nov)
x86
macOS
Bug Flags:
in-moztrap +

Firefox Tracking Flags

(blocking-b2g:2.2+, ux-b2g:2.2, b2g-v2.1 affected)

Details

Attachments

(2 attachments)

(Reporter)

Description

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

Updated

5 years ago
Assignee: nobody → pdahiya
(Assignee)

Comment 2

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

Comment 3

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

Updated

5 years ago
Target Milestone: --- → 2.1 S9 (21Nov)
(Assignee)

Updated

5 years ago
Duplicate of this bug: 1088407
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-

Updated

5 years ago
Keywords: regression
(Assignee)

Comment 9

5 years ago
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.
Flags: needinfo?(pdahiya)
(Assignee)

Comment 13

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

Updated

5 years ago
Flags: needinfo?(pdahiya)
(Assignee)

Comment 14

5 years ago
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Reporter)

Comment 15

4 years ago
Posted image 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.
(Reporter)

Updated

4 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Reporter)

Comment 16

4 years ago
Setting this to block 2.2 since it's a regression.
blocking-b2g: - → 2.2+
(Assignee)

Comment 17

4 years ago
(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!
Flags: needinfo?(swilkes)
Test case has been added in moztrap:

https://moztrap.mozilla.org/manage/case/15913/
Flags: in-moztrap+
Can we update the target milestone? Thanks.
Flags: needinfo?(hkoka)
Priority: -- → P1
(Reporter)

Comment 20

4 years ago
Punam, was your device set to Arabic when you looked at the Gallery? Our build IDs match.
Flags: needinfo?(swilkes)
(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
Flags: needinfo?(pdahiya)
(Assignee)

Comment 22

4 years ago
(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
Flags: needinfo?(pdahiya)
(Assignee)

Updated

4 years ago
Flags: needinfo?(swilkes)
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
Flags: needinfo?(hkoka)
(Reporter)

Comment 24

4 years ago
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?
Flags: needinfo?(swilkes)
(Assignee)

Comment 25

4 years ago
(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
Last Resolved: 5 years ago4 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.