963999 - Difference between customize-entering and customize-entered states is too drastic

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

Currently, when the customize mode enter transition finishes, we switch from the customize-entering state to customize-entered.

Manually toggling those two states locally, the difference between the two states is too drastic.

Not only is the grid not drawn (which, at least when we were animating the padding, was desired for perf reasons), but things seem to change dimensions slightly when switching from one state to the other.

These things include (at least on OS X):

1) The titlebar buttons moving (bug 933933)
2) A border being applied to the customization-container only on customization-entered.
3) Transparent border-left and border-right being applied to the TabsToolbar
4) Padding left and right is applied to every .customization-target

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Another obvious difference is that the background colour of the tab-view-deck is very different during customize-entering than in the customize-entered state. We could definitely make this less drastic by making the customize-entering colour the same.

Comment 2

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Attachment: Visual summary of animation glitches (entering customization mode)

Here's a visual map of the things that are happening while entering customization mode. I included bug numbers where bugs are on file.
I think the issues that don't have their own bugs yet are well inside the scope of this bug.

Comment 3

[Blake Winton (:bwinton) (:☕️)]

Attachment: WIP patch that fixes a few of the easier/more obvious things.

So, here's some of the things fixed.

Philipp: Give it a try, and let me know what you think!
Mike: Could you let me know if anything in the CSS seems strange?

Thanks!

Comment 4

[Blake Winton (:bwinton) (:☕️)]

Just to write down the things Philipp mentioned:
1) Wrong background colour.
2) Text in location bar and search field should be hidden at the start of the animation. (bug 964232)
3) Blueprint pattern should show up immediately at the beginning of the transition.
4) Exit button should be styled blue from the very beginning.
5) Gradient should be the same as in normal window mode.  (Mac only, bug 883145)
6) Maybe it's better to move the + symbol to the right at the beginning of the transition.
7) Items below multi-line labels jump at the end of the transition. (bug 964244)
8) Icons should fade in instead of just appearing. (bug 964262)
9) Buttons should have the same state and styling throughout the transition.

And the things I fixed are 1, 2, 4, and 9.  #3 is not being done on purpose for better perf.

Comment 5

[Blake Winton (:bwinton) (:☕️)]

(And actually, that patch works fairly well, so if the two of you wanted to r and ui-r instead of f-ing it, feel free…  ;)

Comment 6

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Comment on attachment 8368073 [details] [diff] [review]
WIP patch that fixes a few of the easier/more obvious things.

Excellent work!
With this patch, I consider the issues 1, 4 and 9 fixed.
#2 looks better now, but the »about:customizing« string still shows up briefly in the beginning of the animation.

But even in its current state, this is a big improvement.
Ship it! Unless you know how to get rid of the string in the location bar all together. In that case: fix it then ship it ;)

Comment 7

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

There seems to be some conflict with the patch for bug 962677.
When I have both patches applied, the window background doesn't change like intended.
The other stuff works.

Comment 8

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Comment on attachment 8368073 [details] [diff] [review]
WIP patch that fixes a few of the easier/more obvious things.

The interference with Mikes patch evidently is no more (or it had to do with my local environment).
So as far as the UI side is concerned, that patch works great! Thus ui-r+

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Attachment: WIP patch that fixes a few of the easier/more obvious things. (ui-r+'d and feedback+'d from phlsa)

De-bitrotting.

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Attachment: WIP patch that fixes a few of the easier/more obvious things. (ui-r+'d and feedback+'d from phlsa)

More de-bitrotting.

Comment 12

[Blake Winton (:bwinton) (:☕️)]

Mike: are you fine with landing this, then?

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8370919 [details] [diff] [review]
WIP patch that fixes a few of the easier/more obvious things. (ui-r+'d and feedback+'d from phlsa)

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

Yeah, I think I'm OK with this - but we should keep this bug open, as I imagine this might be a multi-patch effort.

