831890 - [B2G][GALLERY] Crop Handles (Markers) disappear after changing orientations.

Status: RESOLVED FIXED

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

Description

[mlevin]

Suite Name: Gallery
Test Case: N/A (Found during ad hoc testing)
Description: After going into edit mode/crop of a photo in Gallery with the Unagi held in the portrait mode rotate the Unagi 90° to landscape mode and crop handles do not work or appear in the wrong place.

Repro:
1) Update to Unagi build ID 20130117070201 Kernel Dec 5
Gecko: 35f645b83289
Gaia: 2794faeec9da
2) Open an existing photo in the gallery while the Unagi is held in portrait manner.
3) Click the edit button then the crop button.
4) Choose 3:2
5) Rotate the Unagi 90° to landscape position.

Expected:
Crop handles to appear when touching the correct area in the crop view.

Actual:
Crop handles either never appear or they appear in the wrong place.

Repro frequency:
(4/4 on 1 Unagi)
(1/1 on another Unagi)

Notes:
See Screenshot Attachment.

Comment 1

[mlevin]

Attachment: Bug 831890

Comment 2

[Chris Kreinbring [:CKreinbring]]

Even worse on Unagi 1.2 mozilla RIL.  Rotating to landscape mode from portrait mode (or vice versa) causes the device to crash.  

Crash ID: bp-81b2b1c2-abbd-45e3-9c57-d7f992130809

Build ID: 20130809040201
Gecko: http://hg.mozilla.org/mozilla-central/rev/e33c2011643e
Gaia: c354940fbe112bcf2d90e2a5654ad1824f3a2348
Platform Version: 26.0a1

A new bug has been written:  bug 903680

Comment 3

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Still ocurs : 
Gaia      574f79512a7b8a9ab99211b16a857ab812d7994e
Gecko     http://hg.mozilla.org/mozilla-central/rev/599100c4ebfe
BuildID   20131220040201
Version   29.0a1
ro.build.version.incremental=eng.archermind.20131114.105818
ro.build.date=Thu Nov 14 10:58:33 CST 2013
Buri

Comment 4

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

An error may be found when we tap the cropping options while this bug affected. They may be related to each other:

E/GeckoConsole( 1954): [JavaScript Error: "TypeError: context is null" {file: "app://gallery.gaiamobile.org/js/ImageEditor.js" line: 891}]

Comment 5

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

I believe this is a regression. But this bug looks like to be recycled by comment 3.

The root cause of this bug has two parts:
1. the resized event is dispatched twice[1]. I don't know why. It is called twice at the first rotation, but once at the following rotation.
2. imageEditor.edit() is called twice[2][3] while rotating the phone and the second one cancels the first one's callback. When edit calls, we use image's onload event to call the callback[4]. The second one overrides the onload callback of the first one. So, the first one is not call which is the code to restore the crop handles.

[1] https://github.com/mozilla-b2g/gaia/blob/4736ca3c3e38a5dfa4ad3aaaeac4f3154c0ef056/apps/gallery/js/ImageEditor.js#L77
[2] https://github.com/mozilla-b2g/gaia/blob/4736ca3c3e38a5dfa4ad3aaaeac4f3154c0ef056/apps/gallery/js/ImageEditor.js#L234
[3] https://github.com/mozilla-b2g/gaia/blob/4736ca3c3e38a5dfa4ad3aaaeac4f3154c0ef056/apps/gallery/js/ImageEditor.js#L608
[4] https://github.com/mozilla-b2g/gaia/blob/4736ca3c3e38a5dfa4ad3aaaeac4f3154c0ef056/apps/gallery/js/ImageEditor.js#L567

Comment 6

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

One more thing: I had tested m-c with 2013-12-01-04-02-01. This version works. But the version of 1/8 does not work.

Comment 7

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

According to comment 6, mark this bug as regression, and regression window is wanted.

Comment 8

[Jason Smith [:jsmith]]

This isn't a regression - this bug has been present since 1.01.

Comment 9

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

I think comment 3 is another bug and not the original bug. Maybe, we should separate them as two bug and keep this one to trace the original one.

The original bug is not reproducible at version >1.2. And the comment 3 is only reproduced at master.

And I still believe the bug mentioned by comment 3 is a regression. It cannot be reproduced at latest v1.2/v1.3 build. But it can be reproduced at master.

Naoki,
May you confirm that and separate comment 3 as an another bug.

Comment 10

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Hi John, 

I can reproduce this on 1.2 : 
Gaia      c606b129a2d1647c0fc7bfb197555026e9b27f9e
Gecko     http://hg.mozilla.org/releases/mozilla-aurora/rev/c5109884ae3a
BuildID   20140110004009
Version   28.0a2
ro.build.version.incremental=eng.archermind.20131114.105818
ro.build.date=Thu Nov 14 10:58:33 CST 2013

Buri as well as on 1.4

Comment 11

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Oops.  I just realized I had used 1.3 not 1.2

