Closed Bug 1214330 Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox :: Toolbars and Customization, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 44
Tracking Status
firefox44 --- verified

People

(Reporter: arni2033, Assigned: Gijs)

References

()

Details

Attachments

(2 files)

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
Are you able to write a patch for this?
Flags: needinfo?(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
Flags: needinfo?(arni2033)
(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.
Flags: needinfo?(gijskruitbosch+bugs)
(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.)
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...
Assignee: nobody → gijskruitbosch+bugs
Blocks: 912351
Status: NEW → ASSIGNED
Flags: needinfo?(gijskruitbosch+bugs)
s/nodes/frames, but there we are.
(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
Bug 1214330 - show spacer when customization palette is hidden so the footer stays at the bottom, r?jaws
Attachment #8678280 - Flags: review?(jaws)
[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?
(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.
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
(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 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.
Attachment #8678280 - Flags: review?(jaws) → review+
Flags: needinfo?(gijskruitbosch+bugs)
https://hg.mozilla.org/mozilla-central/rev/40ef1c5ebb78
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
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]
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.