::: browser/base/content/browser.css
@@ +398,5 @@
>  #identity-box.verifiedIdentity > #identity-icon-labels > #identity-icon-label {
>    -moz-margin-end: 0.25em !important;
>  }
>  
> +#main-window[customizing] toolbaritem > hbox > textbox > hbox > hbox > html|*.textbox-input,

Is this targeting the URL bar? If so, I think we should probably target it directly instead of every toolbaritem.

Comment 14

[Dão Gottwald [:dao]]

Comment on attachment 8370919 [details] [diff] [review]
WIP patch that fixes a few of the easier/more obvious things. (ui-r+'d and feedback+'d from phlsa)

(In reply to Mike Conley (:mconley) from comment #13)
> > +#main-window[customizing] toolbaritem > hbox > textbox > hbox > hbox > html|*.textbox-input,
> 
> Is this targeting the URL bar? If so, I think we should probably target it
> directly instead of every toolbaritem.

Absolutely. Also, the selector for the search bar contains redundant cruft. Please get a patch up for review that fixes that before landing this.

Comment 15

[Blake Winton (:bwinton) (:☕️)]

Attachment: The first final version of the patch.

Okay, here's an updated patch, which cleans up the existing search selector, and does something slightly different with the urlbar.  (Since we're already making it grey, I'm just doing that earlier, rather than hiding the text.)

Comment 16

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8374289 [details] [diff] [review]
The first final version of the patch.

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

feedback+, but I also think there are some adjustments we should make - see below. Thanks Blake!

::: browser/base/content/browser.css
@@ +400,5 @@
>  #identity-box.verifiedIdentity > #identity-icon-labels > #identity-icon-label {
>    -moz-margin-end: 0.25em !important;
>  }
>  
> +#main-window[customizing] #searchbar html|*.textbox-input {

What happened to hiding the text in the URL bar?

::: browser/themes/linux/browser.css
@@ +1985,5 @@
>  /* Customization mode */
>  
>  %include ../shared/customizableui/customizeMode.inc.css
>  
> +#main-window[customizing] #tab-view-deck {

Same as OS X - let's use child selectors.

::: browser/themes/osx/browser.css
@@ +4137,5 @@
>    margin-top: 11px;
>    margin-bottom: 11px;
>  }
>  
> +#main-window[customizing] #tab-view-deck {

Let's use a set of child selectors here, since the depth isn't too deep on these two elements.

Comment 17

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Looks good!
Two things:
- Please re-include the code that removes the text in the text in the URL bar. It still adds visual noise.
- During the transition, there is weird thing happening where the title bar shows two gradients: http://cl.ly/image/2r3s0J192R3V

But overall, I think this is a huge improvement!

Comment 18

[Blake Winton (:bwinton) (:☕️)]

Attachment: The second final version of the patch.

(In reply to Mike Conley (:mconley) from comment #16)
> >  #identity-box.verifiedIdentity > #identity-icon-labels > #identity-icon-label {
> >    -moz-margin-end: 0.25em !important;
> >  }
> > +#main-window[customizing] #searchbar html|*.textbox-input {
> What happened to hiding the text in the URL bar?

I figured since it was being greyed out, perhaps we wanted to leave it there, but then Philipp said:
> - Please re-include the code that removes the text in the text in the URL
> bar. It still adds visual noise.

So now I'm hiding it again.

> ::: browser/themes/osx/browser.css
> @@ +4137,5 @@
> > +#main-window[customizing] #tab-view-deck {
> Let's use a set of child selectors here, since the depth isn't too deep on
> these two elements.

So, it turns out that Philip's other comment:
> - During the transition, there is weird thing happening where the title bar
> shows two gradients: http://cl.ly/image/2r3s0J192R3V

happened because the tab-view-deck doesn't reach the top of the screen, so I changed this selector to target #main-window directly, which fixes it, and removes the child/descendent selector.  :)

> ::: browser/themes/linux/browser.css
> @@ +1985,5 @@
> > +#main-window[customizing] #tab-view-deck {
> Same as OS X - let's use child selectors.

