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

RESOLVED FIXED

Status

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: pdahiya, Assigned: pdahiya)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

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

Updated

5 years ago
Assignee: nobody → pdahiya
(Assignee)

Comment 1

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

Comment 3

5 years ago
Created attachment 8363796 [details] [review]
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
Attachment #8363796 - Flags: review?(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 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.
Attachment #8363796 - Flags: review?(johu)
(Assignee)

Comment 6

5 years ago
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
Attachment #8363796 - Flags: review?(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.
Attachment #8363796 - Flags: review?(johu) → review+
(Assignee)

Comment 8

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

Updated

5 years ago
Flags: needinfo?(johu)
merged to master:
https://github.com/punamdahiya/gaia/commit/fba3615444762a62a9b8d2d9800131dcb82614bd
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: needinfo?(johu)
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.