I could not reproduce in 1.2; I can reproduce in 1.3 and 1.4
Gaia      539a25e1887b902b8b25038c547048e691bd97f6
Gecko     http://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/ec909be2a849
BuildID   20140110004008
Version   26.0
ro.build.version.incremental=eng.archermind.20131114.105818
ro.build.date=Thu Nov 14 10:58:33 CST 2013
Buri

Comment 12

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

(In reply to John Hu [:johnhu] from comment #5)
> The root cause of this bug has two parts:
> 1. the resized event is dispatched twice[1]. I don't know why. It is called
> twice at the first rotation, but once at the following rotation.

This is caused by APZ. When APZ disabled, the resize event is dispatched once.

Comment 13

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

(In reply to John Hu [:johnhu] from comment #5)
> The root cause of this bug has two parts:
> 2. imageEditor.edit() is called twice[2][3] while rotating the phone and the
> second one cancels the first one's callback. When edit calls, we use image's
> onload event to call the callback[4]. The second one overrides the onload
> callback of the first one. So, the first one is not call which is the code
> to restore the crop handles.

This is caused by the WebGL improvement of gallery app: https://github.com/mozilla-b2g/gaia/commit/37be00cc6253b87b96b9db9770323e0fb5f2a921

The main reason to have this symptom is that [1] is called before [2] before this patch. When the patch landed, it uses requestAnimationFrame to do update. So, the sequence is changed to [2] and [1] which clears the restoreCropRegion callback.

[1] https://github.com/mozilla-b2g/gaia/blob/4736ca3c3e38a5dfa4ad3aaaeac4f3154c0ef056/apps/gallery/js/ImageEditor.js#L234
[2] https://github.com/mozilla-b2g/gaia/blob/4736ca3c3e38a5dfa4ad3aaaeac4f3154c0ef056/apps/gallery/js/ImageEditor.js#L608

Comment 14

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

I had filed another for this issue: bug 961636. Besides two resize event while first rotation, resize event is also dispatched twice while app starts up.

(In reply to John Hu [:johnhu][:johu][:醬糊小弟] from comment #12)
> (In reply to John Hu [:johnhu] from comment #5)
> > The root cause of this bug has two parts:
> > 1. the resized event is dispatched twice[1]. I don't know why. It is called
> > twice at the first rotation, but once at the following rotation.
> 
> This is caused by APZ. When APZ disabled, the resize event is dispatched
> once.

Comment 15

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

Attachment: not to fire change event when the value is not changed.

We still have this issue when we disable APZ. The main reason to have this issue is we fire the change event even if the value doesn't change in forceSetExposure. This patch changes to fire the change event only when the value is changed in forceSetExposure.

Comment 16

[David Flanagan [:djf]]

John,

I don't understand what's going on here.  The patch you've attached is for the exposure slider and doesn't have anything to do with the crop handles.  Are you saying that the crop handles can only be fixed by fixing the extra resize event, but that we can fix a similar issue with the exposure slider? 

I remember long ago having issues with multiple resize events (when we used fullscreen mode I think). But when I got two events the first one always had window.innerWidth set to 0, so I could tell it was bad and ignore it.  Have you investigated to see if something like that happens in this case?

I'm happy to review the attached patch, I just don't know what the bug is that it is fixing.

Comment 17

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

David,

This patch is not related to multiple resize events. It's related to the finding at comment 13. Exposure slider calls imageEditor.edit() after imageEditor.resize(). So, the preview.onload handler[1] is replaced by the imageEditor.edit() function call by exposure slider before the one invoked by imageEditor.resize() .

[1] https://github.com/mozilla-b2g/gaia/blob/4736ca3c3e38a5dfa4ad3aaaeac4f3154c0ef056/apps/gallery/js/ImageEditor.js#L567

Comment 18

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

And this patch is trying to remove the useless event dispatched by exposure slider. After some investigation, I found we don't need to dispatch change event which calls imageEditor.edit() in resize and in some cases but the UI still need to be synced.

Comment 19

[David Flanagan [:djf]]

This is a regression I caused with the work in bug 934787. That bug was uplifted to 1.3, and we should consider this simple patch for 1.3 as well.  The risk is low. But I'd say that the user impact is also low since users are unlikely to rotate while cropping.  Is it too late to even be nominating things like this?

Comment 20

[David Flanagan [:djf]]

Comment on attachment 8362433 [details] [review]
not to fire change event when the value is not changed.

John,

Thanks for explaining. I understand now that the exposure slider was sending change events on orientation change even when it was not visible.

Thanks for figuring that out.  Sorry it took so long for me to review it.

Comment 21

[David Flanagan [:djf]]

Landed to master: https://github.com/mozilla-b2g/gaia/commit/de3bab0257b510df323cd2fd277ce34d9f0fc6c6

Comment 22

[Preeti Raghunath(:Preeti)]

Low user impact and hence not blocking on 1.3