1115925 - InContent Prefs - Lack of highlight for selected tree row in High contrast mode

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect, P3)

Description

[Tim Nguyen :ntim]

We can just use border: 1px solid transparent; as a quick fix, so the selected row is distinguishable with a border (transparent -> -moz-dialog in HC).

Or we could use the windows default theme media query, but that will likely regress 3rd party themes.

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

Tim, could you take this bug as well?

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Léon, I've assigned this bug to you. The steps to fix this are listed in comment #0. Please ask if you need more details.

Comment 3

[Tim Nguyen :ntim]

(In reply to Tim Nguyen [:ntim] from comment #0)
> We can just use border: 1px solid transparent; as a quick fix, so the
> selected row is distinguishable with a border (transparent -> -moz-dialog in
> HC).
Turns out we already do that.

The right solution here is to use the -moz-windows-default-theme @media query. You'll need to set ::-moz-tree-row(selected) to have Highlight as background, and HighlightText as text color (in toolkit/themes/windows/global/in-content/common.css).

Comment 4

[Léon McGregor]

I have the following CSS added to the file:
>@media not all and (-moz-windows-default-theme) {
>  xul|treechildren::-moz-tree-row(selected){
>    background-color: Highlight;
>    color: HighlightText;
>  }
>}
However, for some reason, the treerows just aren't updating, and I can't see why:
* The @query is exactly what other queries use to access the High Contrast mode, and triggers correctly
* The Selector is exactly the same selector used to change the tree selection in normal mode, and it can be changed correctly in normal mode
* The colour names are correct, as they can be used in other color properties

Comment 5

[Tim Nguyen :ntim]

(In reply to Léon McGregor from comment #4)
> I have the following CSS added to the file:
> >@media not all and (-moz-windows-default-theme) {
> >  xul|treechildren::-moz-tree-row(selected){
> >    background-color: Highlight;
> >    color: HighlightText;
> >  }
> >}
> However, for some reason, the treerows just aren't updating, and I can't see
> why:
> * The @query is exactly what other queries use to access the High Contrast
> mode, and triggers correctly
> * The Selector is exactly the same selector used to change the tree
> selection in normal mode, and it can be changed correctly in normal mode
> * The colour names are correct, as they can be used in other color properties

The code is right, so it must be a bug in how we handle high contrast.
Btw, can you also make the rules apply to xul|listbox xul|listitem[selected="true"] ?

Dao, any idea what's happening here, is there any workaround ?

Comment 6

[Dão Gottwald [:dao]]

(In reply to Tim Nguyen [:ntim] from comment #5)
> (In reply to Léon McGregor from comment #4)
> > I have the following CSS added to the file:
> > >@media not all and (-moz-windows-default-theme) {
> > >  xul|treechildren::-moz-tree-row(selected){
> > >    background-color: Highlight;
> > >    color: HighlightText;
> > >  }
> > >}
> > However, for some reason, the treerows just aren't updating, and I can't see
> > why:
> > * The @query is exactly what other queries use to access the High Contrast
> > mode, and triggers correctly
> > * The Selector is exactly the same selector used to change the tree
> > selection in normal mode, and it can be changed correctly in normal mode
> > * The colour names are correct, as they can be used in other color properties
> 
> The code is right, so it must be a bug in how we handle high contrast.

I would guess it gets overridden by some other rule. What does DOM Inspector say?

Comment 7

[Léon McGregor]

The DOM Inspector doesn't say anything, because I can't see it show things selected by 
> ::-moz-tree-row(selected)
Because from what I can see, all of the treechildren are in a single element.

Comment 8

[Léon McGregor]

I've searched through all the css files that are displayed on the inc-ontent page's inspector "Style editor" panel. None seem to be overriding the rule.

Comment 9

[Tim Nguyen :ntim]

(In reply to Léon McGregor from comment #8)
> I've searched through all the css files that are displayed on the inc-ontent
> page's inspector "Style editor" panel. None seem to be overriding the rule.

Yeah, I think it's because of our H-C handling.

Can you just try to add a 2px dotted border to see if it's visible enough ?

Comment 10

[Léon McGregor]

Attachment: Clipboard02.jpg - screenshot of border of selected tree row

This border looks good to me.

Comment 11

[Tim Nguyen :ntim]

(In reply to Léon McGregor from comment #10)
> Created attachment 8576102 [details]
> Clipboard02.jpg - screenshot of border of selected tree row
> 
> This border looks good to me.

Yeah, let's go with that. Can you post a patch ?

Comment 12

[Léon McGregor]

Attachment: bug1115925-inContentTreeRowSelect.diff

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8576111 [details] [diff] [review]
bug1115925-inContentTreeRowSelect.diff

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

::: toolkit/themes/windows/global/in-content/common.css
@@ +92,5 @@
> +
> +@media not all and (-moz-windows-default-theme) {
> +xul|treechildren::-moz-tree-row(selected),
> +xul|listbox xul|listitem[selected="true"]{
> +  border: 2px dotted Highlight;

Please indent the rules inside of the media query and also add a comment saying that the 2px width is necessary to be visible behind another high-contrast border that is the same as the background-color.

Comment 14

[Léon McGregor]

Attachment: bug1115925-inContentTreeRowSelect.diff - ver 2

Comment 15

[Tim Nguyen :ntim]

Comment on attachment 8577466 [details] [diff] [review]
bug1115925-inContentTreeRowSelect.diff - ver 2

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

There needs to be a space between ] and {.

Comment 16

[Léon McGregor]

Attachment: bug1115925-inContentTreeRowSelect.diff - ver 3

Corrected spacing on 
> ]{

Comment 17

[Dão Gottwald [:dao]]

Comment on attachment 8577503 [details] [diff] [review]
bug1115925-inContentTreeRowSelect.diff - ver 3

How does this look with Classic (non-high-contrast)?

Comment 18

[Léon McGregor]

Attachment: all.png - Screenshots of various windows themes with this patch enabled

Attached screenshots.

Everything looks as normal in windows 8, windows 7 and windows 7 basic.

In windows 8 and windows 7 HC there is a dotted border. In windows 8 with custom HC colours it looks as if there is a dotted border. These look fine.

In windows 7 classic (no HC), it looks like the normal windows row highlighting, but there is an additional border.

Comment 19

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8577503 [details] [diff] [review]
bug1115925-inContentTreeRowSelect.diff - ver 3

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

I'm OK with showing the dotted borders on Windows 7 Classic mode. I don't see a different way to accomplish this given our available media queries.

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/c4c897cad784

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c4c897cad784

Comment 22

[:Gijs (he/him)]

Comment on attachment 8577503 [details] [diff] [review]
bug1115925-inContentTreeRowSelect.diff - ver 3

(per discussion with Jared in our meeting)

Approval Request Comment
[Feature/regressing bug #]: n/a
[User impact if declined]: users using windows high contrast mode can't see selected tree items
[Describe test coverage new/current, TreeHerder]: nope
[Risks and why]: very low, simple additional CSS specific to non-default Windows themes (classic, high contrast themes)
[String/UUID change made/needed]: nope

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/dd8d81c94d4d

Comment 24

[Camelia Badau [:cbadau], Desktop Test Engineering]

Verified fixed using latest Aurora 39.0a2 (buildID: 20150427004010) and Firefox 38 Beta 8 (buildID: 20150426174329).