But this one I changed, because of the titlebar.  I also changed the selector below that was nigh-identical to this one.

Thanks for the comments!

Comment 19

[Dão Gottwald [:dao]]

Comment on attachment 8375073 [details] [diff] [review]
The second final version of the patch.

>+#main-window[customizing] #urlbar,
> toolbarpaletteitem[removable="false"] {
>   opacity: 0.5;
>   cursor: default;
> }

Seems like this will make the url bar get an opacity of 25% after the transition.
We shouldn't special-case the url bar here in the first place, but rather fix the existing code to fade out those unremovable nodes earlier.

Can you remove this rule while you're at it? http://hg.mozilla.org/mozilla-central/annotate/a62bde1d6efe/browser/base/content/browser.css#l363

>-#wrapper-search-container > #search-container > #searchbar > .searchbar-textbox > .autocomplete-textbox-container > .textbox-input-box > html|*.textbox-input {
>+#main-window[customizing] #urlbar html|*.textbox-input,

Please use the child selector between #urlbar and html|*.textbox-input. That said, is there any reason why we should go all the way to html|*.textbox-input rather than .textbox-input-box or .autocomplete-textbox-container?

>+#main-window[customizing] #searchbar html|*.textbox-input {

ditto

>-#edit-controls@inAnyPanel@ > toolbarbutton[disabled] > .toolbarbutton-icon,
>-#zoom-controls@inAnyPanel@ > toolbarbutton[disabled] > .toolbarbutton-icon {
>+#main-window:not([customizing]) #edit-controls@inAnyPanel@ > toolbarbutton[disabled] > .toolbarbutton-icon,
>+#main-window:not([customizing]) #zoom-controls@inAnyPanel@ > toolbarbutton[disabled] > .toolbarbutton-icon {
>   opacity: .25;
> }

please replace '#main-window:not([customizing])' with ':not(toolbarpaletteitem) >'

Comment 20

[Blake Winton (:bwinton) (:☕️)]

Attachment: The third final version of the patch.

(In reply to Dão Gottwald [:dao] from comment #19)
> >+#main-window[customizing] #urlbar,
> > toolbarpaletteitem[removable="false"] {
> >   opacity: 0.5;
> >   cursor: default;
> > }
> 
> Seems like this will make the url bar get an opacity of 25% after the
> transition.
> We shouldn't special-case the url bar here in the first place, but rather
> fix the existing code to fade out those unremovable nodes earlier.

Fixed.

> Can you remove this rule while you're at it?
> http://hg.mozilla.org/mozilla-central/annotate/a62bde1d6efe/browser/base/
> content/browser.css#l363

Removed.

> >-#wrapper-search-container > #search-container > #searchbar > .searchbar-textbox > .autocomplete-textbox-container > .textbox-input-box > html|*.textbox-input {
> >+#main-window[customizing] #urlbar html|*.textbox-input,
> Please use the child selector between #urlbar and html|*.textbox-input. That
> said, is there any reason why we should go all the way to
> html|*.textbox-input rather than .textbox-input-box or
> .autocomplete-textbox-container?
> >+#main-window[customizing] #searchbar html|*.textbox-input {
> ditto

Changed to "#main-window[customizing] toolbaritem .autocomplete-textbox-container > .textbox-input-box".  I don't know if we should have the "toolbaritem" in there or not.  Thoughts?

> >-#edit-controls@inAnyPanel@ > toolbarbutton[disabled] > .toolbarbutton-icon,
> >-#zoom-controls@inAnyPanel@ > toolbarbutton[disabled] > .toolbarbutton-icon {
> >+#main-window:not([customizing]) #edit-controls@inAnyPanel@ > toolbarbutton[disabled] > .toolbarbutton-icon,
> >+#main-window:not([customizing]) #zoom-controls@inAnyPanel@ > toolbarbutton[disabled] > .toolbarbutton-icon {
> >   opacity: .25;
> > }
> 
> please replace '#main-window:not([customizing])' with
> ':not(toolbarpaletteitem) >'

