1136645 - InContent prefs - Make focusrings match the spec on Windows and Linux

Status: RESOLVED DUPLICATE of bug 1165973

(Firefox :: Settings UI, defect, P2)

Description

[Tim Nguyen :ntim]

Spec : http://cl.ly/image/1T3a070b3E28
We currently use dotted outlines for focusrings (on Windows and Linux), we need to update our styling to reflect the spec. OSX got the styling in bug 1008171.
This shouldn't block shipping the in-content prefs, as it's just a polish issue.

Comment 1

[:Gijs (he/him)]

We should fix the shifting of the downloads bits in the general pane, though, and I think that's enough of a glaring issue that we should fix it pre-shipping. Might as well get the outlines right then, too.

Comment 3

[Tim Nguyen :ntim]

Gijs, I'm not sure that bug 1130145 needs to be tracked here as well. They don't seem tightly related. Bug 1130145 is a regression (of bug 1115924), while this is just a polish bug. What do you think ?

Also, do you think this or bug 1130145 is a good candidate for a mentored bug ? (I've got instructions at https://pastebin.mozilla.org/8824499 if needed)

Comment 4

[:Gijs (he/him)]

(In reply to Tim Nguyen [:ntim] from comment #3)
> Gijs, I'm not sure that bug 1130145 needs to be tracked here as well. They
> don't seem tightly related. Bug 1130145 is a regression (of bug 1115924),
> while this is just a polish bug. What do you think ?

I expect the focus styles change here might interfere with it; indeed, in your pastebin you refer to the same css block twice (I've not looked more closely than that). I don't think this bug necessarily needs to fix bug 1130145 in one go, but I do think what happens here may affect that bug, and so it makes more sense to fix this bug first.

> Also, do you think this or bug 1130145 is a good candidate for a mentored
> bug ? (I've got instructions at https://pastebin.mozilla.org/8824499 if
> needed)

Not really, until this bug is fixed.

Regarding the instructions, you say:

> you'll need to override the default ones first (should be similar to [2])

which doesn't really make sense to me. I assume the spec is for the default behaviour; I don't see why about:preferences should get different focus rings than other in-content pages. The instructions also miss detail as to where you think the extra styles should go (which I think is moot given my previous point, but anyway) and what it means to "deal with high-contrast". Note that high contrast themes only exist on Windows, and turning off page colors is also possible on OS X, so I don't really understand the "Windows and Linux" thing, either. :-)

Comment 5

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

Tim, do you think you could take this?

Comment 6

[Tim Nguyen :ntim]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #5)
> Tim, do you think you could take this?

Nope, I don't have much time.

(In reply to :Gijs Kruitbosch from comment #4)
> (In reply to Tim Nguyen [:ntim] from comment #3)
> > Gijs, I'm not sure that bug 1130145 needs to be tracked here as well. They
> > don't seem tightly related. Bug 1130145 is a regression (of bug 1115924),
> > while this is just a polish bug. What do you think ?
> 
> I expect the focus styles change here might interfere with it; indeed, in
> your pastebin you refer to the same css block twice (I've not looked more
> closely than that). I don't think this bug necessarily needs to fix bug
> 1130145 in one go, but I do think what happens here may affect that bug, and
> so it makes more sense to fix this bug first.
Not really, bug 1130145 is caused by an extra padding on the label box. While this bug will mainly change the (radio) icon.

> > Also, do you think this or bug 1130145 is a good candidate for a mentored
> > bug ? (I've got instructions at https://pastebin.mozilla.org/8824499 if
> > needed)
> 
> Not really, until this bug is fixed.
The two bugs can be handled separately.

> Regarding the instructions, you say:
> 
> > you'll need to override the default ones first (should be similar to [2])
> 
> which doesn't really make sense to me. I assume the spec is for the default
> behaviour; I don't see why about:preferences should get different focus
> rings than other in-content pages. 
The default styling refers to the dotted outline we already have on Windows and Linux.

> The instructions also miss detail as to
> where you think the extra styles should go (which I think is moot given my
> previous point, but anyway) and what it means to "deal with high-contrast".
> Note that high contrast themes only exist on Windows, and turning off page
> colors is also possible on OS X, so I don't really understand the "Windows
> and Linux" thing, either. :-)
I thought they existed as well on Linux (since the checkboxes and radios have a special treatment on Linux).

Comment 7

[Tim Nguyen :ntim]

Attachment: Patch

Just got a bit of time to tackle this.

This patch works fine on Windows, except when in high contrast mode. In High contrast mode, the outline always seems to be black, even when I set the outline color to Highlight or GrayText. Because of that, the outline is invisible on dark high contrast themes (it's visible on the white high contrast theme though).

I haven't tested this on Linux, but if anyone could, that would be awesome.

Comment 8

[:Gijs (he/him)]

Comment on attachment 8575333 [details] [diff] [review]
Patch

Looks OK on Windows, but tabs now get both focus styles for some reason (ie also a dotted outline).

I'm confused by the dep on bug 1141607. AIUI you're saying GrayText doesn't work. Have you tried -moz-use-text-color ? How does this look on classic themes?

Comment 9

[:Gijs (he/him)]

Err, point about bug 1141607 being, the summary says "outline-color doesn't adapt", and I think you mean "GrayText doesn't adapt" ?

Comment 10

[Tim Nguyen :ntim]

(In reply to :Gijs Kruitbosch from comment #9)
> Err, point about bug 1141607 being, the summary says "outline-color doesn't
> adapt", and I think you mean "GrayText doesn't adapt" ?

All values get changed to black.

Comment 11

[:Gijs (he/him)]

Hey Tim, did you have time to look at this patch again?

Comment 12

[Tim Nguyen :ntim]

(In reply to :Gijs Kruitbosch from comment #11)
> Hey Tim, did you have time to look at this patch again?

Nope :/ my schedule will hopefully free up next week, but feel free to work on it.

Comment 13

[:Gijs (he/him)]

Attachment: Make inContent prefs focusring match the spec on Windows and Linux

This was more frustrating than I thought it was going to be, but this works.

Comment 14

[:Gijs (he/him)]

Tested on OSX & Windows, could do with testing on Linux. Might get to that later today. Hopefully. Leaving needinfo for that.

Comment 15

[:Gijs (he/him)]

On linux this still leaves focus outlines on button-boxes. I haven't figured out why yet. :-\

Comment 16

[:Gijs (he/him)]

Outstanding issues:

- wiggling link outlines (default style, but these are borders because of bug 1141607 and so we need to update margins for individually styled (marginalized :-) ) links so that they don't move when they get/lose focus.

- button box outline on Linux

- tidying up

Comment 17

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

We should get this uplifted to 38 if we can get it uplifted before the next Beta goes out. Otherwise we should probably hold off on it for 38.

Comment 18

[:Gijs (he/him)]

With bug 1141607 fixed, we should be able to use outline instead of border, which will make fixing this easier and the patches simpler (and will make it possible to fix the wiggling of the buttons without oodles of very specific CSS). However, I am not currently working on this, so I'm going to unassign.

Comment 19

[:Gijs (he/him)]

Pretty sure this got fixed in bug 1165973.