962639 - [Gallery] Select a picture and then change orientations to landscape, shows parts of the previous and next pictures

Status: RESOLVED FIXED

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

Description

[Punam Dahiya [:pdahiya]]

If you select a picture and then change orientations to landscape you see the parts of the previous and next pictures in gallery. Steps to replicate

1. Click on a picture in thumbnails list

2. Change orientation to landscape

Actual: Parts of previous and next picture shows

Expected: Picture should open centered in landscape mode without showing parts of previous and next picture

This issue is replicable in below environment
Os Version - 1.4.0.0 - prerelease
BuildId:20140117041037
Platform - 29.0a1
Device - Hamachi
Gaia Master - 811110c87e1c985c564210d6ca3797f42f0dd0e2



This issue is not seen in v1.3

Comment 1

[Punam Dahiya [:pdahiya]]

On investigating this issue seems to be fallout of flatfish changes in  bug 928254

As part of flatfish changes, we are using screenlayoutchange instead of resize handler to handle change in orientation.

We are using setFramesPosition to position next and previous frame correct distance from current frame
https://github.com/mozilla-b2g/gaia/commit/8521e1f89e55b5b2508a0bcec878e3505b1e800b#diff-508f3a8a4cb447602009081be5eb9236R1208

The issue is when setFramesPosition method is called from screenlayoutchange handler, it's not able to calculate the correct window.innerWidth for that orientation
https://github.com/mozilla-b2g/gaia/blob/master/apps/gallery/js/frames.js#L39

I propose using screenlayoutchange handler for changes specific to video 
https://github.com/mozilla-b2g/gaia/commit/8521e1f89e55b5b2508a0bcec878e3505b1e800b#diff-508f3a8a4cb447602009081be5eb9236R1195
and keep resize handler for handling resizeFrames and setFramesPosition.

Comment 2

[Punam Dahiya [:pdahiya]]

Update 
https://github.com/mozilla-b2g/gaia/blob/master/apps/gallery/js/frames.js#L39 in Comment1 should be 

https://github.com/mozilla-b2g/gaia/blob/master/apps/gallery/js/frames.js#L390

Comment 3

[Punam Dahiya [:pdahiya]]

Attachment: PR with fix of showing parts of prev and next image in landscape mode

Hi John

The attached PR fixes the issue by using resizeHandler to call setFramesPosition. I have tested the fix on Hamachi device but am not able to test in on tablet.

Please review and let me know if you see any changes. Thanks

Comment 4

[John Hu [:johnhu][:johu][:醬糊小弟]]

Punam,

There is a bug 958197 which is related to this one. This issue is caused by the enablement of APZ. If you turned that off, you will not have this issue. I will view this patch as a work around for that bug.

Two information in your PR and comment here:
1. two resize events => this is also caused by APZ. See the test app in bug 961636.
2. tablet issue, you may use Firefox Nightly to open your app and resize the screen to 1280x800 (if you have phone UI around your main screen, you need to use 1312x861). And you can test your patch in tablet.

Comment 5

[John Hu [:johnhu][:johu][:醬糊小弟]]

Comment on attachment 8363796 [details] [review]
PR with fix of showing parts of prev and next image in landscape mode

Thanks Punam,
I had saw the PR. I don't think we need to have the patches of other bugs inside this one, maybe you don't know those bugs before. So, I just clear the flag. When you finish it, please put the review flag again.

Comment 6

[Punam Dahiya [:pdahiya]]

Comment on attachment 8363796 [details] [review]
PR with fix of showing parts of prev and next image in landscape mode

Thanks John for feedback and groundwork you have done in finding root cause of this issue. I agree we can use this patch as a work around. I have updated the patch with your feedback. Please review

Comment 7

[John Hu [:johnhu][:johu][:醬糊小弟]]

Comment on attachment 8363796 [details] [review]
PR with fix of showing parts of prev and next image in landscape mode

Punam,

Thanks for finding the reason of unexpected error message. I prefer we write it in the comment and please add the bug number of APZ to comment if it is a work around.

Comment 8

[Punam Dahiya [:pdahiya]]

Thanks John for the review, I have updated the PR with the comment. For some reason I am not seeing the merge button. Can you please help land the PR to master. Thanks

Comment 9

[John Hu [:johnhu][:johu][:醬糊小弟]]

merged to master:
https://github.com/punamdahiya/gaia/commit/fba3615444762a62a9b8d2d9800131dcb82614bd