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

VERIFIED FIXED in Firefox 41

Status

()

defect
P1
normal
Rank:
1
VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: ttaubert, Assigned: ttaubert)

Tracking

Trunk
Firefox 41
Points:
3
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify +

Firefox Tracking Flags

(firefox41 verified)

Details

(Whiteboard: [fxprivacy])

Attachments

(4 attachments, 4 obsolete attachments)

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.
Flags: qe-verify+
Flags: firefox-backlog+
Whiteboard: [fxprivacy]
Blocks: 1168415
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Iteration: --- → 41.2 - Jun 8
Priority: -- → P1
No longer blocks: 1168415
Blocks: 1168883
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?
Attachment #8612429 - Flags: review?(gijskruitbosch+bugs)
This patch moves the #identity-popup styles to separate files.
Attachment #8612431 - Flags: review?(gijskruitbosch+bugs)
The two patches posted until here do NOT change any code. It's really only copy-and-paste.
The new design has no help button anymore, let's get rid of it.
Attachment #8612438 - Flags: review?(gijskruitbosch+bugs)
This patch restyles the "More Information" button.
Attachment #8612443 - Flags: review?(gijskruitbosch+bugs)

Comment 7

4 years ago
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.
Attachment #8612429 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 8

4 years ago
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?
Attachment #8612431 - Flags: review?(gijskruitbosch+bugs) → review+

Updated

4 years ago
Attachment #8612438 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 9

4 years ago
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?
Attachment #8612443 - Flags: review?(gijskruitbosch+bugs) → feedback+
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?
Attachment #8612429 - Flags: review?(ted)
(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.
(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.
Applied feedback except :focus styles. Will have to talk to Aislinn to find out what we can do about that.
Attachment #8612443 - Attachment is obsolete: true
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.
Attachment #8613484 - Attachment is obsolete: true
Attachment #8613551 - Flags: review?(gijskruitbosch+bugs)

Comment 15

4 years ago
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.
Attachment #8613551 - Flags: review?(gijskruitbosch+bugs)
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.
Attachment #8613551 - Attachment is obsolete: true
Attachment #8613578 - Flags: review?(gijskruitbosch+bugs)

Comment 17

4 years ago
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.
Attachment #8613578 - Flags: review?(gijskruitbosch+bugs) → review+
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.
Attachment #8612429 - Flags: review?(ted) → review-
You're right, we don't need them for now. I can re-add them if needed later.
Attachment #8612429 - Attachment is obsolete: true
Attachment #8614154 - Flags: review?(ted)
Attachment #8614154 - Flags: review?(ted) → review+
Depends on: 1171085
Rank: 1
QA Contact: catalin.varga
No longer depends on: 1173755
+    <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.
Depends on: 1180642
Verified as fixed using:

FF 41 
Build Id: 20150715095519
OS: WIn 7 x64, Mac Os X 10.9.5, Ubuntu 14.04 x64
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.