Closed Bug 1180202 Opened 5 years ago Closed 5 years ago

Identity block should change background color on hover and always be separated from page address

Categories

(Firefox :: Address Bar, defect, P1)

defect
Points:
3

Tracking

()

VERIFIED FIXED
Firefox 42
Iteration:
42.1 - Jul 13
Tracking Status
firefox42 --- verified

People

(Reporter: Paolo, Assigned: Paolo)

References

Details

(Whiteboard: [fxprivacy] [campaign])

Attachments

(5 files)

In order to support multiple page status icons all linked to the Control Center, in particular the Tracking Protection shield in bug 1175689, the identity block in the address bar is being redesigned according to the mockup in attachment 8626244 [details].

In this design the identity block should always be separated from the page address. On mouse hover, instead of the current subtle highlighting, the background of the entire area changes.
Flags: qe-verify+
Flags: firefox-backlog+
Blocks: 1180207
Blocks: 1180213
Bug 1180202 - Identity block should change background color on hover and always be separated from page address.
Attachment #8629382 - Flags: review?(ttaubert)
Rank: 2
This does not aim to be pixel perfect yet, for example we don't have exact color specifications for every case yet, but should work acceptably on the platforms where I tested it:

- Windows 7 - Default theme
- Windows 7 - High Contrast White theme
- Windows 7 - High Contrast Black theme
- Mac OS X 10.9 - HiDPI
- Linux

On each platform, I tested some of the following variants:

- Disabled
- Insecure
- HTTPS DV
- HTTPS EV
- Internal product page (Nightly color only)

I also tested some of those combinations with and without the notification icons area.

I've tested in popup windows as well (without navigation) and in normal windows with various states of the forward button.

I've only tested the above in LTR mode, with the light theme, and no lightweight theme.

See bug 1180213 for the full test matrix.
Comment on attachment 8629382 [details]
MozReview Request: Bug 1180202 - Identity block should change background color on hover and always be separated from page address. r=ttaubert

https://reviewboard.mozilla.org/r/12597/#review11117

On OSX I can see a top and bottom white border:

http://i.imgur.com/bVPirv3.png

Are we going to separate the block displaying the organization name in a separate bug? In the mockup that block doesn't have a grey background.

::: browser/themes/linux/browser.css:951
(Diff revision 1)
> +  margin-bottom: -1px;
> +  -moz-margin-start: -1px;

margin: -1px 0;

::: browser/themes/linux/browser.css:954
(Diff revision 1)
> +  padding-top: 2px;
> +  padding-bottom: 2px;

padding: 2px 0;

::: browser/themes/osx/browser.css:1650
(Diff revision 1)
>    -moz-margin-end: 3px;
> -  padding-top: 1px;
> -  padding-bottom: 1px;
> +  margin-top: -1px;
> +  margin-bottom: -1px;

margin: -1px 0;
-moz-margin-end: 3px;

::: browser/themes/osx/browser.css:1653
(Diff revision 1)
> +  padding-top: 2px;
> +  padding-bottom: 2px;
>    -moz-padding-start: 4px;
> -  -moz-padding-end: 0;
> +  -moz-padding-end: 4px;

