912351 - Make sure we hide the palette before removing items from the palette

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

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

Attachment: Patch

The patch in bug 907412 depopulates the palette before running the transition, but it is easy to see the items being removed from the palette one-by-one. Oddly enough, before the items are removed from the palette, the palette has .hidden set to true. However, it appears that this style change is not flushed before the items are removed so it doesn't have much effect.

I tried doing:
> this.visiblePalette.hidden = true;
> this.visiblePalette.offsetHeight;
to force the layout flush, but that didn't work for some reason.

The attached patch however does visibly hide the palette during this operation.

Comment 1

[:Gijs (he/him)]

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

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

::: browser/components/customizableui/src/CustomizeMode.jsm
@@ +341,5 @@
>    },
>  
>    depopulatePalette: function() {
>      return Task.spawn(function() {
> +      this.visiblePalette.style.display = "none";

Can't you leave this as setting .hidden and then leave a comment regarding this display getting as forcing a reflow? Internally, the hidden property seems to just set hidden="true" on the widget, and then we style that as display: none from xul.css.

Comment 2

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

(In reply to :Gijs Kruitbosch from comment #1)
> Can't you leave this as setting .hidden and then leave a comment regarding
> this display getting as forcing a reflow?

Yeah, that should be no problem.

> Internally, the hidden property
> seems to just set hidden="true" on the widget, and then we style that as
> display: none from xul.css.

Right, this is why I don't understand why the offsetHeight flush didn't work, but perhaps it is an issue related to requesting offsetHeight and not related to the .hidden property.

Comment 3

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

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

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

(Re-requesting review)

(In reply to :Gijs Kruitbosch from comment #1)
> Can't you leave this as setting .hidden and then leave a comment regarding
> this display getting as forcing a reflow? Internally, the hidden property
> seems to just set hidden="true" on the widget, and then we style that as
> display: none from xul.css.

I went back and tried using .hidden=true but it isn't hiding the elements fast enough to stop seeing them disappear. Could this be a bug that came with that big XBL rewrite?

I have no issue with adding the comment by the way :)

Comment 4

[:Gijs (he/him)]

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

(In reply to Jared Wein [:jaws] from comment #3)
> Comment on attachment 799282 [details] [diff] [review]
> Patch
> 
> Review of attachment 799282 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (Re-requesting review)
> 
> (In reply to :Gijs Kruitbosch from comment #1)
> > Can't you leave this as setting .hidden and then leave a comment regarding
> > this display getting as forcing a reflow? Internally, the hidden property
> > seems to just set hidden="true" on the widget, and then we style that as
> > display: none from xul.css.
> 
> I went back and tried using .hidden=true but it isn't hiding the elements
> fast enough to stop seeing them disappear. Could this be a bug that came
> with that big XBL rewrite?

I don't know. Can you add a comment before it and file a bug about it? Bonus points for testcases, but this is pretty weird. Ideally, we shouldn't have to do this the hacky way.

Also, can you verify that this plays well with the initial show? I thought the palette started with hidden="true"? I would have thought that'd mean it wouldn't show up at all; how does this work? Am I simply misremembering?

r+ given that the 'better way' doesn't actually work... :-\

(and assuming there's no inconsistency as noted in the paragraph above)

Comment 5

[:Gavin Sharp [email: [e-mail]]]

(In reply to Jared Wein [:jaws] from comment #3)
> I went back and tried using .hidden=true but it isn't hiding the elements
> fast enough to stop seeing them disappear. Could this be a bug that came
> with that big XBL rewrite?

You're saying you saw a behavior difference between setting .hidden=true and setting .style.display = "none", with no other changes? That sounds like a serious bug worth getting a testcase for and filing, as Gijs suggests.

Comment 6

[:Gijs (he/him)]

The difference is because in browser/themes/shared/customizableui/customizeMode.inc.css, we set display: block with an ID rule, and that overrides the attribute-based rule: https://mxr.mozilla.org/projects-central/source/ux/browser/themes/shared/customizableui/customizeMode.inc.css#124


r=me if you change the patch to use .hidden instead of the style attribute messing, and change the CSS selector in question to use :not([hidden]) or :not([hidden="true"]) if the latter is necessary to get it to work. (easiest way to check is just use the browser console and set gNavToolbox.palette.hidden, and see what happens)

Comment 7

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

https://hg.mozilla.org/projects/ux/rev/e5e735235d91

Comment 8

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

Thank you Gijs for investigating this!

Comment 9

[:Gijs (he/him)]

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