In-content preferences : some buttons are bigger than others

VERIFIED FIXED in Firefox 32

Status

()

defect
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: ge3k0s, Assigned: Paenglab)

Tracking

Trunk
Firefox 32
All
Windows 7
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: p=0 s=it-32c-31a-30b.2 [qa!])

Attachments

(4 attachments, 12 obsolete attachments)

23.34 KB, image/png
Details
4.24 KB, patch
jaws
: review+
Details | Diff | Splinter Review
1.12 MB, application/x-zip-compressed
Details
7.63 KB, patch
jaws
: review+
Details | Diff | Splinter Review
Reporter

Description

5 years ago
Some buttons (notably "Exceptions" and "Browse" ) are bigger than other ones. I see this on Windows, but I don't know if it's the case on other platforms
Reporter

Updated

5 years ago
Blocks: 718011, 738796
Assignee

Comment 1

5 years ago
Posted patch inContentButtonFix.patch (obsolete) — Splinter Review
Bug 991073 removed this rules from shared file but didn't add them to Windows specific files. This patch adds then now.

I checked on OS X and this rules aren't needed there.
Assignee: nobody → richard.marti
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #8404175 - Flags: review?(jaws)
That was intentional. Setting margin-top and margin-bottom feels like the wrong way to go about making the buttons have a specific height.
Attachment #8404175 - Flags: review?(jaws) → review-
Assignee

Comment 3

5 years ago
Posted patch inContentButtonFix.patch (obsolete) — Splinter Review
Is this approach better?
Attachment #8404175 - Attachment is obsolete: true
Attachment #8404202 - Flags: review?(jaws)
Comment on attachment 8404202 [details] [diff] [review]
inContentButtonFix.patch

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

Yeah, this is better. But please use "-moz-box-align: center;" instead of the attribute.
Attachment #8404202 - Flags: review?(jaws) → feedback+
Assignee

Comment 5

5 years ago
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #4)
> 
> Yeah, this is better. But please use "-moz-box-align: center;" instead of
> the attribute.

Should I put it in shared or only Windows?
Flags: needinfo?(jaws)
Probably shared, but we'll need to test that it is correct.
Flags: needinfo?(jaws)

Updated

5 years ago
Depends on: 993524
Assignee

Comment 7

5 years ago
Posted patch inContentButtonFix.patch (obsolete) — Splinter Review
I checked it on Linux and I'm seeing no difference.

A try build for all three platforms can be found here: https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/richard.marti@gmail.com-418177f66966
Attachment #8404202 - Attachment is obsolete: true
Attachment #8404864 - Flags: review?(jaws)
Assignee

Comment 8

5 years ago
Posted patch inContentButtonFix.patch (obsolete) — Splinter Review
Oops, the comment in patch was a bit weird.
Attachment #8404864 - Attachment is obsolete: true
Attachment #8404864 - Flags: review?(jaws)
Attachment #8404869 - Flags: review?(jaws)
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #4)
> Yeah, this is better. But please use "-moz-box-align: center;" instead of
> the attribute.

Why, out of curiosity?
(In reply to :Gavin Sharp (email gavin@gavinsharp.com) from comment #9)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #4)
> > Yeah, this is better. But please use "-moz-box-align: center;" instead of
> > the attribute.
> 
> Why, out of curiosity?

It's easier for themes to override the CSS style than it is for the attribute.
Comment on attachment 8404869 [details] [diff] [review]
inContentButtonFix.patch

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

The Popup Exceptions button is still too big, http://screencast.com/t/kk5pbZDj
Attachment #8404869 - Flags: review?(jaws) → review-
Assignee

Comment 12

5 years ago
Posted patch inContentButtonFix.patch v2 (obsolete) — Splinter Review
Hmm, missed this one. I hope I have now all.
Attachment #8404869 - Attachment is obsolete: true
Attachment #8406299 - Flags: review?(jaws)
Flags: firefox-backlog?
Flags: firefox-backlog? → firefox-backlog+
I wish we had a more generic solution to this. I'm looking for one now.
Comment on attachment 8406299 [details] [diff] [review]
inContentButtonFix.patch v2

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

::: browser/themes/windows/preferences/in-content/preferences.css
@@ +38,5 @@
> +#popupPolicyRow,
> +#savePasswordsBox,
> +#masterPasswordBox {
> +  -moz-box-align: center;
> +}

It seems that the following will work and should be better for the future,

groupbox > grid > rows > row,
groupbox > hbox,
radiogroup > hbox {
  -moz-box-align: center;
}

