[Control Center] Style the security subview's More Information button like a footer

VERIFIED FIXED in Firefox 47

Status

()

defect
P1
normal
VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: dao, Assigned: nhnt11)

Tracking

(Depends on 1 bug, Blocks 1 bug)

Trunk
Firefox 47
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox43 affected, firefox47 verified)

Details

(Whiteboard: [fxprivacy])

Attachments

(2 attachments, 5 obsolete attachments)

Reporter

Updated

4 years ago
Blocks: 1196053
Whiteboard: [fxprivacy]
Whiteboard: [fxprivacy] → [fxprivacy] [triage]
Flags: qe-verify+
Priority: -- → P3
Whiteboard: [fxprivacy] [triage] → [fxprivacy]
Do we want this in FF 42, considering the merge from the next week?
Reporter

Comment 2

4 years ago
Wanted, not strictly required, I think.
Marco,

This bug actually makes the security subpanel for a page with Mixed Content look bad, so we may consider moving this to a P1 or P2.

ex: see subview here https://people.mozilla.org/~tvyas/mixedcontent.html and here https://people.mozilla.org/~tvyas/mixedboth.html
Flags: needinfo?(mmucci)
(In reply to Tanvi Vyas [:tanvi] from comment #3)
> Marco,
> 
> This bug actually makes the security subpanel for a page with Mixed Content
> look bad, so we may consider moving this to a P1 or P2.
> 
> ex: see subview here https://people.mozilla.org/~tvyas/mixedcontent.html and
> here https://people.mozilla.org/~tvyas/mixedboth.html

Marking for triage so team can review at daily update meeting.
Flags: needinfo?(mmucci)
Whiteboard: [fxprivacy] → [fxprivacy] [triage]
Priority: P3 → P2
Whiteboard: [fxprivacy] [triage] → [fxprivacy]
Blocks: 1216897
Priority: P2 → P3
Assignee

Comment 5

3 years ago
Posted patch Patch (obsolete) — Splinter Review
Based on https://www.dropbox.com/s/z7n0lda2cr5zaci/Screenshot%202015-08-31%2016.32.09.png?dl=0.

I assumed the similarity to the "Change Search Settings" button in the search panel was intentional, so I copied the CSS from that.
Assignee: nobody → nhnt11
Status: NEW → ASSIGNED
Attachment #8717732 - Flags: review?(paolo.mozmail)
Assignee

Comment 6

3 years ago
Posted image Screenshots (obsolete) —
Top to bottom: default, :hover, :hover:active.
Assignee

Comment 7

3 years ago
Posted patch Patch v1.1 (obsolete) — Splinter Review
Missed one style, sorry :(
Attachment #8717732 - Attachment is obsolete: true
Attachment #8717732 - Flags: review?(paolo.mozmail)
Attachment #8717734 - Flags: review?(paolo.mozmail)
Assignee

Comment 8

3 years ago
Posted image Screenshots v1.1
With patch 1.1 applied. Again - top to bottom: default, :hover, :hover:active.
Attachment #8717733 - Attachment is obsolete: true
Assignee

Comment 9

3 years ago
Posted patch Patch v1.2 (obsolete) — Splinter Review
Added a 1em minimum margin above the footer for cases where the body has a lot of content.
Attachment #8717734 - Attachment is obsolete: true
Attachment #8717734 - Flags: review?(paolo.mozmail)
Attachment #8717738 - Flags: review?(paolo.mozmail)

Comment 10

3 years ago
Comment on attachment 8717738 [details] [diff] [review]
Patch v1.2

Looks good, but Jared will be a better reviewer for the theme changes.
Attachment #8717738 - Flags: review?(paolo.mozmail) → review?(jaws)
Assignee

Comment 11

3 years ago
Posted patch Patch v2 (obsolete) — Splinter Review
Add rules for :focus, removed a couple of redundant styles.
Attachment #8717738 - Attachment is obsolete: true
Attachment #8717738 - Flags: review?(jaws)
Attachment #8718081 - Flags: review?(jaws)
Comment on attachment 8718081 [details] [diff] [review]
Patch v2

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

I had to rebase this patch a small amount in panel.inc.xul, please update rebase your local tree in your next revision. Also, please use mozreview for requesting review as it allows me to expand the diff to get more context.

This rule in panel.inc.css is cutting off the bottom-right of the button,
#identity-popup-multiView > .panel-viewcontainer > .panel-viewstack > .panel-subviews {
  background: var(--panel-arrowcontent-background);
  border-bottom-right-radius: 3.5px;
  padding: 0;
}

::: browser/themes/shared/controlcenter/panel.inc.css
@@ +279,5 @@
> +
> +#identity-popup-securityView-footer > button {
> +  -moz-appearance: none;
> +  margin: 0;
> +  border-top: 1px solid var(--panel-separator-color);

The other borders need to have border-width:0; set on them.

The top border of this button doesn't look right. Looking closely it looks like a transparent row of pixels, though they may just be a lot lighter than the button right next to it. See http://screencast.com/t/KPCUGEBC6tP for a screenshot of what this patch looks in my local build.

@@ +281,5 @@
> +  -moz-appearance: none;
> +  margin: 0;
> +  border-top: 1px solid var(--panel-separator-color);
> +  padding: 8px 20px;
> +  color: #000;

What was wrong with color: ButtonText? On Windows this is using background-color: ThreeDFace because this patch only applies a background-color on the footer, not the <button>.

Nonetheless, please keep background-color and color defined next to each other. In this case, you have background-color defined on the #identity-popup-securityView-footer but the color is defined on the <button> child of the footer.
Attachment #8718081 - Flags: review?(jaws) → review-
Assignee

Comment 14

3 years ago
I've tested this patch on Ubuntu and Win8, and the button looks fine to me.

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #12)
> Also, please use mozreview for
> requesting review as it allows me to expand the diff to get more context.

First time using MozReview, let me know if I did it right or if there's something I missed.

> This rule in panel.inc.css is cutting off the bottom-right of the button,
> #identity-popup-multiView > .panel-viewcontainer > .panel-viewstack >
> .panel-subviews {
>   background: var(--panel-arrowcontent-background);
>   border-bottom-right-radius: 3.5px;
>   padding: 0;
> }

Thanks for catching this! The border-radius seems to be an OS X thing (the other panel styles render it visually absent on the other platforms) so I moved it to the OS X specific stylesheet.

> ::: browser/themes/shared/controlcenter/panel.inc.css
> @@ +279,5 @@
> > +
> > +#identity-popup-securityView-footer > button {
> > +  -moz-appearance: none;
> > +  margin: 0;
> > +  border-top: 1px solid var(--panel-separator-color);
> 
> The other borders need to have border-width:0; set on them.
> 
> The top border of this button doesn't look right. Looking closely it looks
> like a transparent row of pixels, though they may just be a lot lighter than
> the button right next to it. See http://screencast.com/t/KPCUGEBC6tP for a
> screenshot of what this patch looks in my local build.

I've set border: none; and set the border-top color to #ccc instead of the panel separator color. This value is used elsewhere as well (notably in the search suggestions panel) and contrasts nicely with the footer button.

> @@ +281,5 @@
> > +  -moz-appearance: none;
> > +  margin: 0;
> > +  border-top: 1px solid var(--panel-separator-color);
> > +  padding: 8px 20px;
> > +  color: #000;
> 
> What was wrong with color: ButtonText? On Windows this is using
> background-color: ThreeDFace because this patch only applies a
> background-color on the footer, not the <button>.
> 
> Nonetheless, please keep background-color and color defined next to each
> other. In this case, you have background-color defined on the
> #identity-popup-securityView-footer but the color is defined on the <button>
> child of the footer.

Hmm, I copied over the colors from the CSS for the search suggestions panel, and it seems to set the background-color of the button's container, and kept the button transparent. The hover and active colors for the button apply a value with alpha, which effectively darkens the color of the footer underneath.

Would you prefer that I set the color of the button directly, instead of having the colors apply over each other?

In any case, I thought moz-appearance: none would get rid of any background color, my bad. I've forced it to transparent now.
Assignee

Updated

3 years ago
Attachment #8718081 - Attachment is obsolete: true
Comment on attachment 8725042 [details]
MozReview Request: Bug 1202280 - [Control Center] Style the security subview's More Information button like a footer. r=jaws

https://reviewboard.mozilla.org/r/37285/#review34183

Sorry for the delay on reviewing this, but it looks good :) r=me
Attachment #8725042 - Flags: review?(jaws) → review+
Assignee

Comment 16

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/c36859eaec38c5f0aeb886b97be0950f7a0dd259
Bug 1202280 - [Control Center] Style the security subview's More Information button like a footer. r=jaws

Comment 17

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c36859eaec38
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Iteration: --- → 47.3 - Mar 7
Priority: P3 → P1
QA Contact: paul.silaghi
Looks fine, except a small dotted border on Windows - http://i.imgur.com/Og1AavU.png.
Verified fixed FX 47.0a1 (2016-03-06) Win 7, Ubuntu 14.04, OS X 10.10.5.
Let me know if you want the above issue filed separately.
Status: RESOLVED → VERIFIED
Flags: needinfo?(nhnt11)
Assignee

Comment 19

3 years ago
Hmm, did we not want the focus ring?

(In reply to Paul Silaghi, QA [:pauly] from comment #18)
> Let me know if you want the above issue filed separately.

Yeah, that would be great, thanks!
Flags: needinfo?(nhnt11)
Depends on: 1254909
Reporter

Updated

3 years ago
Depends on: 1296945
Reporter

Updated

3 years ago
Depends on: 1298016
You need to log in before you can comment on or make changes to this bug.