1089591 - "Restore defaults" breaks customize mode

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Cornel Ionce [:noni] [Hubs QA]]

Attachment: broken_customize_mode.gif

This issue is reproducing on Windows 7 64-bit, Ubuntu 14.04 and Mac OS X 10.9 using:
Latest Nightly, build ID: 20141026030205.
Latest Aurora, build ID: 20141026004001.

STR:
1. Open Firefox
2. Click on the Menu Panel and enter Customize Mode.
3. Reorder a few items in the panel.
4. Select Restore Defaults.
5. Try to reorder items again.

Expected Results:
All of the items can be correctly reordered.

Actual Results:
After selecting "Restore defaults", the customize mode is broken.

Comment 1

[:Gijs (he/him)]

[Tracking Requested - why for this release]: serious functionality regression

Comment 2

[Alice0775 White]

Regression window(fx)
Good:
https://hg.mozilla.org/integration/fx-team/rev/9ef1c5956a05
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:35.0) Gecko/20100101 Firefox/35.0 ID:20140904093341
Bad:
https://hg.mozilla.org/integration/fx-team/rev/ca57c19e84c5
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:35.0) Gecko/20100101 Firefox/35.0 ID:20140904094337
Pushlog:
https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=9ef1c5956a05&tochange=ca57c19e84c5

Rgressed by:
ca57c19e84c5	Alex Bardas — Bug 979041 - Wrap an area (area_tabstrip) synchronously before the transition and use requestAnimationFrame. r=mconley

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Hey Alex - do you think you have some cycles to figure out what broke here?

Comment 4

[Alex Bardas :alexbardas]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #3)
> Hey Alex - do you think you have some cycles to figure out what broke here?

I think I figured it out. Before we were doing this.areas = [] at the beginning of each wrapToolbarItems. We don't want to do this anymore now, but we should clear this.areas when we call unwrapToolbarItems. I have a 1 line patch which fixes this. Let me come with a regression test for it and make everyone happy.

Comment 5

[Alex Bardas :alexbardas]

Attachment: v1 Clear areas Set after unwrapping toolbar items + regression test

Comment 6

[:Gijs (he/him)]

Comment on attachment 8513033 [details] [diff] [review]
v1 Clear areas Set after unwrapping toolbar items + regression test

Review of attachment 8513033 [details] [diff] [review]:
-----------------------------------------------------------------

You forgot to add the test file. :-)

Comment 7

[Alex Bardas :alexbardas]

Attachment: v1.1 Clear areas Set after unwrapping toolbar items + regression test

Added the test :)

Comment 8

[:Gijs (he/him)]

Comment on attachment 8513039 [details] [diff] [review]
v1.1 Clear areas Set after unwrapping toolbar items + regression test

Review of attachment 8513039 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/customizableui/test/browser_1089591_still_customizable_after_reset.js
@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

Nit: Unless you don't like to make this public domain, no need for a license header.

@@ +9,5 @@
> +  yield startCustomizing();
> +  let historyButton = document.getElementById("wrapper-history-panelmenu");
> +  let devButton = document.getElementById("wrapper-developer-button");
> +
> +  ok(historyButton && devButton, "Stuff should exist");

Nit: slightly more descriptive string than "stuff" ? :D

@@ +12,5 @@
> +
> +  ok(historyButton && devButton, "Stuff should exist");
> +  simulateItemDrag(historyButton, devButton);
> +  gCustomizeMode.reset();
> +  yield waitForCondition(function() !gCustomizeMode.resetting);

Nit: () => !gCustomizeMode.resetting

