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.

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

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

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}]

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

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.

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

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

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.

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

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

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

(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

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.

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.

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.

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

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.

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 on attachment 8362433 [details] 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.

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

Low user impact and hence not blocking on 1.3