Closed
Bug 1136645
Opened 10 years ago
Closed 9 years ago
InContent prefs - Make focusrings match the spec on Windows and Linux
Categories
(Firefox :: Settings UI, defect, P2)
Firefox
Settings UI
Tracking
()
RESOLVED
DUPLICATE
of bug 1165973
Iteration:
41.1 - May 25
People
(Reporter: ntim, Unassigned)
References
(Blocks 2 open bugs)
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
jaws
:
review+
Gijs
:
review-
|
Details | Diff | Splinter Review |
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•10 years ago
|
||
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.
Blocks: ship-incontent-prefs
Updated•10 years ago
|
Points: --- → 3
Priority: -- → P2
Reporter | ||
Comment 3•10 years ago
|
||
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)
Comment 4•10 years ago
|
||
(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)
Reporter | ||
Comment 6•10 years ago
|
||
(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)
Reporter | ||
Comment 7•10 years ago
|
||
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)
Comment 8•10 years ago
|
||
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+
Comment 9•10 years ago
|
||
Err, point about bug 1141607 being, the summary says "outline-color doesn't adapt", and I think you mean "GrayText doesn't adapt" ?
Reporter | ||
Comment 10•10 years ago
|
||
(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•10 years ago
|
||
Hey Tim, did you have time to look at this patch again?
Flags: needinfo?(ntim.bugs)
Reporter | ||
Comment 12•10 years ago
|
||
(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)
Updated•10 years ago
|
Flags: needinfo?(gijskruitbosch+bugs)
Comment 13•10 years ago
|
||
This was more frustrating than I thought it was going to be, but this works.
Attachment #8594916 -
Flags: review?(jaws)
Updated•10 years ago
|
Attachment #8575333 -
Attachment is obsolete: true
Updated•10 years ago
|
Assignee: ntim.bugs → gijskruitbosch+bugs
Comment 14•10 years ago
|
||
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)
Updated•10 years ago
|
Attachment #8594916 -
Flags: review?(jaws) → review+
Comment 15•10 years ago
|
||
On linux this still leaves focus outlines on button-boxes. I haven't figured out why yet. :-\
Comment 16•10 years ago
|
||
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•10 years ago
|
||
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.
Updated•10 years ago
|
Iteration: --- → 40.3 - 11 May
status-firefox38:
--- → wontfix
status-firefox39:
--- → affected
status-firefox40:
--- → affected
Flags: qe-verify+
Flags: in-testsuite-
Flags: firefox-backlog+
Updated•10 years ago
|
Attachment #8594916 -
Flags: review-
Updated•10 years ago
|
Iteration: 40.3 - 11 May → 41.1 - May 25
Comment 18•9 years ago
|
||
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
Updated•9 years ago
|
Comment 19•9 years ago
|
||
Pretty sure this got fixed in bug 1165973.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → DUPLICATE
Updated•9 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•