1146269 - [Control Center] New styling for "More Information" section at the bottom of the identity panel

Status: VERIFIED FIXED

(Firefox :: General, defect, P1)

Description

[Tim Taubert [:ttaubert] (inactive)]

Bug 1125230 includes a wireframe for v1.0 of the control center. The "More Information" section at the bottom was simplified to only include a single big button.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Design Document:

http://people.mozilla.org/~shorlander/mockups/Control-Center/Control-Center-i01-02.html

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1146269-Move-the-identity-panel-to-its-own-XUL-f.patch

This patch moves the #identity-popup to its own file and directory. This is all preparation for future changes coming to the panel. The moz.build files probably need build peer review?

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0002-Bug-1146269-Move-identity-panel-styles-to-their-own-.patch

This patch moves the #identity-popup styles to separate files.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

The two patches posted until here do NOT change any code. It's really only copy-and-paste.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0003-Bug-1146269-Remove-help-button-from-identity-panel.patch

The new design has no help button anymore, let's get rid of it.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0004-Bug-1146269-Update-styling-for-More-Information-butt.patch

This patch restyles the "More Information" button.

Comment 7

[:Gijs (he/him)]

Comment on attachment 8612429 [details] [diff] [review]
0001-Bug-1146269-Move-the-identity-panel-to-its-own-XUL-f.patch

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

rs=me

::: browser/components/controlcenter/moz.build
@@ +8,5 @@
> +    'content',
> +]
> +
> +with Files('**'):
> +    BUG_COMPONENT = ('Firefox', 'General')

This is kind of sad, but OK. "Device permissions" is too specific to those doorhangers, and "Location bar" seems to also not be right here. Meh.

Comment 8

[:Gijs (he/him)]

Comment on attachment 8612431 [details] [diff] [review]
0002-Bug-1146269-Move-identity-panel-styles-to-their-own-.patch

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

I'm assuming you doublechecked your copy-paste work.

See below for a suggestion, though...

::: browser/base/content/browser.xul
@@ +8,5 @@
>  
>  <?xml-stylesheet href="chrome://browser/content/browser.css" type="text/css"?>
>  <?xml-stylesheet href="chrome://browser/content/places/places.css" type="text/css"?>
>  <?xml-stylesheet href="chrome://browser/skin/devtools/common.css" type="text/css"?>
> +<?xml-stylesheet href="chrome://browser/skin/controlcenter/panel.css" type="text/css"?>

If these are just for the control panel, why don't we make them a scoped stylesheet instead?

Comment 9

[:Gijs (he/him)]

Comment on attachment 8612443 [details] [diff] [review]
0004-Bug-1146269-Update-styling-for-More-Information-butt.patch

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

Your patch removes focus styles but adds no new ones, meaning keyboard a11y is lost, at least on Mac. Please get UX to devise styles that work. Sharing the hover styles may or may not be an option.

On what platforms have you tested this?

::: browser/themes/linux/controlcenter/panel.css
@@ +73,5 @@
>    padding: 10px;
>  }
>  
>  #identity-popup-button-container {
> +  background-color: hsla(210,4%,10%,.07);

This is the same everywhere, please move it to panel.inc.css

::: browser/themes/osx/controlcenter/panel.css
@@ +111,1 @@
>  #identity-popup-more-info-button {

The styles for this button seem to be the same everywhere, so please move them to the shared file as much as possible.

@@ +113,5 @@
> +  border-top: 1px solid hsla(210,4%,10%,.14);
> +  background: transparent;
> +  -moz-appearance: none;
> +  margin-top: 5px;
> +  max-height: 0;

Why do you need a max-height?

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8612429 [details] [diff] [review]
0001-Bug-1146269-Move-the-identity-panel-to-its-own-XUL-f.patch

Ted, can you please take a look at the moz.build file changes in this patch?

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

