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

Status: RESOLVED WORKSFORME

(Firefox OS Graveyard :: Gaia::Gallery, defect, P1)

Description

[Stephany Wilkes]

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.

Comment 1

[Yeojin Chung [:YeojinC]]

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.

Comment 2

[Punam Dahiya [:pdahiya]]

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.

Comment 3

[Punam Dahiya [:pdahiya]]

Attachment: PR with fix of Bug 1087058

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!

Comment 5

[David Flanagan [:djf]]

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.

Comment 6

[David Flanagan [:djf]]

[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.

Comment 7

[David Flanagan [:djf]]

I've confirmed that this bug also affects the Gallery view activity and the Camera preview function.

Comment 8

[David Flanagan [:djf]]

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.

Comment 9

[Punam Dahiya [:pdahiya]]

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!

Comment 10

[bhavana bajaj [:bajaj]]

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.

Comment 11

[David Flanagan [:djf]]

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.

Comment 12

[David Flanagan [:djf]]

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.

Comment 13

[Punam Dahiya [:pdahiya]]

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

Comment 14

[Punam Dahiya [:pdahiya]]

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

Comment 15

[Stephany Wilkes]

Attachment: 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.

Comment 16

[Stephany Wilkes]

Setting this to block 2.2 since it's a regression.

Comment 17

[Punam Dahiya [:pdahiya]]

(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!

Comment 18

[Alissa（leave from Mozilla）]

Test case has been added in moztrap:

https://moztrap.mozilla.org/manage/case/15913/

Comment 19

[Kai-Chih Hu [:khu]]

Can we update the target milestone? Thanks.

Comment 20

[Stephany Wilkes]

Punam, was your device set to Arabic when you looked at the Gallery? Our build IDs match.

Comment 21

[Hema Koka [:hema]]

(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

Comment 22

[Punam Dahiya [:pdahiya]]

(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

Comment 23

[Hema Koka [:hema]]

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

Comment 24

[Stephany Wilkes]

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?

Comment 25

[Punam Dahiya [:pdahiya]]

(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!