but it would be better if we didn't have to have this hack in here in the first place. The increased size of the checkboxes is causing it. Would you like to look in to fixing the checkbox design so it doesn't introduce this issue?
Attachment #8406299 - Flags: review?(jaws) → feedback+
Assignee

Comment 15

5 years ago
Posted patch inContentButtonFix.patch v3 (obsolete) — Splinter Review
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #14)
> Comment on attachment 8406299 [details] [diff] [review]
> inContentButtonFix.patch v2
> 
> Review of attachment 8406299 [details] [diff] [review]:
> 
> It seems that the following will work and should be better for the future,
> 
> groupbox > grid > rows > row,
> groupbox > hbox,
> radiogroup > hbox {
>   -moz-box-align: center;
> }
> 
> but it would be better if we didn't have to have this hack in here in the
> first place. The increased size of the checkboxes is causing it. Would you
> like to look in to fixing the checkbox design so it doesn't introduce this
> issue?

I see no other solution than your proposal. The top/bottom margins to the checkboxes and radiobuttons are to make the needed spacing between the lines. And thus are doing the buttons growing in hboxes.

I had to remove the '>' in groupbox > hbox to apply also to #cookiesBox or I'd need to add: groupbox > deck > vbox > vbox > vbox > hbox. But then I could also better use your rules and add #cookiesBox. What do you think?
Attachment #8406299 - Attachment is obsolete: true
Attachment #8409115 - Flags: review?(jaws)
(In reply to Richard Marti (:Paenglab) from comment #15)
> I see no other solution than your proposal. The top/bottom margins to the
> checkboxes and radiobuttons are to make the needed spacing between the
> lines. And thus are doing the buttons growing in hboxes.

This is the wrong way to introduce spacing between the lines. We should just remove that rule.
Assignee

Comment 17

5 years ago
What could be the better way to do this spacing?
I'm not convinced we need this extra spacing, especially since it wasn't uniform.
Assignee

Comment 19

5 years ago
Posted patch inContentButtonFix.patch v4 (obsolete) — Splinter Review
Okay, removed the spacing.
Attachment #8409115 - Attachment is obsolete: true
Attachment #8409162 - Flags: review?(jaws)
Comment on attachment 8409162 [details] [diff] [review]
inContentButtonFix.patch v4

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

Sorry for the delay. This is better, thanks. Please make the following change though.

::: browser/themes/shared/incontentprefs/preferences.css
@@ +418,5 @@
>  
>  radio:hover::before,
>  radio[selected]::before {
> +  display: block;
> +  position: relative;

We can get the same outcome if we set position:relative; on the radio and checkbox, and then set position:absolute on the generated-content. The display:block; property above can then be removed. I would prefer this route instead.
Attachment #8409162 - Flags: review?(jaws) → review+
Assignee

Comment 21

5 years ago
Posted patch inContentButtonFix.patch v5 (obsolete) — Splinter Review
I'm asking again for review to be sure it's like you wished.
Attachment #8409162 - Attachment is obsolete: true
Attachment #8412147 - Flags: review?(jaws)
Comment on attachment 8412147 [details] [diff] [review]
inContentButtonFix.patch v5

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

With this patch applied, the dots and checkmarks are out of place: http://screencast.com/t/jmrDUIYr
Attachment #8412147 - Flags: review?(jaws) → review-
You should remove the margin-top:-5px from checkbox[checked]::before and also the margin-top:-6px from radio[selected]::before.
That screenshot in comment #22 in on Windows by the way.
Assignee

Comment 25

5 years ago
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #23)
> You should remove the margin-top:-5px from checkbox[checked]::before and
> also the margin-top:-6px from radio[selected]::before.

On my Win8 tablet with scale 1.35 I'm seeing the same as you. With removing the margin-top, the dots and checkmarks are out of place on my Win7 with scale 1.0. It seems using position:absolute needs for every scale a different margin-top (it's also different on Linux and OS X). With patch v4, the dots and checkmarks are on all systems correctly aligned.

With patch v5 also not all radios are correctly aligned. The "Always ask me where to save" in General/Downloads isn't correctly vertically centered.

Shouldn't we better use v4, also when it's uses the display: block?

PS: What is not good with display: block?
Flags: needinfo?(jaws)
Assignee

Comment 26

5 years ago
I don't know what was yesterday but today the negative margins aren't needed.
Attachment #8412147 - Attachment is obsolete: true
Attachment #8412773 - Flags: review?(jaws)
Attachment #8412773 - Flags: review?(jaws) → review+
Assignee

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/76604cbff5ab
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 31
Hi Liz, can you review if this bug requires any QA verification.
Flags: needinfo?(lhenry)
Whiteboard: [qa?]
Whiteboard: [qa?] → p=0 s=it-32c-31a-30b.1 [qa?]
Flags: needinfo?(lhenry)
QA Contact: camelia.badau
Whiteboard: p=0 s=it-32c-31a-30b.1 [qa?] → p=0 s=it-32c-31a-30b.1 [qa+]
Posted file issue MAC.zip
Verified fixed on Windows 7 64bit and Ubuntu 13.10 32bit using the latest Nightly 32.0a1 (buildID: 20140429030201). 

Verified on Mac OSX 10.7.5 using the latest Nightly 32.0a1 (buildID: 20140429030201) and the following issue was found: 
                 - under Privacy (Tracking section): the dots from radios are out of place
                 - under Advanced (Update and Certificates sections): the dots from radios are out of place
     See "issue MAC.zip".
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Reporter

Comment 31

5 years ago
Also note that the spacing on Windows doesn't fit the specs anymore either (the checkboxes and radio buttons are too close to each others).
Assignee

Comment 32

5 years ago
(In reply to Guillaume C. [:ge3k0s] from comment #31)
> Also note that the spacing on Windows doesn't fit the specs anymore either
> (the checkboxes and radio buttons are too close to each others).

See comment 16 - 18
Reporter

Comment 33

5 years ago
(In reply to Richard Marti (:Paenglab) from comment #32)
> (In reply to Guillaume C. [:ge3k0s] from comment #31)
> > Also note that the spacing on Windows doesn't fit the specs anymore either
> > (the checkboxes and radio buttons are too close to each others).
> 
> See comment 16 - 18

Thanks for the info. However that would be best to have UX input on that (this isn't very nice visually speaking).
Assignee

Comment 34

5 years ago
(In reply to Camelia Badau, QA [:cbadau] from comment #30)
> Created attachment 8415206 [details]
> issue MAC.zip
> 
> Verified fixed on Windows 7 64bit and Ubuntu 13.10 32bit using the latest
> Nightly 32.0a1 (buildID: 20140429030201). 
> 
> Verified on Mac OSX 10.7.5 using the latest Nightly 32.0a1 (buildID:
> 20140429030201) and the following issue was found: 
>                  - under Privacy (Tracking section): the dots from radios
> are out of place
>                  - under Advanced (Update and Certificates sections): the
> dots from radios are out of place
>      See "issue MAC.zip".

I can't say why this happens, and also why only on OS X. If someone could tell where the problem lies, this would help.
Assignee

Comment 35

5 years ago
Posted patch bug994265fix.patch (obsolete) — Splinter Review
This is a different approach without using ::before.
Attachment #8416446 - Flags: review?(jaws)
Assignee

Comment 36

5 years ago
needinfo no more needed.
Flags: needinfo?(jaws)
Assignee

Comment 37

5 years ago
I forgot to mention, I don't see the OS X issue with my latest patch.
Comment on attachment 8416446 [details] [diff] [review]
bug994265fix.patch

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

::: browser/themes/shared/incontentprefs/preferences.css
@@ +367,3 @@
>  .checkbox-check[checked] {
>    border-color: #0096dc;
> +  background-image: -moz-image-rect(url("check.png"), 0, 30, 10, 15),

Please use the full path here, "chrome://browser/skin/preferences/in-content/check.png"

@@ +367,4 @@
>  .checkbox-check[checked] {
>    border-color: #0096dc;
> +  background-image: -moz-image-rect(url("check.png"), 0, 30, 10, 15),
> +                    linear-gradient(#ffffff, rgba(255,255,255,0.8)) !important;

Let's find a way to remove the "!important" here. Which selector has more specificity than this one? It can't be the one right above it, as they are mutually exclusive due to the ":not([checked])" vs "[checked]" selectors.
Attachment #8416446 - Flags: review?(jaws) → feedback+
Assignee

Comment 39

5 years ago
Posted patch bug994265fix.patch (obsolete) — Splinter Review
Addred the full path.

The !important is needed to override this !important:
http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/windows/global/checkbox.css#83

I added a comment.
Attachment #8416446 - Attachment is obsolete: true
Attachment #8416671 - Flags: review?(jaws)
Comment on attachment 8416671 [details] [diff] [review]
bug994265fix.patch

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

Still one lingering question:

::: browser/themes/shared/incontentprefs/preferences.css
@@ +384,4 @@
>  @media (min-resolution: 2dppx) {
> +  checkbox:not([checked]):hover .checkbox-check {
> +    background-size: contain;
> +    background-image: -moz-image-rect(url("chrome://browser/skin/preferences/in-content/check@2x.png"), 0, 42, 42, 0),

Why are the dimensions of the check@2x.png changing? Related, why did the check@2x.png image change?
Attachment #8416671 - Flags: review?(jaws) → review-
Assignee

Comment 41

5 years ago
Posted patch bug994265fix.patch (obsolete) — Splinter Review
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #40)
> Comment on attachment 8416671 [details] [diff] [review]
> bug994265fix.patch
> 
> Review of attachment 8416671 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Still one lingering question:
> 
> ::: browser/themes/shared/incontentprefs/preferences.css
> @@ +384,4 @@
> >  @media (min-resolution: 2dppx) {
> > +  checkbox:not([checked]):hover .checkbox-check {
> > +    background-size: contain;
> > +    background-image: -moz-image-rect(url("chrome://browser/skin/preferences/in-content/check@2x.png"), 0, 42, 42, 0),
> 
> Why are the dimensions of the check@2x.png changing? Related, why did the
> check@2x.png image change?

This is because the .checkbox-check has a inner border size of 21px and the background-image with it's linear-gradient has to fill the whole background and the old sizes didn't fit this correctly (also the check@2x.png's real height was 20px and defined was 30px). To make it more clear for the @2x definitions I changed now also check.png to 21px * 21px and adapted the dimensions.
Attachment #8416671 - Attachment is obsolete: true
Attachment #8418898 - Flags: review?(jaws)
Assignee

Updated

5 years ago
Duplicate of this bug: 1008173
Assignee

Comment 43

5 years ago
Posted patch bug994265fix.patch (obsolete) — Splinter Review
Updated to tip.
Attachment #8418898 - Attachment is obsolete: true
Attachment #8418898 - Flags: review?(jaws)
Attachment #8420146 - Flags: review?(jaws)
Reporter

Comment 44

5 years ago
FWIW here's what latest mockup looks like : http://people.mozilla.org/~mmaslaney/incontent/Preferences%20-%20General@2x.png
Assignee

Comment 45

5 years ago
This is not declared as final and will be fixed in their bugs.
Whiteboard: p=0 s=it-32c-31a-30b.1 [qa+] → p=0 [qa+]
Comment on attachment 8420146 [details] [diff] [review]
bug994265fix.patch

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

::: browser/themes/shared/incontentprefs/preferences.css
@@ +435,4 @@
>    }
>  
> +  .checkbox-check[checked] {
> +    background-size: 21px 21px;

Please revert the changes to check.png and check@2x.png. The reason the background-image gradient isn't extending outside of the checkbox size is because the background-size for the second background-image isn't explicitly defined, therefore inheriting the background-size of the first background-image.

For example, doing this for the 2dppx images will fix the issue:
>  .checkbox-check[checked] {
>    background-size: 15px 10px, auto;
>    background-image: -moz-image-rect(url("chrome://browser/skin/preferences/in-content/check@2x.png"), 0, 60, 20, 30),
>                      linear-gradient(#ffffff, rgba(255,255,255,0.8)) !important;
>  }

With that change, this should be good-to-go.
Attachment #8420146 - Flags: review?(jaws) → review-
Assignee

Comment 47

5 years ago
Yeah, I forgot we can set the background-size differently for both images.
Attachment #8420146 - Attachment is obsolete: true
Attachment #8422700 - Flags: review?(jaws)
Comment on attachment 8422700 [details] [diff] [review]
bug994265fix.patch

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

Thanks! This is much better.
Attachment #8422700 - Flags: review?(jaws) → review+
Assignee

Updated

5 years ago
Attachment #8412773 - Attachment description: inContentButtonFix.patch v6 → inContentButtonFix.patch v6 checked-in
https://hg.mozilla.org/integration/fx-team/rev/45d8933aefc0
Keywords: checkin-needed
Whiteboard: p=0 [qa+] → p=0 [qa+][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/45d8933aefc0
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Whiteboard: p=0 [qa+][fixed-in-fx-team] → p=0 [qa+]
Whiteboard: p=0 [qa+] → p=0 s=it-32c-31a-30b.2 [qa+]
Verified fixed on Mac OSX 10.7.5 using latest Nightly 32.0a1 (buildID: 20140519030202).
Status: RESOLVED → VERIFIED
Whiteboard: p=0 s=it-32c-31a-30b.2 [qa+] → p=0 s=it-32c-31a-30b.2 [qa!]
Target Milestone: Firefox 31 → Firefox 32
You need to log in before you can comment on or make changes to this bug.