(In reply to :Gijs Kruitbosch from comment #8)
> >  <?xml-stylesheet href="chrome://browser/content/browser.css" type="text/css"?>
> >  <?xml-stylesheet href="chrome://browser/content/places/places.css" type="text/css"?>
> >  <?xml-stylesheet href="chrome://browser/skin/devtools/common.css" type="text/css"?>
> > +<?xml-stylesheet href="chrome://browser/skin/controlcenter/panel.css" type="text/css"?>
> 
> If these are just for the control panel, why don't we make them a scoped
> stylesheet instead?

Tried doing that but it didn't want to apply my styles for some reason... Guess I'll file a follow-up.

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

(In reply to :Gijs Kruitbosch from comment #9)
> Your patch removes focus styles but adds no new ones, meaning keyboard a11y
> is lost, at least on Mac. Please get UX to devise styles that work. Sharing
> the hover styles may or may not be an option.

I copied the :hover and :active styling from the PanelUI, looks like that doesn't have :focus styles either?

> On what platforms have you tested this?

On all three.

> The styles for this button seem to be the same everywhere, so please move
> them to the shared file as much as possible.

Great, will do with pleasure. I'm still remembering some arbitrary rules where even with the same styling those rules were cloned into the respective theme files.

> Why do you need a max-height?

Ah, forgot to remote that from one of my first attempts.

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0004-Bug-1146269-Update-styling-for-More-Information-butt.patch, v2

Applied feedback except :focus styles. Will have to talk to Aislinn to find out what we can do about that.

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0004-Bug-1146269-Update-styling-for-More-Information-butt.patch, v3

Talked to Aislinn and our solution will be to not automatically focus the "More Info" button when the panel is opened. The :focus style will be the same as for :hover.

Comment 15

[:Gijs (he/him)]

Comment on attachment 8613551 [details] [diff] [review]
0004-Bug-1146269-Update-styling-for-More-Information-butt.patch, v3

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

::: browser/base/content/browser.js
@@ -7042,5 @@
>      this._identityPopup.openPopup(this._identityIcon, "bottomcenter topleft");
>    },
>  
>    onPopupShown : function(event) {
> -    document.getElementById('identity-popup-more-info-button').focus();

Sadly, this doesn't work because you can't actually tab-navigate into the popup otherwise.

There is also some scary blur handler-based magic here that would mean I would really prefer not to mess with this. I'd rather followup-bug actual focus styling if that's going to help unblock things.

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0004-Bug-1146269-Update-styling-for-More-Information-butt.patch, v4

No focus styles and no autofocus for the "More Info" button. This is a compromise to unblock further control center work. We will prioritize implementing the site information subsection in 41.3 to re-enable autofocus and fix identity panel a11y.

Comment 17

[:Gijs (he/him)]

Comment on attachment 8613578 [details] [diff] [review]
0004-Bug-1146269-Update-styling-for-More-Information-butt.patch, v4

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

ok, let's do this, but let's not forget the followup bug to get the focus story here sorted. Please file one, add the "access" keyword and cc me and :MarcoZ.

Comment 18

[(not currently active) Ted Mielczarek]

Comment on attachment 8612429 [details] [diff] [review]
0001-Bug-1146269-Move-the-identity-panel-to-its-own-XUL-f.patch

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

If you're just putting that .xul file in that directory to #include it from elsewhere you don't need any of those new moz.build files. You can put the BUG_COMPONENT bit in the parent moz.build.

Comment 19

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1146269-Move-the-identity-panel-to-its-own-XUL-f.patch, v2

You're right, we don't need them for now. I can re-add them if needed later.

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/c3607a49752e
https://hg.mozilla.org/integration/fx-team/rev/cbfa5a6f0804
https://hg.mozilla.org/integration/fx-team/rev/13a00d0dffa6
https://hg.mozilla.org/integration/fx-team/rev/1fa49d1c999b

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c3607a49752e
https://hg.mozilla.org/mozilla-central/rev/cbfa5a6f0804
https://hg.mozilla.org/mozilla-central/rev/13a00d0dffa6
https://hg.mozilla.org/mozilla-central/rev/1fa49d1c999b

Comment 22

[Pardal Freudenthal (:ShareBird)]

+    <button id="identity-popup-more-info-button" flex="1"
+            label="&identity.moreInfoLinkText2;"

Please avoid the flex, orient, align and pack inline attributes. Please use the corresponding css attributes.

Comment 23

[Catalin Varga [QA][:VarCat]]

Verified as fixed using:

FF 41 
Build Id: 20150715095519
OS: WIn 7 x64, Mac Os X 10.9.5, Ubuntu 14.04 x64