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

VERIFIED FIXED in Firefox 38

Status

()

Firefox
Preferences
P3
normal
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: ntim, Assigned: Léon McGregor, Mentored)

Tracking

unspecified
Firefox 39
Points:
2

Firefox Tracking Flags

(firefox38 verified, firefox39 verified)

Details

(Whiteboard: [lang=css])

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

3 years ago
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.
Component: Developer Tools → Preferences
Priority: -- → P3
Points: --- → 2
Tim, could you take this bug as well?
Flags: needinfo?(ntim007)
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.
Assignee: nobody → lemcgregor3
Mentor: ntim007@yahoo.it
Status: NEW → ASSIGNED
Flags: needinfo?(ntim007)
(Reporter)

Comment 3

3 years ago
(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).
(Assignee)

Comment 4

3 years ago
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
Flags: needinfo?(ntim007)
(Assignee)

Updated

3 years ago
Whiteboard: [lang=css]
(Reporter)

Comment 5

3 years ago
(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 ?
Flags: needinfo?(ntim007)
(Reporter)

Updated

3 years ago
Flags: needinfo?(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?
Flags: needinfo?(dao)
(Assignee)

Comment 7

3 years ago
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.
(Assignee)

Comment 8

3 years ago
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.
(Reporter)

Comment 9

3 years ago
(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 ?
(Assignee)

Comment 10

3 years ago
Created attachment 8576102 [details]
Clipboard02.jpg - screenshot of border of selected tree row

This border looks good to me.
(Reporter)

Comment 11

3 years ago
(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 ?
(Assignee)

Comment 12

3 years ago
Created attachment 8576111 [details] [diff] [review]
bug1115925-inContentTreeRowSelect.diff
Attachment #8576111 - Flags: review?(jaws)
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.
Attachment #8576111 - Flags: review?(jaws) → feedback+
(Assignee)

Comment 14

3 years ago
Created attachment 8577466 [details] [diff] [review]
bug1115925-inContentTreeRowSelect.diff - ver 2
Attachment #8576111 - Attachment is obsolete: true
Attachment #8577466 - Flags: review?(jaws)
(Reporter)

Comment 15

3 years ago
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 {.
(Assignee)

Updated

3 years ago
Attachment #8577466 - Flags: review?(jaws)
(Assignee)

Comment 16

3 years ago
Created attachment 8577503 [details] [diff] [review]
bug1115925-inContentTreeRowSelect.diff - ver 3

Corrected spacing on 
> ]{
Attachment #8577466 - Attachment is obsolete: true
Attachment #8577503 - Flags: review?(jaws)
Comment on attachment 8577503 [details] [diff] [review]
bug1115925-inContentTreeRowSelect.diff - ver 3

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

Comment 18

3 years ago
Created attachment 8577601 [details]
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 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.
Attachment #8577503 - Flags: review?(jaws) → review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/c4c897cad784
Keywords: checkin-needed
Whiteboard: [lang=css] → [lang=css][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/c4c897cad784
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Whiteboard: [lang=css][fixed-in-fx-team] → [lang=css]
Target Milestone: --- → Firefox 39

Comment 22

3 years ago
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
Attachment #8577503 - Flags: approval-mozilla-aurora?
Attachment #8577503 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
status-firefox38: --- → affected
Verified fixed using latest Aurora 39.0a2 (buildID: 20150427004010) and Firefox 38 Beta 8 (buildID: 20150426174329).
Status: RESOLVED → VERIFIED
status-firefox38: fixed → verified
status-firefox39: fixed → verified
You need to log in before you can comment on or make changes to this bug.