1183203 - Identity block hover background color is jarring on the dark Dev Edition theme

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect, P1)

Description

[Brian Grinstead [:bgrins]]

Attachment: dark-deved-identity-open.png

The grey background for hover/active state on the identity block looks out of place in the dark Dev Edition theme (see screenshot)

Comment 1

[Brian Grinstead [:bgrins]]

It's too white, it should blend it better with the UI in the dark theme

Comment 2

[Brian Grinstead [:bgrins]]

Also, it bleeds into the side of the shield icon to the left

Comment 3

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #2)
> Also, it bleeds into the side of the shield icon to the left

Which is a problem in both the light and dark Dev Edition themes.

Comment 4

[:Paolo Amadini]

Thanks for finding this out. This type of issues is also why we've built some contingency in bug 1180213, there's no way we could test all the combinations in a reasonable amount of time. Look at that test matrix, it's insane! :-)

I'm not sure how to test the Developer Edition theme in Nightly either, the instructions I found seem outdated.

Comment 5

[Brian Grinstead [:bgrins]]

No problem, this sort of thing happens in Dev Edition sometimes - and I'm hoping it becomes less common over time as we introduce more CSS variables into our code base.  To test out the theme on nightly, just go to about:addons -> Appearance and then apply the Developer Edition theme.  Then open the devtools toolbox and switch the theme to Dark in the options panel (or just set devtools.theme to 'dark').

I will upload a quick patch showing a possible approach for fixing this

Comment 6

[:Paolo Amadini]

(In reply to Brian Grinstead [:bgrins] from comment #5)
> To test out the theme on nightly, just go to
> about:addons -> Appearance and then apply the Developer Edition theme.  Then
> open the devtools toolbox and switch the theme to Dark in the options panel
> (or just set devtools.theme to 'dark').
> 
> I will upload a quick patch showing a possible approach for fixing this

Great, thanks!

Comment 7

[Brian Grinstead [:bgrins]]

Attachment: identity-devedition-pt1.patch

This is the easiest way to handle changing colors in the Dev Edition theme (by using a CSS variable).  There are still two more problems though -

1) When hovering (at least on osx yosemite) the top/bottom border of the nav bar is covered up by the background.  Seems to be due to a -2px top/bottom margin on the #identity-box
2) The background is still bleeding onto the shield.  This is because the border-image on the popup box is hidden - https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/devedition.inc.css#243.  That's because the border image (chrome://browser/skin/[e-mail]) has a white color hardcoded into it that makes it stand out in the dark DE theme.  I wonder if the image actually needs that color in there or if it could just be the rightward facing arrow with a transparent background.

Anyway, this fixes the first part of the issues by overriding the color with a variable.  What do you think?

Comment 8

[:Paolo Amadini]

(In reply to Brian Grinstead [:bgrins] from comment #7)
> Anyway, this fixes the first part of the issues by overriding the color with
> a variable.  What do you think?

Looks good! Seems like the variable solves the specificity issue as well and is more elegant anyways.

> 1) When hovering (at least on osx yosemite) the top/bottom border of the nav
> bar is covered up by the background.  Seems to be due to a -2px top/bottom
> margin on the #identity-box

I guess this border exists only in the Developer Edition theme, while in the standard OS X 10.10 theme the navigation bar is flat. If this is the case, the Developer Edition should probably override the rule that sets the margin:

#urlbar:not([focused="true"]) > #identity-box {

I wonder if we can do this using variables as well...

> I wonder if the image
> actually needs that color in there or if it could just be the rightward
> facing arrow with a transparent background.

I guess one of the sides of the arrow would need the color in it because the notification icons may have a different (or transparent) background color with some themes. Actually, we may want to rethink and simplify that...

I'll test this in more detail tomorrow, hope this helps in the meantime!

Comment 9

[Brian Grinstead [:bgrins]]

(In reply to :Paolo Amadini from comment #8)
> > 1) When hovering (at least on osx yosemite) the top/bottom border of the nav
> > bar is covered up by the background.  Seems to be due to a -2px top/bottom
> > margin on the #identity-box
> 
> I guess this border exists only in the Developer Edition theme, while in the
> standard OS X 10.10 theme the navigation bar is flat. If this is the case,
> the Developer Edition should probably override the rule that sets the margin:
> 
> #urlbar:not([focused="true"]) > #identity-box {
> 

