Closed Bug 1022578 Opened 10 years ago Closed 9 years ago

Can't tell what category is selected in about:preferences when using High Contrast mode

Categories

(Firefox :: Settings UI, defect)

30 Branch
x86_64
Windows 8.1
defect
Not set
normal
Points:
1

Tracking

()

VERIFIED FIXED
Firefox 36
Iteration:
37.1
Tracking Status
firefox36 --- verified
firefox37 --- verified

People

(Reporter: Unfocused, Assigned: Paenglab)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

Attached image Screenshot
There's no indication as to which category is currently selected in about:preferences when using High Contrast mode. See attached screenshot and try to figure out what's selected.
Flags: firefox-backlog+
No longer blocks: 1022597
Not a hard blocker since the text is still visible for clicking and about:addons has the same problem.
No longer blocks: ship-incontent-prefs
Blocks: 718011
Attached patch bug1022578.patchSplinter Review
This patch uses borders instead of box-shadows for tabs and categories. With this HC-themes are showing the thick border in their default text color. I had to play with the padding instead using a transparent border when not active because HC-themes are showing the border still with their text color (see the border around the checkmark text on screenshot of comment 3).
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #8527314 - Flags: review?(jaws)
Attached image HC-screenshot.png
Screenshot with patch applied on HC-theme.
Comment on attachment 8527314 [details] [diff] [review]
bug1022578.patch

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

This is really close. I think with the proposed change below it should be ready for an r+.

::: toolkit/themes/shared/in-content/common.inc.css
@@ +94,5 @@
>  
>  xul|tab {
>    -moz-appearance: none;
>    margin-top: 0;
> +  padding: 4px 20px;

We shouldn't add internal vertical padding for this. Instead, we can do:
border: 4px solid transparent;

And then for the [selected] tab, we can just set the border-bottom-color to #ff9500.
Attachment #8527314 - Flags: review?(jaws) → review-
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #4)
> Comment on attachment 8527314 [details] [diff] [review]
> bug1022578.patch
> 
> Review of attachment 8527314 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is really close. I think with the proposed change below it should be
> ready for an r+.
> 
> ::: toolkit/themes/shared/in-content/common.inc.css
> @@ +94,5 @@
> >  
> >  xul|tab {
> >    -moz-appearance: none;
> >    margin-top: 0;
> > +  padding: 4px 20px;
> 
> We shouldn't add internal vertical padding for this. Instead, we can do:
> border: 4px solid transparent;
> 
> And then for the [selected] tab, we can just set the border-bottom-color to
> #ff9500.

I had to use the padding because transparent borders don't work for High Contrast themes. They show the transparent border with the foreground color which ends in a always shown 4px line. That's also why every .checkbox-label-box and .radio-label-box has always a border around them on HC-themes (I'm planning to file a bug for this).
Comment on attachment 8527314 [details] [diff] [review]
bug1022578.patch

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

I filed bug 1105364 to get the issue that you found fixed. It doesn't make sense to me that 'transparent' gets an opaque color in high contrast mode.

If you're OK with the proposed change below, upload a new patch and request for review. Nice work :)

::: toolkit/themes/shared/in-content/common.inc.css
@@ +94,5 @@
>  
>  xul|tab {
>    -moz-appearance: none;
>    margin-top: 0;
> +  padding: 4px 20px;

Ok, this should be padding: 0 20px 4px; No need to add padding-top here.
Attached image tabs.png
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #6)
> Comment on attachment 8527314 [details] [diff] [review]
> bug1022578.patch
> 
> Review of attachment 8527314 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/themes/shared/in-content/common.inc.css
> @@ +94,5 @@
> >  
> >  xul|tab {
> >    -moz-appearance: none;
> >    margin-top: 0;
> > +  padding: 4px 20px;
> 
> Ok, this should be padding: 0 20px 4px; No need to add padding-top here.

I added the top padding to center the text between the two horizontal lines. The screenshot shows the two tabs on left and right with your proposal and the middle one with padding-top: 4px;. Which one should be used?
Comment on attachment 8527314 [details] [diff] [review]
bug1022578.patch

Ok, I'm fine with the uniform top and bottom padding.
Attachment #8527314 - Flags: review- → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3fec8eb74f10
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: p=1[fixed-in-fx-team] → p=1
Target Milestone: --- → Firefox 36
Iteration: --- → 37.1
Points: --- → 1
Flags: qe-verify?
Whiteboard: p=1
Flags: qe-verify? → qe-verify+
QA Contact: camelia.badau
Verified fixed on Windows 8.1 64bit and 32bit using latest Nightly 37.0a1 (buildID: 20141204030201) and latest Aurora 36.0a2(buildID: 20141204004001).
Status: RESOLVED → VERIFIED
Blocks: 1022597
You need to log in before you can comment on or make changes to this bug.