The problem with that change is that they don't get wrapped in a toolbarpaletteitem until after the customization animation ends, and we want them to not have the opacity at the start of the customization.

Comment 21

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8375552 [details] [diff] [review]
The third final version of the patch.

I'll take this one.

Comment 22

[Mike Conley (:mconley) (:⚙️)]

Hm. With the attached patch, it feels like the transition itself gets jankier, while the "finish" is smoother.

Going to the judges with this one:

Baseline (all but Windows): https://tbpl.mozilla.org/?tree=Try&rev=71efe1f6d4a7
Baseline (Windows):  https://tbpl.mozilla.org/?tree=Try&rev=21e77b801df3
Patch: https://tbpl.mozilla.org/?tree=Try&rev=af8b84426de9
Compare talos (all but Windows): http://compare-talos.mattn.ca/?oldRevs=71efe1f6d4a7&newRev=af8b84426de9&server=graphs.mozilla.org&submit=true
Compare talos (Windows): http://compare-talos.mattn.ca/?oldRevs=21e77b801df3&newRev=af8b84426de9&server=graphs.mozilla.org&submit=true

Comment 23

[Mike Conley (:mconley) (:⚙️)]

So this patch regresses us on CART, so I wasn't just imagining it.

I've narrowed it down to the linear-gradient being applied to the main-window. If I make that a solid colour, the jank appears to go away.

Local CART testing suggests that the resulting patch doesn't add any overhead to our CART results for entering, but *might* add a tiny bit of time to our exit animation. I'll push to try just to see. Even if it does, we might want to eat that cost for the visual smoothness it gives us.

Comment 24

[Mike Conley (:mconley) (:⚙️)]

Baseline (all but Windows): https://tbpl.mozilla.org/?tree=Try&rev=71efe1f6d4a7
Baseline (Windows):  https://tbpl.mozilla.org/?tree=Try&rev=21e77b801df3
Patch: https://tbpl.mozilla.org/?tree=Try&rev=939e8ac484a1
Compare talos (all but Windows): http://compare-talos.mattn.ca/?oldRevs=71efe1f6d4a7&newRev=939e8ac484a1&server=graphs.mozilla.org&submit=true
Compare talos (Windows): http://compare-talos.mattn.ca/?oldRevs=21e77b801df3&newRev=939e8ac484a1&server=graphs.mozilla.org&submit=true

Comment 25

[Mike Conley (:mconley) (:⚙️)]

Gah - that patch push fires off a bunch of tests and is also not running talos. Classic fat-finger.

