Closed Bug 994265 Opened 10 years ago Closed 10 years ago

In-content preferences : some buttons are bigger than others

Categories

(Firefox :: Settings UI, defect)

All
Windows 7
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 32

People

(Reporter: u428464, Assigned: Paenglab)

References

Details

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

Attachments

(4 files, 12 obsolete files)

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
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
Blocks: 718011, 738796
Attached 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-
Attached 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+
(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)
Depends on: 993524
Attached 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)
Attached 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-
Attached 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+
Attached 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.
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.
Attached 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+
Attached 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.
(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)
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+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/76604cbff5ab
Status: ASSIGNED → RESOLVED
Closed: 10 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+]
Attached 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 → ---
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).
(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
(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).
(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.
Attached patch bug994265fix.patch (obsolete) — Splinter Review
This is a different approach without using ::before.
Attachment #8416446 - Flags: review?(jaws)
needinfo no more needed.
Flags: needinfo?(jaws)
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+
Attached 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-
Attached 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)
Attached 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)
FWIW here's what latest mockup looks like : http://people.mozilla.org/~mmaslaney/incontent/Preferences%20-%20General@2x.png
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-
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+
Attachment #8412773 - Attachment description: inContentButtonFix.patch v6 → inContentButtonFix.patch v6 checked-in
https://hg.mozilla.org/mozilla-central/rev/45d8933aefc0
Status: REOPENED → RESOLVED
Closed: 10 years ago10 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.