Closed Bug 1136645 Opened 7 years ago Closed 6 years ago

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

Categories

(Firefox :: Preferences, defect, P2)

defect
Points:
3

Tracking

()

RESOLVED DUPLICATE of bug 1165973
Iteration:
41.1 - May 25
Tracking Status
firefox38 --- wontfix
firefox39 --- affected
firefox40 --- affected

People

(Reporter: ntim, Unassigned)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file, 1 obsolete file)

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.
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.
Points: --- → 3
Priority: -- → P2
Duplicate of this bug: 1103445
Blocks: 1130145
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)
Flags: needinfo?(gijskruitbosch+bugs)
(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. :-)
Flags: needinfo?(gijskruitbosch+bugs)
Tim, do you think you could take this?
Flags: needinfo?(ntim007)
(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).
Flags: needinfo?(ntim007)
Attached patch Patch (obsolete) — Splinter Review
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.
Assignee: nobody → ntim007
Status: NEW → ASSIGNED
Attachment #8575333 - Flags: feedback?(jaws)
Attachment #8575333 - Flags: feedback?(gijskruitbosch+bugs)
Depends on: 1141607
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?
Attachment #8575333 - Flags: feedback?(jaws)
Attachment #8575333 - Flags: feedback?(gijskruitbosch+bugs)
Attachment #8575333 - Flags: feedback+
Err, point about bug 1141607 being, the summary says "outline-color doesn't adapt", and I think you mean "GrayText doesn't adapt" ?
(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.
Hey Tim, did you have time to look at this patch again?
Flags: needinfo?(ntim.bugs)
(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.
Flags: needinfo?(ntim.bugs)
Flags: needinfo?(gijskruitbosch+bugs)
This was more frustrating than I thought it was going to be, but this works.
Attachment #8594916 - Flags: review?(jaws)
Attachment #8575333 - Attachment is obsolete: true
Assignee: ntim.bugs → gijskruitbosch+bugs
Tested on OSX & Windows, could do with testing on Linux. Might get to that later today. Hopefully. Leaving needinfo for that.
Flags: needinfo?(gijskruitbosch+bugs)
Attachment #8594916 - Flags: review?(jaws) → review+
On linux this still leaves focus outlines on button-boxes. I haven't figured out why yet. :-\
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
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.
Iteration: --- → 40.3 - 11 May
Flags: qe-verify+
Flags: in-testsuite-
Flags: firefox-backlog+
Attachment #8594916 - Flags: review-
Iteration: 40.3 - 11 May → 41.1 - May 25
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.
Assignee: gijskruitbosch+bugs → nobody
Status: ASSIGNED → NEW
See Also: → 1243353
Pretty sure this got fixed in bug 1165973.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1165973
See Also: 1243353
You need to log in before you can comment on or make changes to this bug.