Re-pushed, this time only doing talos svgr. Sorry for the cost, releng. :(

Baseline (all but Windows): https://tbpl.mozilla.org/?tree=Try&rev=71efe1f6d4a7
Baseline (Windows):  https://tbpl.mozilla.org/?tree=Try&rev=21e77b801df3
Patch: https://tbpl.mozilla.org/?tree=Try&rev=c1b9b5ed979b
Compare talos (all but Windows): http://compare-talos.mattn.ca/?oldRevs=71efe1f6d4a7&newRev=c1b9b5ed979b&server=graphs.mozilla.org&submit=true
Compare talos (Windows): http://compare-talos.mattn.ca/?oldRevs=21e77b801df3&newRev=c1b9b5ed979b&server=graphs.mozilla.org&submit=true

Comment 26

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v4

De-bitrotted.

Comment 27

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v4.1

Ok, I _think_ this is the one. Going to push to try to make sure I didn't regress CART here.

Comment 28

[Mike Conley (:mconley) (:⚙️)]

Baseline: https://tbpl.mozilla.org/?tree=Try&rev=77f48900ece6
Patch: https://tbpl.mozilla.org/?tree=Try&rev=a7ce46ef9f1f
Compare talos: http://compare-talos.mattn.ca/?oldRevs=77f48900ece6&newRev=a7ce46ef9f1f&server=graphs.mozilla.org&submit=true

Comment 29

[Mike Conley (:mconley) (:⚙️)]

Ugh. Even getting rid of the linear gradient didn't improve things for Linux. Let me see if I can figure out what's going wrong there... *sigh*

Comment 30

[Mike Conley (:mconley) (:⚙️)]

So I'm getting (surprise!) wildly varying results when running CART locally.

I'm going to trigger 7 more runs each for the builds I linked to in comment 28. Maybe we'll converge on some kind of agreement there.

Comment 31

[Mike Conley (:mconley) (:⚙️)]

*sigh*, so the regression appears to be legit. Now comes the part where I try to determine which part is regressing Linux so I can extract it, or somehow turn it off just for Linux.

Comment 32

[Mike Conley (:mconley) (:⚙️)]

Bumping to P2, as this is a really-want-to-have.

Comment 33

[Mike Conley (:mconley) (:⚙️)]

Attempting to determine which section of this patch is causing Ubuntu's CART regression hasn't been too successful - the numbers move around pretty wildly. :(

Taking it to the next level - I'm going to carpet-bomb try with certain sections of the patch reversed to see if I can narrow it down.

Comment 34

[Mike Conley (:mconley) (:⚙️)]

Carpet bomb is paying off - the problem seems to be isolated to the changes under browser/base/content/browser.css.

So that's the good news; we have a better idea of where the problem is.

The bad news is that the stuff under browser/base/content/browser.css are changes that alter all OS's, so as it currently stands, it's not in a good position to abstract the stuff out just from Ubuntu so we can land right away.

So we're not done yet - next, I have to determine _what_ it is under browser/base/content/browser.css is causing the slowdown, and then see if there's anything we can do about it.

Comment 35

[Mike Conley (:mconley) (:⚙️)]

Ah, proof of my claim in the last comment:

Baseline: https://tbpl.mozilla.org/?tree=Try&rev=e0f897540b28
Patch without changes to browser/base/content/browser.css: https://tbpl.mozilla.org/?tree=Try&rev=d032452e5658
Compare talos: http://compare-talos.mattn.ca/?oldRevs=e0f897540b28&newRev=d032452e5658&server=graphs.mozilla.org&submit=true

Comment 36

[Mike Conley (:mconley) (:⚙️)]

So this is pretty freaky, but it looks like there are two parts of the browser/base/content/browser.css changes that are causing this regression.

I broke the change to browser.css into 3 chunks (A, B and C), with one rule change per chunk.  I took the original patch, reverted all of the browser/base/content/browser.css changes, and then applied each of the individual chunks on top to see if any of them individually were causing the regression.

Baseline: https://tbpl.mozilla.org/?tree=Try&rev=e0f897540b28
Patch + Chunk A: https://tbpl.mozilla.org/?tree=Try&rev=3fd1ca5a9359
Patch + Chunk B: https://tbpl.mozilla.org/?tree=Try&rev=776a455b6c4c
Patch + Chunk C: https://tbpl.mozilla.org/?tree=Try&rev=32701284e0df

Here are the compare-talos's for Baseline vs each chunk:

Baseline vs Chunk A: http://compare-talos.mattn.ca/?oldRevs=e0f897540b28&newRev=3fd1ca5a9359&server=graphs.mozilla.org&submit=true
Baseline vs Chunk B: http://compare-talos.mattn.ca/?oldRevs=e0f897540b28&newRev=776a455b6c4c&server=graphs.mozilla.org&submit=true
Baseline vs Chunk C: http://compare-talos.mattn.ca/?oldRevs=e0f897540b28&newRev=32701284e0df&server=graphs.mozilla.org&submit=true

TL;DR - no individual chunk seems to be causing the regression.

Imagine my frustration.

So I started doing chunk combinations - A+B, B+C, A+C:

A+B: https://tbpl.mozilla.org/?tree=Try&rev=712a88ae6825
B+C: https://tbpl.mozilla.org/?tree=Try&rev=3491c595ccda
A+C: https://tbpl.mozilla.org/?tree=Try&rev=cc5c52f5deeb

Baseline vs. A+B: http://compare-talos.mattn.ca/?oldRevs=e0f897540b28&newRev=712a88ae6825&server=graphs.mozilla.org&submit=true
Baseline vs. B+C: http://compare-talos.mattn.ca/?oldRevs=e0f897540b28&newRev=3491c595ccda&server=graphs.mozilla.org&submit=true
Baseline vs. A+C: http://compare-talos.mattn.ca/?oldRevs=e0f897540b28&newRev=cc5c52f5deeb&server=graphs.mozilla.org&submit=true

AH HAH. Baseline vs. A+C shows the regression.

These are the A and C changes:

A:

-#wrapper-search-container > #search-container > #searchbar > .searchbar-textbox > .autocomplete-textbox-container > .textbox-input-box > html|*.textbox-input {
+#main-window[customizing] toolbaritem .autocomplete-textbox-container > .textbox-input-box {
   visibility: hidden;
 }

C:

-toolbarpaletteitem[removable="false"] {
+#main-window[customizing] toolbaritem[removable="false"] {
   opacity: 0.5;
   cursor: default;
 }

My recommendation is to land everything except these two changes, and then file follow-ups to see if we can solve these for the other platforms without regressing anybody.

Comment 37

[Mike Conley (:mconley) (:⚙️)]

So I just talked to phlsa, and here's what we're going to try:

We're going to land all of this patch _except_ for the part that changes the opacity of the non-removable items.

Then, as a follow-up, we're going to see about animating the opacity on the non-removable items _after_ entering customize mode, much like we're now fading in the palette items.

Comment 38

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v4.2

Alright, this reverts the change that makes the removable items half transparent at the start of the customization transition.

Comment 39

[Dão Gottwald [:dao]]

Comment on attachment 8386151 [details] [diff] [review]
Patch v4.2

>-#wrapper-search-container > #search-container > #searchbar > .searchbar-textbox > .autocomplete-textbox-container > .textbox-input-box > html|*.textbox-input {
>+#main-window[customizing] toolbaritem .autocomplete-textbox-container > .textbox-input-box {
>   visibility: hidden;
> }