padding: 2px 4px;
Attachment #8629382 - Flags: review?(ttaubert)
(In reply to Tim Taubert [:ttaubert] from comment #4)
> On OSX I can see a top and bottom white border:
> http://i.imgur.com/bVPirv3.png

Thanks, that's likely only on 10.10 only so I'll have to make the margins conditional on the OSX version.

> Are we going to separate the block displaying the organization name in a
> separate bug? In the mockup that block doesn't have a grey background.

Yes, bug 1180207, which has lower priority.

> ::: browser/themes/linux/browser.css:951
> (Diff revision 1)
> > +  margin-bottom: -1px;
> > +  -moz-margin-start: -1px;
> 
> margin: -1px 0;

I'll check that the margin shorthands don't reset other margins set in lower precedence rules.
(In reply to :Paolo Amadini from comment #5)
> Thanks, that's likely only on 10.10 only so I'll have to make the margins
> conditional on the OSX version.

Hm, although I wonder whether we still need a border, maybe darker, since the background of the toolbar area is also grey. I can't test on that version of OS X so I'll have to guess.
QA Contact: mwobensmith
Comment on attachment 8629382 [details]
MozReview Request: Bug 1180202 - Identity block should change background color on hover and always be separated from page address. r=ttaubert

Bug 1180202 - Identity block should change background color on hover and always be separated from page address. r=ttaubert
Attachment #8629382 - Attachment description: MozReview Request: Bug 1180202 - Identity block should change background color on hover and always be separated from page address. → MozReview Request: Bug 1180202 - Identity block should change background color on hover and always be separated from page address. r=ttaubert
Attachment #8629382 - Flags: review?(ttaubert)
https://reviewboard.mozilla.org/r/12597/#review11393

Tim can you test whether this looks acceptable on OS X 10.10? We're not aiming for pixel perfection yet.
(In reply to :Paolo Amadini from comment #8)
> Tim can you test whether this looks acceptable on OS X 10.10? We're not
> aiming for pixel perfection yet.

Instead of overriding all of the style it seems that we only need "margin: -2px 0" to let it reach the top and bottom border. The design [1] uses a darker grey than your patch, this makes the identity box flow into the browser chrome [2].

The only things we should change for 10.10 are the margin and the background color. Or pick a background color that's darker in general?

[1] http://i.imgur.com/C23n2Tx.png
[2] http://i.imgur.com/eDG4KAM.png
(In reply to Tim Taubert [:ttaubert] from comment #9)
> The design [1] uses a
> darker grey than your patch, this makes the identity box flow into the
> browser chrome [2].

The patch has the same grey value as the mockup, it's the surrounding chrome in the mockup that is lighter.

> The only things we should change for 10.10 are the margin and the background
> color. Or pick a background color that's darker in general?

Hm, the label text already has a reduced contrast with the current background value, a darker one would reduce contrast further. Is the border so ugly as an interim solution? Do you think we should block landing this patch on an updated design from UX for OS X 10.10?
We'll use a different color for OSX 10.10 and we're now waiting on Ashley to provide the value.
(In reply to :Paolo Amadini from comment #11)
> We'll use a different color for OSX 10.10 and we're now waiting on Ashley to
> provide the value.

Option #1 F0EDED
https://www.dropbox.com/s/qclyojzeuu7j47x/Screenshot%202015-07-08%2011.45.37.png?dl=0

Option #2 EEEEEE
https://www.dropbox.com/s/uz1r5uer19gsuxl/Screenshot%202015-07-08%2011.46.07.png?dl=0
(In reply to agrigas from comment #12)
> Option #1 F0EDED
> https://www.dropbox.com/s/qclyojzeuu7j47x/Screenshot%202015-07-08%2011.45.37.
> png?dl=0

Do you think it's fine if we use this lighter color on all platforms?
(In reply to :Paolo Amadini from comment #13)
> (In reply to agrigas from comment #12)
> > Option #1 F0EDED
> > https://www.dropbox.com/s/qclyojzeuu7j47x/Screenshot%202015-07-08%2011.45.37.
> > png?dl=0
> 
> Do you think it's fine if we use this lighter color on all platforms?

Can you post screenshots of each? I can't make a call without seeing how it looks...
Here are the two colors for comparison on OS X 10.9. Do you need the screenshots for the other platforms or are you fine with making the call given these? Other screenshots will require some more time to prepare. There's also bug 1180213 already on file for finding things like the exact color for each platform if it needs to be fine-tuned.
Comment on attachment 8629382 [details]
MozReview Request: Bug 1180202 - Identity block should change background color on hover and always be separated from page address. r=ttaubert

Bug 1180202 - Identity block should change background color on hover and always be separated from page address. r=ttaubert
(In reply to :Paolo Amadini from comment #16)
> Created attachment 8631126 [details]
> Screenshot on Mac OS X 10.9, darker color
> 
> Here are the two colors for comparison on OS X 10.9. Do you need the
> screenshots for the other platforms or are you fine with making the call
> given these? Other screenshots will require some more time to prepare.
> There's also bug 1180213 already on file for finding things like the exact
> color for each platform if it needs to be fine-tuned.

Lets go with the lighter color on OSX 10. I can't make the call on the other platforms without seeing screenshots
Comment on attachment 8629382 [details]
MozReview Request: Bug 1180202 - Identity block should change background color on hover and always be separated from page address. r=ttaubert

Bug 1180202 - Identity block should change background color on hover and always be separated from page address. r=ttaubert
Comment on attachment 8629382 [details]
MozReview Request: Bug 1180202 - Identity block should change background color on hover and always be separated from page address. r=ttaubert

https://reviewboard.mozilla.org/r/12597/#review11501

Not a huge fan of the current look (http://i.imgur.com/BI1HNQD.png) but let's iterate, maybe it looks better with the icons grouped :)

The one issue I found though is that the focus border does NOT overlap the grey box and that looks rather shitty: http://i.imgur.com/t6OVpWy.png. Sorry for pushing back again, I wish I had found this earlier :(

::: browser/themes/osx/browser.css:1649
(Diff revision 4)
> +@media not all and (-moz-mac-yosemite-theme) {
> +  #identity-box {

Do we really need that negated media query? Shouldn't the margin and padding just be properly overridden on 10.10?
Attachment #8629382 - Flags: review?(ttaubert)
(In reply to Tim Taubert [:ttaubert] from comment #20)
> The one issue I found though is that the focus border does NOT overlap the
> grey box and that looks rather shitty: http://i.imgur.com/t6OVpWy.png. Sorry
> for pushing back again, I wish I had found this earlier :(

Hm, maybe I can fix that by reducing the margins when the textbox has focus.

> Do we really need that negated media query?

No, I just thought it made the alternative clearer, but I can just let the second declaration override the margins.
Comment on attachment 8629382 [details]
MozReview Request: Bug 1180202 - Identity block should change background color on hover and always be separated from page address. r=ttaubert

Bug 1180202 - Identity block should change background color on hover and always be separated from page address. r=ttaubert
Attachment #8629382 - Flags: review?(ttaubert)
Comment on attachment 8629382 [details]
MozReview Request: Bug 1180202 - Identity block should change background color on hover and always be separated from page address. r=ttaubert

Bug 1180202 - Identity block should change background color on hover and always be separated from page address. r=ttaubert
I chose to repeat moz-margin-end in all declarations due to specificity issues, for the rest the case with the focus ring looks slightly better on OS X 10.9 as well.
Comment on attachment 8629382 [details]
MozReview Request: Bug 1180202 - Identity block should change background color on hover and always be separated from page address. r=ttaubert

https://reviewboard.mozilla.org/r/12597/#review11517

LGTM, thanks!
Attachment #8629382 - Flags: review?(ttaubert) → review+
Moving your changes to the right file and removing the old one.
Attachment #8631675 - Flags: review?(paolo.mozmail)
Attachment #8631675 - Flags: review?(paolo.mozmail) → review+
Depends on: 1183203
Depends on: 1183774
Whiteboard: [fxprivacy] → [fxprivacy] [campaign]
Verified as fixed using the following environment:

FF 42
Build Id: 20150729030208
OS: Win 7 x64, Mac OS X 10.10.4, Ubuntu 14.04 x86
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.