871296 - "Undo Close Tab" breaks customization mode

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Alejandro Rodriguez [:Alopepeo]]

1. Open a custom page in a new tab (I used the Home Page).
2. Enter customization mode.
3. Close the "Customize UX" tab from the close button. You should go back to your previous tab (In this case the Home Page).
4. Right click on your custom tab and then on "Undo Close Tab". 
5. Close the restored tab ("Customize UX").

Result:
The customization tab is still there but now it is merged with your previous tab.

Expected result:
The customization tab should be properly closed and it should leave your previous tab untouched.

Comment 1

[u428464]

Can reproduce.

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

This also shows itself if you use Restore Session and had a Customize tab opened. It looks like we are not running the initialization code for customization. We'll most likely need to hook in to the pageshow event handler for this.

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

The palette and other items weren't getting unwrapped and cleaned up if the tab got closed. "Undo close tab" then re-wrapped some items, etc. Things got broken.

Pretty simple to just make sure that exit() gets called when the document is unloaded. Normally the "unload" event is discouraged because it disables the use of the bfcache, but I don't think we want to keep the Customization page alive in the bfcache.

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 751974 [details] [diff] [review]
Patch

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

::: browser/components/customizableui/src/CustomizeMode.jsm
@@ +146,5 @@
>      this.visiblePalette.addEventListener("dragexit", this);
>      this.visiblePalette.addEventListener("drop", this);
>      this.visiblePalette.addEventListener("dragend", this);
>  
> +    window.addEventListener("unload", this.exit.bind(this));

I'm not sure why, but I can't get |window.addEventListener("unload", this);| along with |handleEvent| to work. |handleEvent()| is never called for the "unload" event.

Comment 5

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

Comment on attachment 751974 [details] [diff] [review]
Patch

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

Hm. This doesn't seem to fix the problem described in comment 0. If I Undo Close Tab for about:customizing, my browser still seems to break by not letting me exit customization mode.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v2

This should work and it makes more sense. When we restore a closed tab, due to how the tabs switch focus, we actually end up entering, exiting, entering, and exiting. We had code to build the palette in the transitionend event handler after entering, which doesn't fire when the transition doesn't happen in this case (since it's toggling so fast).

Also, we weren't properly depopulating the palette.

Comment 7

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

Comment on attachment 753490 [details] [diff] [review]
Patch v2

Sorry Jared, I think bug 870011 just bit-rotted you. :/

Can you update this patch?

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v2 (rebased)

Comment 9

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

Comment on attachment 754243 [details] [diff] [review]
Patch v2 (rebased)

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

Looks good to me. Thanks Jared!

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/projects/ux/rev/183746f613a4

Comment 11

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/183746f613a4