Target the search bar and the url bar here specifically. This should work (untested):

+#main-window[customizing] :-moz-any(#urlbar, .searchbar-textbox) > .autocomplete-textbox-container > .textbox-input-box

Comment 40

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v4.3

Thanks Dao - I tested your approach, and it works.

Comment 41

[Mike Conley (:mconley) (:⚙️)]

Surprise, tree is closed.

I'll go ahead and file that bug about making the opacity transition for non-removable items.

Comment 42

[Mike Conley (:mconley) (:⚙️)]

I've filed bug 980007 for the opacity transition work.

Comment 43

[Mike Conley (:mconley) (:⚙️)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/b5f281d7610c

Comment 44

[Carsten Book [:Tomcat]]

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

Comment 45

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8386163 [details] [diff] [review]
Patch v4.3

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 

You guessed it - Australis!


User impact if declined: 

A more abrupt, jarring customization transition - especially for users on OS X.


Testing completed (on m-c, etc.): 

Plenty of local testing, and time to bake on m-c.


Risk to taking this patch (and alternatives if risky): 

Very low.


String or IDL/UUID changes made by this patch:

None.

Comment 46

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8386163 [details] [diff] [review]
Patch v4.3

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 

You guessed it - Australis!


User impact if declined: 

A more abrupt, jarring customization transition - especially for users on OS X.


Testing completed (on m-c, etc.): 

Plenty of local testing, and time to bake on m-c.


Risk to taking this patch (and alternatives if risky): 

Very low. This will need to be landed in tandem with bug 981062 to avoid a CART regression on Windows 8.


String or IDL/UUID changes made by this patch:

None.

Comment 47

[Mike Conley (:mconley) (:⚙️)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/362973711ccf