1196313 - Add a transition for the background color of the identity box on hover

Status: VERIFIED FIXED

(Firefox :: Theme, defect, P1)

Description

[:Paolo Amadini]

We can add a transition for the background color of the identity box on hover, similar to the one used for toolbar buttons.

Comment 1

[:Paolo Amadini]

Attachment: MozReview Request: Bug 1196313 - Add a transition for the background color of the identity box on hover. r=MattN

Bug 1196313 - Add a transition for the background color of the identity box on hover. r=bgrins

Comment 2

[:Paolo Amadini]

[Tracking Requested - why for this release]:

Small incremental improvement to the site identity block we redesigned for Firefox 42, that makes it behave more similarly to the toolbar buttons on hover.

Comment 3

[Liz Henry (:lizzard) (relman/hg->git project)]

I'm not sure that release management needs to track this. 

But once it lands, please nominate it for uplift to 42. Thanks!

Comment 4

[Brian Grinstead [:bgrins]]

Comment on attachment 8649953 [details]
MozReview Request: Bug 1196313 - Add a transition for the background color of the identity box on hover. r=MattN

https://reviewboard.mozilla.org/r/16519/#review14957

::: browser/themes/shared/identity-block/identity-block.inc.css:31
(Diff revision 1)
> +  transition: background-color 0.15s ease;

Why 'ease'?  It seems like the toolbarbuttons don't use that.

::: browser/themes/shared/identity-block/identity-block.inc.css:71
(Diff revision 1)
> -  transition-delay: 100s;
> +  transition-delay: 0.15s, 100s;

This won't do what you want.  See http://jsbin.com/xerogopule/edit?html,css,output and notice that the font-size changes in 1s, not 2s.  You'll need a second 100s after the first to get both paddings to have the same delay.

Comment 5

[:Paolo Amadini]

(In reply to Brian Grinstead [:bgrins] from comment #4)
> Why 'ease'?  It seems like the toolbarbuttons don't use that.

Hm, this was the original transition from Philipp but yu're right, we could use the same timing function as toolbar buttons.

The transition time is also 150ms here versus 250ms for buttons, though this might be intended since the change in color is more pronounced. Philipp, is this correct?

Comment 6

[:Paolo Amadini]

Comment on attachment 8649953 [details]
MozReview Request: Bug 1196313 - Add a transition for the background color of the identity box on hover. r=MattN

Bug 1196313 - Add a transition for the background color of the identity box on hover. r=bgrins

Comment 7

[Brian Grinstead [:bgrins]]

Comment on attachment 8649953 [details]
MozReview Request: Bug 1196313 - Add a transition for the background color of the identity box on hover. r=MattN

https://reviewboard.mozilla.org/r/16519/#review15123

Code changes look fine

Comment 8

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

If we switch to a linear easing function, we should shorten the transition time even further (to 100ms). 150ms looks good with »ease«.

There's also a 150ms transition delay in there. I think we should remove that so that the transition doesn't look yanky.

Comment 9

[:Paolo Amadini]

https://reviewboard.mozilla.org/r/16519/#review15491

In the previous version of the patch, I simply extended this rule with a new background color transition:

@conditionalForwardWithUrlbar@:not([switchingtabs]) > #urlbar > #identity-box {
  transition: background-color 150ms, padding-left, padding-right;
}

While the "transition: background-color 150ms" was always present for #identity-box.

However, for some reason I don't understand this regressed the scenario where you switch tabs using the CTRL+TAB key combination while hovering with the mouse over a disabled forward button. The difference seems to be that previously there was no transition at all in the base rule, while now we were adding more transition properties in addition to the existing one.

I suspect this can be either a bug in our platform code for CSS or a "feature" of the CSS specification. The entire technique used here of delaying a transition, re-triggering a non-delayed transition by using a .01px difference but only in some circumstances looks error-prone and dependent on edge cases...

This new version of the patch seems to handle that correctly and remove some duplication.

I talked briefly with Brian and we'd like another pair of eyes on this. Matt, Gijs?

Comment 10

[:Paolo Amadini]

Comment on attachment 8649953 [details]
MozReview Request: Bug 1196313 - Add a transition for the background color of the identity box on hover. r=MattN

Bug 1196313 - Add a transition for the background color of the identity box on hover. r=MattN

Comment 11

[:Paolo Amadini]

Matt, Gijs, details for the review request are in comment 9, I still haven't figured out exactly how to publish a comment together with the review request in MozReview.

Comment 12

[:Paolo Amadini]

https://reviewboard.mozilla.org/r/16519/#review14957

> Why 'ease'?  It seems like the toolbarbuttons don't use that.

As Philipp pointed out the choice was between 100ms linear and 150ms ease, I think the former was barely noticeable so I went with Philipp's original ease timing function that in practice looks more similar to toolbar buttons despite timings are different. Oddities of perception.

Comment 13

[:Gijs (he/him)]

Comment on attachment 8649953 [details]
MozReview Request: Bug 1196313 - Add a transition for the background color of the identity box on hover. r=MattN

https://reviewboard.mozilla.org/r/16519/#review15501

WFM, but I'd prefer Dão to look at this too.

Comment 14

[Dão Gottwald [:dao]]

Comment on attachment 8649953 [details]
MozReview Request: Bug 1196313 - Add a transition for the background color of the identity box on hover. r=MattN

It's unfortunate that the transition and transition-delay properties are further apart now while needing to be kept in sync. Can you at least extend the "forward button hiding is delayed when hovered" comment to say that the latter two delays are for padding-left/right?

Comment 15

[:Paolo Amadini]

(In reply to Dão Gottwald [:dao] from comment #14)
> It's unfortunate that the transition and transition-delay properties are
> further apart now while needing to be kept in sync. Can you at least extend
> the "forward button hiding is delayed when hovered" comment to say that the
> latter two delays are for padding-left/right?

Will do, thanks for the quick review.

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/0077cc462038

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0077cc462038

Comment 18

[:Paolo Amadini]

Comment on attachment 8649953 [details]
MozReview Request: Bug 1196313 - Add a transition for the background color of the identity box on hover. r=MattN

Approval Request Comment
[Feature/regressing bug #]: Identity Block redesign
[User impact if declined]: Behavior on hover not matching exact visual design
[Describe test coverage new/current, TreeHerder]: Landed on mozilla-central
[Risks and why]: We had to handle some edge cases with tab switching while hovering. Any issue we didn't catch already would only affect the exact distance between the back button and the site icon.
[String/UUID change made/needed]: None

Comment 19

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8649953 [details]
MozReview Request: Bug 1196313 - Add a transition for the background color of the identity box on hover. r=MattN

Let's take it in 42.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c92238c77438

Comment 21

[Paul Silaghi, QA [:pauly]]

Marking [qe-] since the transition is barely visible to naked eyes.

Comment 22

[Khalid Syfullah Zaman [:khalid32]]

I have reproduced this bug on Nightly 43.0a1 (2015-08-19) on ubuntu 14.04 LTS, 32 bit!

The bug's fix is now verified on Latest Beta 42.0b3!

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

[testday-20151002]