"Undo Close Tab" breaks customization mode

RESOLVED FIXED in Firefox 28

Status

()

Firefox
Theme
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Alopepeo, Assigned: jaws)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 28
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Australis:M6])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

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

Updated

5 years ago
Blocks: 770135
Can reproduce.
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
No longer blocks: 770135
Whiteboard: [Australis:M6]
Blocks: 872617
Created attachment 751974 [details] [diff] [review]
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.
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Attachment #751974 - Flags: review?(mconley)
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 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.
Attachment #751974 - Flags: review?(mconley) → review-
Created attachment 753490 [details] [diff] [review]
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.
Attachment #751974 - Attachment is obsolete: true
Attachment #753490 - Flags: review?(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?
Attachment #753490 - Flags: review?(mconley)
Created attachment 754243 [details] [diff] [review]
Patch v2 (rebased)
Attachment #753490 - Attachment is obsolete: true
Attachment #754243 - Flags: review?(mconley)
Comment on attachment 754243 [details] [diff] [review]
Patch v2 (rebased)

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

Looks good to me. Thanks Jared!
Attachment #754243 - Flags: review?(mconley) → review+
https://hg.mozilla.org/projects/ux/rev/183746f613a4
Whiteboard: [Australis:M6] → [Australis:M6][fixed-in-ux]

Comment 11

4 years ago
https://hg.mozilla.org/mozilla-central/rev/183746f613a4
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:M6][fixed-in-ux] → [Australis:M6]
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.