1214330 - Customization footer twitches (teleports to the top and back) when I click "Restore defaults" or "Cancel"

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[arni2033]

Attachment: screenshot - 'Restore defaults' and 'Undo' are broken 2015.10.07 18-41-52.png

STR:   (Win7_64, Nightly 44, 32bit, ID 20151012030612, new profile)
1. Click australis menu (≡)
2. Click "Customize"
3. Place Pocket button to the menu
4. Click "Restore defaults"
5. Click "Undo"

Result:   After Step 4 and after Step 5 Customization palette disappears for 0.1 second, and
          that causes twitching of #customization-footer (it teleports to the top) [see screenshot]

Expectations: No twitching. There's already enough twitching things in firefox browser

I found out that customization palette becomes invisible during reordering of elements. Looks like it gains "display:none;". So, the easiest solution is to set "visibility:hidden;" to customization palette during reordering.

Here's the line that hides customization palette:
>    depopulatePalette: function() {
>      return Task.spawn(function() {
> ->     this.visiblePalette.hidden = true;

> URL:   https://hg.mozilla.org/releases/mozilla-esr38/file/895579563ce0/browser/components/customizableui/CustomizeMode.jsm#l784

Comment 1

[:Gijs (he/him)]

Are you able to write a patch for this?

Comment 2

[arni2033]

I think I can't do that at least for about 2 weeks (because compiling stuff requires much time)
But I tested this CSS and it was OK.
>   #customization-palette { /* bug 1214330: remove :not([hidden]) in order NOT to change the styling of the palette when it becomes hidden */
>     margin-bottom: 25px;
>   }
>   #customization-palette { /* bug 1214330: remove :not([hidden]) in order to override normal [hidden] styling (display:none;) And NOT to change the styling of the palette when it becomes hidden */
>     display: block;
>     flex: 1 1 auto;
>     overflow: auto;
>     min-height: 3em;
>   }
>   
>   #customization-palette[hidden]
>   {
>     visibility:hidden; /* bug 1214330: only hide it to prevent twitching of customization-footer */
>   }
Currently chrome://browser/content/browser.css has 2 selectors   #customization-palette:not([hidden])   , which should be replaced by simply   #customization-palette   . And one rule is required to "hide" the palette:   #customization-palette[hidden]{visibility:hidden}

These changes are enough, because id-selector is more specific than attribute-selector [hidden].

// But even after this bugfix customization performance is far from ideal with all those blinking items

Comment 3

[:Gijs (he/him)]

(In reply to arni2033 from comment #2)
> I think I can't do that at least for about 2 weeks (because compiling stuff
> requires much time)

For future reference, https://groups.google.com/forum/?fromgroups=&hl=en#!topic/firefox-dev/eECPFbT5sv0 may help here.

I'll try to look at making your changes into a usable patch sometime tomorrow.

Comment 4

[:Gijs (he/him)]

(I didn't get to this today, sorry - one of the first on my list for tomorrow... though I suspect the hidden thing also comes in when we go into/out of customize mode, which means it might be safer (for perf etc.) for the reset/undo code to set visibility:hidden directly (and remove it) rather than changing the meaning of [hidden]. I'll look properly tomorrow.)

Comment 5

[:Gijs (he/him)]

Baseline try revision: bde8be02e3e8
try push with patch: remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=e7d5d0c098fb

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=bde8be02e3e8&newProject=try&newRevision=e7d5d0c098fb

we'll need to wait for builds to finish and will then need to do retriggers, but then the above should be helpful in seeing perf impact.

FWIW, the bug that introduced the code here was bug 912351... while that was a visual fix, I'm not sure what the perf implications are for keeping all the nodes around...

Comment 6

[:Gijs (he/him)]

s/nodes/frames, but there we are.

Comment 7

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #5)
> Baseline try revision: bde8be02e3e8
> try push with patch: remote:  
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=e7d5d0c098fb
> 
> https://treeherder.mozilla.org/perf.html#/
> compare?originalProject=try&originalRevision=bde8be02e3e8&newProject=try&newR
> evision=e7d5d0c098fb
> 
> we'll need to wait for builds to finish and will then need to do retriggers,
> but then the above should be helpful in seeing perf impact.
> 
> FWIW, the bug that introduced the code here was bug 912351... while that was
> a visual fix, I'm not sure what the perf implications are for keeping all
> the nodes around...

Yeah, so that clearly doesn't work from a perf perspective...

https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=bde8be02e3e8&newProject=try&newRevision=e7d5d0c098fb&originalSignature=8d47c06d6df551fe085883ad80cfe3fcb2d501f6&newSignature=8d47c06d6df551fe085883ad80cfe3fcb2d501f6

Comment 8

[:Gijs (he/him)]

Trying a different approach:

baseline: remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=e3e748153826
with patch: remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=85e0caafc9b3

perfherder:

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=e3e748153826&newProject=try&newRevision=85e0caafc9b3

Comment 9

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1214330 - show spacer when customization palette is hidden so the footer stays at the bottom, r?jaws

Bug 1214330 - show spacer when customization palette is hidden so the footer stays at the bottom, r?jaws

Comment 10

[arni2033]

[just thinking aloud]
Are you sure that in your patch you want to (first) hide the palette and (then) show the spacer?
Technically, footer will still twitch, but of course it won't be visible to the human eye.
Why not just keep that spacer always visible?

Comment 11

[:Gijs (he/him)]

(In reply to arni2033 from comment #10)
> [just thinking aloud]
> Are you sure that in your patch you want to (first) hide the palette and
> (then) show the spacer?
> Technically, footer will still twitch, but of course it won't be visible to
> the human eye.

No, you're assuming that changes immediately trigger a re-layout. That is not the case.

> Why not just keep that spacer always visible?

Because it will change the size of the visible palette (ie the box of items), which will be visible if you have enough items (or your window is small enough) to cause a scrollbar.

Comment 12

[arni2033]

1) Ok, nevermind
2) Nope, spacers have height==0px; is such cases. That additional indent is caused by 
   #customization-palette { margin-bottom:25px; }. I removed [hidden] attribute on this screenshot:
>  http://ssmaker.ru/99240d36.png

Comment 13

[:Gijs (he/him)]

(In reply to arni2033 from comment #12)
> 1) Ok, nevermind
> 2) Nope, spacers have height==0px; is such cases.

Hmm, I neglected to check this. The general statement you're making is not correct (spacers behave differently depending on CSS and flex attributes), but you're right in this instance. Which is interesting, because in a normal-sized window it takes up space, and I assumed it would continue to do so, which turns out not to be the case.

In fact, the spacer *used to* have a flex attribute, which would have had the behaviour I mentioned. It still *does* have flex, but now through new-style flexbox from:

https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.css#1213

which might behave differently now (in that it can/does collapse to 0) from how it did when we implemented that code (the flex spec changed a while back, as did our implementation).

The change from the flex attribute was from https://hg.mozilla.org/mozilla-central/rev/80b59bea12f8 which was bug 987586.

I'll need to do another trypush to figure out if keeping the spacer always visible is better or worse - I expect better, but I don't know for sure. :-\

Comment 14

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

Comment on attachment 8678280 [details]
MozReview Request: Bug 1214330 - show spacer when customization palette is hidden so the footer stays at the bottom, r?jaws

https://reviewboard.mozilla.org/r/23161/#review20657

This looks fine to me as long as your talos numbers back it up. Thanks for looking in to this.

Comment 15

[:Gijs (he/him)]

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=bcb3e3699005


https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=e3e748153826&newProject=try&newRevision=bcb3e3699005

Comment 16

[Pulsebot]

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

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/40ef1c5ebb78

Comment 18

[Khalid Syfullah Zaman [:khalid32]]

I have reproduced this bug on Nightly 44.0a1 (2015-10-13) on ubuntu 14.04 LTS, 32 bit!

The bug's fix is now verified on Latest Nightly 45.0a1!

Build ID: 20151202030210
User Agent: Mozilla/5.0 (X11; Linux i686; rv:45.0) Gecko/20100101 Firefox/45.0

[bugday-20151202]