(we'll deprecate the function() foo syntax at some point, hopefully sooner rather than later)

@@ +18,5 @@
> +
> +  historyButton = document.getElementById("wrapper-history-panelmenu");
> +  devButton = document.getElementById("wrapper-developer-button");
> +  simulateItemDrag(historyButton, devButton);
> +  ok(historyButton && devButton, "Stuff should exist");

Nit: same string here.

This should probably come before the item drag, which will fail if either is null, I think...

Comment 9

[Alex Bardas :alexbardas]

Attachment: v2 Clear areas Set after unwrapping toolbar items + regression test

I've removed the license header. I only test panelmenu and not other areas, but I think it's enough for this regression.

Comment 10

[:Gijs (he/him)]

Comment on attachment 8513058 [details] [diff] [review]
v2 Clear areas Set after unwrapping toolbar items + regression test

Review of attachment 8513058 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, thanks!

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8513058 [details] [diff] [review]
v2 Clear areas Set after unwrapping toolbar items + regression test

Review of attachment 8513058 [details] [diff] [review]:
-----------------------------------------------------------------

Where does this fail without your patch? The simulateItemDrag calls shouldn't fail... will it fail because the wrappers will not exist?

If so, r=me (and no need for another round of review on this).

::: browser/components/customizableui/test/browser_1089591_still_customizable_after_reset.js
@@ +1,3 @@
> +"use strict";
> +
> +// Dragging again the elements after a reset should work

nit: "Dragging again the elements" -> "Dragging the elements again"

@@ +1,4 @@
> +"use strict";
> +
> +// Dragging again the elements after a reset should work
> +add_task(function() {

function* since this is a generator.

@@ +18,5 @@
> +  simulateItemDrag(historyButton, devButton);
> +  yield endCustomizing();
> +});
> +
> +add_task(function asyncCleanup() {

function *

Comment 12

[Alex Bardas :alexbardas]

Attachment: Clear areas Set after unwrapping toolbar items + regression test

Added Gijs as reviewer. Will push to try when it opens.

Comment 13

[Alex Bardas :alexbardas]

Attachment: Clear areas Set after unwrapping toolbar items + regression test

Applied Mike's suggestions. Without the fix in the module, the test would fail because it doesn't find the wrappers.

Comment 14

[Alex Bardas :alexbardas]

https://tbpl.mozilla.org/?tree=Try&rev=dc13ce2487a3

Comment 15

[Alex Bardas :alexbardas]

There were some nits, mostly because of copy-paste, but try run looks good. I'm marking it for checkin.

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/e6c622c61207

Comment 17

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://treeherder.mozilla.org/ui/logviewer.html#?job_id=1031907&repo=fx-team

Comment 18

[Alex Bardas :alexbardas]

(In reply to Carsten Book [:Tomcat] from comment #17)
> sorry had to back this out for test failures like
> https://treeherder.mozilla.org/ui/logviewer.html#?job_id=1031907&repo=fx-team

I made a small change to the test (triggering the reset again before calling endCustomizing()). I'll trigger the bc1 multiple times, hopefully we'll get rid of the oranges.

Comment 19

[Alex Bardas :alexbardas]

Attachment: v3 Clear areas Set after unwrapping toolbar items + regression test

I couldn't reproduce the error but I added the reset at the end of the test and re-ran the try run tests (also re-triggered bc1 multiple times):

https://tbpl.mozilla.org/?tree=Try&rev=7581c3732077

Comment 20

[:Gijs (he/him)]

Comment on attachment 8514055 [details] [diff] [review]
v3 Clear areas Set after unwrapping toolbar items + regression test

Please reland the earlier patch; I think the failures were nothing to do with this patch. See bug 1091135, bug 1091437, bug 1091561.

Comment 21

[Alex Bardas :alexbardas]

Attachment: Clear areas Set after unwrapping toolbar items + regression test

Ok, uploading again the previous version because the try error was unrelated to this patch.

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/c24ae4a27f86

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c24ae4a27f86

Comment 24

[Cornel Ionce [:noni] [Hubs QA]]

Confirming this fix on latest Nightly, build ID: 20141102030204.

Comment 25

[Alex Bardas :alexbardas]

Comment on attachment 8514400 [details] [diff] [review]
Clear areas Set after unwrapping toolbar items + regression test

Approval Request Comment
[Feature/regressing bug #]: 979041
[User impact if declined]: Affects users which press "Restore Defaults" on customize mode. After that, drag & drop doesn't work anymore.
[Describe test coverage new/current, TBPL]: 1 new test file + was manually tested & verified
[Risks and why]: Almost none, only one line of production code was added.
[String/UUID change made/needed]: None

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4f57e28c9ef6

Comment 27

[Cornel Ionce [:noni] [Hubs QA]]

The "Restore Defaults" button no longer breaks customize mode in Firefox 35.0a2, build ID: 20141116004002.