Yes, that is correct - the border only exists in the Dev Edition theme (it was originally added in Bug 1096413).

> I wonder if we can do this using variables as well...

I've generally tried to use variables in cases that can apply across all platforms and handled platform specific bug fixes by copying the selector into the corresponding devedition.css file.  We could prevent duplicating a selector in this case though by moving the fix into an OSX-specific variable, which seems kind of yucky but also would be less likely to accidentally break the DE theme in the future if one of those values changes.

Another simpler fix could be to set overflow: hidden on #urlbar.  Not sure if that'd cause any problems but we could then also simplify some of the #urlbar:not([focused="true"]) > #identity-box styles in browser.css since we don't have to worry about the box covering up the normal focus border.

Comment 10

[Brian Grinstead [:bgrins]]

Attachment: identity-devedition-pt2.patch

Attempts to fix the positioning issues

Comment 11

[:Paolo Amadini]

Comment on attachment 8632937 [details] [diff] [review]
identity-devedition-pt1.patch

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

::: browser/themes/shared/devedition.inc.css
@@ +68,5 @@
> +:root[devtoolstheme="dark"] #identity-box {
> +  --identity-box-chrome-color: #46afe3;
> +  --identity-box-chrome-background-image: linear-gradient(#5F6670 0, #5F6670 100%);
> +  --identity-box-verified-background-image: linear-gradient(#5F6670 0, #5F6670 100%);
> +  --verified-identity-box-backgroundcolor: transparent;

Any reason why this existing variable is not named "--identity-box-verified-background-color"? We might as well rename it while we're here.

@@ +69,5 @@
> +  --identity-box-chrome-color: #46afe3;
> +  --identity-box-chrome-background-image: linear-gradient(#5F6670 0, #5F6670 100%);
> +  --identity-box-verified-background-image: linear-gradient(#5F6670 0, #5F6670 100%);
> +  --verified-identity-box-backgroundcolor: transparent;
> +  --identity-box-selected-background-color: rgba(231,230,230,.2);

"-selected" sounds good, but wondering whether it should be "-active" like other variables? It applies to the "hover" state as well, though.

Comment 12

[:Paolo Amadini]

Comment on attachment 8633025 [details] [diff] [review]
identity-devedition-pt2.patch

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

::: browser/themes/shared/devedition.inc.css
@@ +232,5 @@
>  
>  #urlbar {
>    -moz-border-start: none !important;
>    opacity: 1 !important;
> +  overflow: hidden;

Does this have any visible effect on OS X 10.10 at present?

Comment 13

[Brian Grinstead [:bgrins]]

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

Comment 14

[Brian Grinstead [:bgrins]]

(In reply to :Paolo Amadini from comment #11)
> Comment on attachment 8632937 [details] [diff] [review]
> identity-devedition-pt1.patch
> 
> Review of attachment 8632937 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/themes/shared/devedition.inc.css
> @@ +68,5 @@
> > +:root[devtoolstheme="dark"] #identity-box {
> > +  --identity-box-chrome-color: #46afe3;
> > +  --identity-box-chrome-background-image: linear-gradient(#5F6670 0, #5F6670 100%);
> > +  --identity-box-verified-background-image: linear-gradient(#5F6670 0, #5F6670 100%);
> > +  --verified-identity-box-backgroundcolor: transparent;
> 
> Any reason why this existing variable is not named
> "--identity-box-verified-background-color"? We might as well rename it while
> we're here.
> 

Good catch, thanks

> @@ +69,5 @@
> > +  --identity-box-chrome-color: #46afe3;
> > +  --identity-box-chrome-background-image: linear-gradient(#5F6670 0, #5F6670 100%);
> > +  --identity-box-verified-background-image: linear-gradient(#5F6670 0, #5F6670 100%);
> > +  --verified-identity-box-backgroundcolor: transparent;
> > +  --identity-box-selected-background-color: rgba(231,230,230,.2);
> 
> "-selected" sounds good, but wondering whether it should be "-active" like
> other variables? It applies to the "hover" state as well, though.

Stuck with selected, but wouldn't be opposed to renaming it in the future

Comment 15

[Brian Grinstead [:bgrins]]

I think the easiest thing to do will be to get a a new version of http://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/urlbar-arrow.png and http://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/[e-mail].  At that point we could also move all of the urlbar-arrow icons into the shared/ themes folder, since they are the same across platforms.

The most likely option would be to replace the white fill on the left with #171B1F to match the Dev Edition URL bar.  In this case we can remove any custom positioning in the DE theme and just swap out the border image instead.  Aislinn, would you be able to help?  There weren't any mockups of this state for the original Dev Edition theme, but I don't see any reason why we wouldn't want that same arrow look that we have in Australis when the shield is next to a notification.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

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

Comment 17

[Ash Grigas]

Here is an updated color spec for the Dev edition. Try both options to see which has the best contrast across platforms.

https://www.dropbox.com/s/voxyn7q2brjb7ch/hover%20state.png?dl=0

Comment 18

[:Paolo Amadini]

Attachment: No notification area arrow in standard theme

On the other hand, now that we have the redesigned Control Center area, does it make sense to keep the arrow in the standard theme? It looks like a plain separator works just as well. There's also less of a distinction as we're pulling more icons (like logins and plugins for example) out of the notification area in the long run, in fact the Developer Edition already does that.

Note that this screenshot is a rough test, the icon on the left will be different and the separator on the right should look like the one on the left.

Comment 19

[Ash Grigas]

(In reply to :Paolo Amadini from comment #18)
> Created attachment 8634097 [details]
> No notification area arrow in standard theme
> 
> On the other hand, now that we have the redesigned Control Center area, does
> it make sense to keep the arrow in the standard theme? It looks like a plain
> separator works just as well. There's also less of a distinction as we're
> pulling more icons (like logins and plugins for example) out of the
> notification area in the long run, in fact the Developer Edition already
> does that.
> 
> Note that this screenshot is a rough test, the icon on the left will be
> different and the separator on the right should look like the one on the
> left.

Can you post a screenshot of how dev edition handles plug-ins in the url bar? I'm not familiar with it.

For reg. version, I think we should keep the arrow since we still want to keep some seperation from the non-control center items that are temporary (ie. plug-in notification) and the control center items (ie. lock and shield - most of time just lock). We also still need the arrow for other notifications like the key for passwords. Its better to have differentiation until we get to the updated control center icon behavior post 42.

Comment 20

[:Paolo Amadini]

Attachment: Notification area in Developer Edition theme

(In reply to agrigas from comment #19)
> Can you post a screenshot of how dev edition handles plug-ins in the url
> bar? I'm not familiar with it.

Currently, there is no arrow in Developer Edition but neither there is a separator.

> For reg. version, I think we should keep the arrow since we still want to
> keep some seperation from the non-control center items that are temporary
> (ie. plug-in notification) and the control center items (ie. lock and shield
> - most of time just lock). We also still need the arrow for other
> notifications like the key for passwords.

Wouldn't a regular vertical separator work as well for this purpose?

It seems in the end this worked well in Developer Edition even with no separation at all. I'm asking because a plain separator would solve some technical challenges and make the number of combinations we have to handle less insane by aligning our editions (look at the test matrix in bug 1180213 if you haven't already!).

> Its better to have differentiation
> until we get to the updated control center icon behavior post 42.

I'm not familiar with this, what would this new behavior be exactly?

Comment 21

[Ash Grigas]

(In reply to :Paolo Amadini from comment #20)
> Created attachment 8634125 [details]
> Notification area in Developer Edition theme
> 
> (In reply to agrigas from comment #19)
> > Can you post a screenshot of how dev edition handles plug-ins in the url
> > bar? I'm not familiar with it.
> 
> Currently, there is no arrow in Developer Edition but neither there is a
> separator.
> 
> > For reg. version, I think we should keep the arrow since we still want to
> > keep some seperation from the non-control center items that are temporary
> > (ie. plug-in notification) and the control center items (ie. lock and shield
> > - most of time just lock). We also still need the arrow for other
> > notifications like the key for passwords.
> 
> Wouldn't a regular vertical separator work as well for this purpose?
> 
> It seems in the end this worked well in Developer Edition even with no
> separation at all. I'm asking because a plain separator would solve some
> technical challenges and make the number of combinations we have to handle
> less insane by aligning our editions (look at the test matrix in bug 1180213
> if you haven't already!).
> 
> > Its better to have differentiation
> > until we get to the updated control center icon behavior post 42.
> 
> I'm not familiar with this, what would this new behavior be exactly?

I don't think no seperator or a line seperator is sufficient. If it makes it easier technically, leave regular Fx as is and use a line seperator for Dev edition. It definitely needs some differentiation between the control center anchor and the plug-in. The plug-in isn't persistant, the control center is.

I can review 43 plans in stand up - easier than via this bug to give you a sense of where we're going.

Comment 22

[:Paolo Amadini]

(In reply to agrigas from comment #21)
> I don't think no seperator or a line seperator is sufficient. If it makes it
> easier technically, leave regular Fx as is and use a line seperator for Dev
> edition. It definitely needs some differentiation between the control center
> anchor and the plug-in. The plug-in isn't persistant, the control center is.

Well, I'm not sure "arrow" means "non persistent" to me... but anyways, my opinion is that if we deem the arrow necessary, we should use use it on all themes and platforms. The argument that we may use a vertical separator for Developer Edition because it's easier technically could be applied to the regular theme too.

Tangentially, if the regular theme today had a vertical separator instead of an arrow, what would you suggest to enhance the separation? An arrow, or something else?

> I can review 43 plans in stand up - easier than via this bug to give you a
> sense of where we're going.

Okay, thank you!

Comment 23

[Brian Grinstead [:bgrins]]

(In reply to agrigas from comment #21)
> (In reply to :Paolo Amadini from comment #20)
> > Created attachment 8634125 [details]
> > Notification area in Developer Edition theme
> > 
> > (In reply to agrigas from comment #19)
> > > Can you post a screenshot of how dev edition handles plug-ins in the url
> > > bar? I'm not familiar with it.
> > 
> > Currently, there is no arrow in Developer Edition but neither there is a
> > separator.
> > 
> > > For reg. version, I think we should keep the arrow since we still want to
> > > keep some seperation from the non-control center items that are temporary
> > > (ie. plug-in notification) and the control center items (ie. lock and shield
> > > - most of time just lock). We also still need the arrow for other
> > > notifications like the key for passwords.
> > 
> > Wouldn't a regular vertical separator work as well for this purpose?
> > 
> > It seems in the end this worked well in Developer Edition even with no
> > separation at all. I'm asking because a plain separator would solve some
> > technical challenges and make the number of combinations we have to handle
> > less insane by aligning our editions (look at the test matrix in bug 1180213
> > if you haven't already!).
> > 
> > > Its better to have differentiation
> > > until we get to the updated control center icon behavior post 42.
> > 
> > I'm not familiar with this, what would this new behavior be exactly?
> 
> I don't think no seperator or a line seperator is sufficient. If it makes it
> easier technically, leave regular Fx as is and use a line seperator for Dev
> edition. It definitely needs some differentiation between the control center
> anchor and the plug-in. The plug-in isn't persistant, the control center is.

Technically it'd be easiest to do the same thing in both versions.  I'd be happy with using the arrow separator for now in Dev Edition if you could create an icon for it.

Comment 24

[Ash Grigas]

(In reply to Brian Grinstead [:bgrins] from comment #23)
> (In reply to agrigas from comment #21)
> > (In reply to :Paolo Amadini from comment #20)
> > > Created attachment 8634125 [details]
> > > Notification area in Developer Edition theme
> > > 
> > > (In reply to agrigas from comment #19)
> > > > Can you post a screenshot of how dev edition handles plug-ins in the url
> > > > bar? I'm not familiar with it.
> > > 
> > > Currently, there is no arrow in Developer Edition but neither there is a
> > > separator.
> > > 
> > > > For reg. version, I think we should keep the arrow since we still want to
> > > > keep some seperation from the non-control center items that are temporary
> > > > (ie. plug-in notification) and the control center items (ie. lock and shield
> > > > - most of time just lock). We also still need the arrow for other
> > > > notifications like the key for passwords.
> > > 
> > > Wouldn't a regular vertical separator work as well for this purpose?
> > > 
> > > It seems in the end this worked well in Developer Edition even with no
> > > separation at all. I'm asking because a plain separator would solve some
> > > technical challenges and make the number of combinations we have to handle
> > > less insane by aligning our editions (look at the test matrix in bug 1180213
> > > if you haven't already!).
> > > 
> > > > Its better to have differentiation
> > > > until we get to the updated control center icon behavior post 42.
> > > 
> > > I'm not familiar with this, what would this new behavior be exactly?
> > 
> > I don't think no seperator or a line seperator is sufficient. If it makes it
> > easier technically, leave regular Fx as is and use a line seperator for Dev
> > edition. It definitely needs some differentiation between the control center
> > anchor and the plug-in. The plug-in isn't persistant, the control center is.
> 
> Technically it'd be easiest to do the same thing in both versions.  I'd be
> happy with using the arrow separator for now in Dev Edition if you could
> create an icon for it.

Can you just take the icon used in regular mode that Paolo is using?

Comment 25

[:Paolo Amadini]

(In reply to agrigas from comment #24)
> Can you just take the icon used in regular mode that Paolo is using?

No :-)

Comment 26

[Brian Grinstead [:bgrins]]

(In reply to agrigas from comment #24)
> > Technically it'd be easiest to do the same thing in both versions.  I'd be
> > happy with using the arrow separator for now in Dev Edition if you could
> > create an icon for it.
> 
> Can you just take the icon used in regular mode that Paolo is using?

See Comment 15 - it needs a fill of #171B1F to match the darker URL bar

Comment 27

[Brian Grinstead [:bgrins]]

Just leaving a ni? to keep track of what we discussed about generating the dark arrows for the DE theme.

Comment 28

[Brian Grinstead [:bgrins]]

Attachment: dark-arrow-icons.zip

the icon files for dev edition dark theme are attached

Comment 29

[Brian Grinstead [:bgrins]]

Attachment: MozReview Request: Bug 1183203 - Move urlbar-arrow assets into themes/shared;r=paolo

Bug 1183203 - Move urlbar-arrow assets into themes/shared;r=paolo

Comment 30

[Brian Grinstead [:bgrins]]

Attachment: MozReview Request: Bug 1183203 - Use dark urlbar-arrow icon for dark Dev Edition theme;r=paolo

Bug 1183203 - Use dark urlbar-arrow icon for dark Dev Edition theme;r=paolo

Comment 31

[Brian Grinstead [:bgrins]]

Attachment: MozReview Request: Bug 1183203 - Adjust positioning for identity-box in Dev Edition theme so the hover state is clipped properly;r=paolo

Bug 1183203 - Adjust positioning for identity-box in Dev Edition theme so the hover state is clipped properly;r=paolo

Comment 32

[:Paolo Amadini]

Comment on attachment 8634972 [details]
MozReview Request: Bug 1183203 - Adjust positioning for identity-box in Dev Edition theme so the hover state is clipped properly;r=paolo

https://reviewboard.mozilla.org/r/13489/#review12119

Looks good on OS X 10.9 and Windows.

::: browser/themes/osx/devedition.css:95
(Diff revision 1)
> +/* Prevent the hover styling from on the identity icon from overlapping the
> +   urlbar border. */

nit: looks like there's an additional "from" in the comment?

Comment 33

[:Paolo Amadini]

https://reviewboard.mozilla.org/r/13489/#review12125

Ship It!

Comment 34

[Brian Grinstead [:bgrins]]

remote:   https://hg.mozilla.org/integration/fx-team/rev/84cec4155399
remote:   https://hg.mozilla.org/integration/fx-team/rev/c8e9f0dc8f91
remote:   https://hg.mozilla.org/integration/fx-team/rev/e6832b935378

Comment 35

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

https://hg.mozilla.org/mozilla-central/rev/84cec4155399
https://hg.mozilla.org/mozilla-central/rev/c8e9f0dc8f91
https://hg.mozilla.org/mozilla-central/rev/e6832b935378

Comment 36

[Catalin Varga [QA][:VarCat]]

Verified as fixed using the following environment:

FF 42 DevEdition
Build Id: 20150812004006
OS: Win 7 x64, Ubuntu 12.04 x86, Mac Os X 10.9.5