900433 - Disabling menubar in customization mode, then exiting customization mode, breaks browser

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[:Gijs (he/him)]

STR:

0) toggle menubar on using the toolbox's contextmenu
1) open customize mode
2) toggle menubar off
3) close customize mode

ER:
We're fine

AR:
We break the world because we try to unwrap the toolbar's wrapper which somehow has no menubar-items child in it, which causes us to throw exceptions and break all over the place. Oops. Not sure why the menubar-items are gone from the wrapper though... :-\

(see also: bug 886030)

Comment 1

[:Gijs (he/him)]

P1 because bad breakage sadfaces.

Comment 2

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1: deal with toolbar item wrapper magic on hide/ unhide

Alright, I noticed that customizable widgets inside toolbars are not unwrapped when it's hidden, causing weirdness. For instance, the currentset attribute is persisted including 'wrapper-toolbar-menu', which is not good I think.

The problem with the patch I posted above is that it fixes the problem, sure, but does so rather inelegantly: unwrapping ALL toolbars, remove the customizing attribute and wrap ALL toolbars again. I'd rather do this specifically per toolbar, but that's something I'd appreciate your feedback on.

Thanks!

Comment 3

[Blair McBride [:Unfocused] (UNAVAILABLE)]

The old customization code (customizeToolbar.js) doesn't handle this - why does it break for us, but not in the old code?


Also, related bug: We shouldn't be wrapping the menubar items at all on OSX.

Comment 4

[Mike de Boer [:mikedeboer]]

(In reply to Blair McBride [:Unfocused] from comment #3)
> The old customization code (customizeToolbar.js) doesn't handle this - why
> does it break for us, but not in the old code?

Have you tried hiding the menubar or bookmarks bar on Linux or Windows with Fx running the old cust. code? I was in for a surprise when I did; it...
* doesn't hide the toolbar at all, or
* hides the menubar always beyond repair, or
* hides it, but jumps back after exiting cust. mode.

IMHO, this part is completely broken. I could not find a bug report yet, so I guess none of our users have found out about this yet ;)

So as another suggestion: why don't we just make 'context' one of the attributes to restore after unwrapping, so that the context menu doesn't work at all during cust. mode? It adds unnecessary complexity during a mode where the user is focused on rearranging the contents of their UI setup, of which the ability to (un)hide a toolbar is not an integral part of the workflow. Or am I assuming too much here?

> Also, related bug: We shouldn't be wrapping the menubar items at all on OSX.

Sure, but I haven't seen this break anything on OSX just yet. So, if changed, that might be considered an optimization.

Comment 5

[:Gijs (he/him)]

(In reply to Mike de Boer [:mikedeboer] from comment #4)
> So as another suggestion: why don't we just make 'context' one of the
> attributes to restore after unwrapping, so that the context menu doesn't
> work at all during cust. mode? It adds unnecessary complexity during a mode
> where the user is focused on rearranging the contents of their UI setup, of
> which the ability to (un)hide a toolbar is not an integral part of the
> workflow. Or am I assuming too much here?

We considered disabling the context menu in bug 886030 and decided against it (for rationale, see that bug), and worked to make it work. I'm 99% sure this used to work when that bug landed. I don't know why that work is now broken (or when it broke), but that is what needs investigating.

Comment 6

[Mike de Boer [:mikedeboer]]

(In reply to :Gijs Kruitbosch from comment #5)
> We considered disabling the context menu in bug 886030 and decided against
> it (for rationale, see that bug), and worked to make it work. I'm 99% sure
> this used to work when that bug landed. I don't know why that work is now
> broken (or when it broke), but that is what needs investigating.

I don't see a consideration, nor rationale in that bug. Was this in another bug, perhaps? Or was it an otr discussion?

Comment 7

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Mike de Boer [:mikedeboer] from comment #4)
> (In reply to Blair McBride [:Unfocused] from comment #3)
> > The old customization code (customizeToolbar.js) doesn't handle this - why
> > does it break for us, but not in the old code?
> 
> Have you tried hiding the menubar or bookmarks bar on Linux or Windows with
> Fx running the old cust. code?

Heh... I did, and it seemed to work fine for me (that's why I asked). Go figure.

Comment 8

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 790183 [details] [diff] [review]
Patch v1: deal with toolbar item wrapper magic on hide/ unhide

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

f- on the basis that this is using a sledgehammer on a drawing board pin. Also, it's not clear to me *why* un-wrapping/re-wrapping solves this.

Not convinced just hiding the context menu is an acceptable solution either - IMO hiding/showing toolbars is part of the customization workflow. If anything, I'd argue that being able to do that should *only* be available in customization mode.

Comment 9

[:Gijs (he/him)]

Attachment: properly remove transitionend function

Let's at least get this sorted for sanity's sake...

Comment 10

[Mike de Boer [:mikedeboer]]

Comment on attachment 792766 [details] [diff] [review]
properly remove transitionend function

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

\o/ good find!

Comment 11

[:Gijs (he/him)]

Comment on attachment 792766 [details] [diff] [review]
properly remove transitionend function

https://hg.mozilla.org/projects/ux/rev/7023dd816d6a

Comment 12

[Mike de Boer [:mikedeboer]]

As per a bit of magic, Gijs-magic to be precise, this bug has magically resolved itself when attachment 792766 [details] [diff] [review] is applied.

I think it's time for a little dance. http://goo.gl/vD5GQu

Comment 13

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/7023dd816d6a