553992 - No focus rings for buttons in download manager, places window and the update dialog

Status: RESOLVED FIXED

(Toolkit :: Themes, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: screenshot

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.3a4pre) Gecko/20100321 Minefield/3.7a4pre ID:20100321063324

Since the landing of bug 480178 the buttons at the bottom do not have any focus ring anymore. You cannot see if a button is in a hover or active state. I have no idea how that is exposed to screen readers. I'll cc Marco. Eventually this is only a problem on OS X.

Comment 1

[Robert Strong (they/them - no direct email)]

Henrik, is there the same bug for the Download manager, etc.?

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

As you point that out, yes it is. Seems to be a widget issue? I can even not see the focus ring for Namoroka builds. Looks like it's older.

Comment 3

[Markus Stange [:mstange]]

I was going to wait for bug 418521 before adding focus rings to this button style. It's a theme issue.

Comment 4

[Marco Zehe (:MarcoZ)]

For screen reader users this usually does not have any impact, since they rely on internal structures/states rather than pure visual indications to communicate states like "focused". However, visually impaired and sighted keyboard navigation users will suffer from this.

Comment 6

[Stefan [:stefanh]]

Attachment: Patch v1.0

I haven't yet tested the update dialog and I'll be away for a few days, but this patch should fix buttons in the update dialog, download manager and the places window (all buttons that use the toolbarbutton rules in shared.inc). I use different border color depending on aqua/graphite, but that might be an overkill. The corners where particulary tricky in graphite, so I ended up with just making the borders a bit more transparent.

Comment 7

[Stefan [:stefanh]]

Attachment: Screenshots (Aqua/Graphite)

Comment 8

[Stefan [:stefanh]]

Comment on attachment 544605 [details] [diff] [review]
Patch v1.0

For screenshots, see attachment #545091 [details]. Do I btw need ui-r? If so, would you mind suggesting a name?

Comment 9

[Dão Gottwald [:dao]]

Comment on attachment 544605 [details] [diff] [review]
Patch v1.0

Why aren't you just using focusRingShadow instead of toolbarbuttonFocusRingShadow?

Comment 10

[Stefan [:stefanh]]

(In reply to comment #9)
> Comment on attachment 544605 [details] [diff] [review] [review]
> Patch v1.0
> 
> Why aren't you just using focusRingShadow instead of
> toolbarbuttonFocusRingShadow?

I don't think focusRingShadow matches the look we want. I belive my toolbarbuttonFocusRingShadow matches the toolbar focus rings in the OS (e.g "Show All" in system preferences) much better.

Comment 11

[Dão Gottwald [:dao]]

> I don't think focusRingShadow matches the look we want. I belive my
> toolbarbuttonFocusRingShadow matches the toolbar focus rings in the OS (e.g
> "Show All" in system preferences) much better.

Could you provide screenshots that show this difference? I don't have a Mac to test this.

Comment 12

[Nomis101]

Will this Bug also fix Bug 587813 or is this different?

Comment 13

[Stefan [:stefanh]]

(In reply to comment #12)
> Will this Bug also fix Bug 587813 or is this different?

That looks like a different issue, more like some kind of re-paint problem.

Comment 14

[Stefan [:stefanh]]

Attachment: Screenshots

(In reply to comment #11)
> > I don't think focusRingShadow matches the look we want. I belive my
> > toolbarbuttonFocusRingShadow matches the toolbar focus rings in the OS (e.g
> > "Show All" in system preferences) much better.
> 
> Could you provide screenshots that show this difference? I don't have a Mac
> to test this.

From left to right: toolbarbuttonFocusRingShadow - native - focusRingShadow.

The difference is not huge. But, especially for the places buttons, the focusRingShadow is a bit thicker and sligtly more "fluffy" (zoom in and it becomes even more visible).

Comment 15

[Dão Gottwald [:dao]]

In fact, the difference is very minor. Given this and since apparently this is a temporary solution until we get bug 672050, I'd prefer if you just reused focusedRingShadow.

Comment 16

[Stefan [:stefanh]]

Attachment: V1.1

(In reply to comment #15)
> In fact, the difference is very minor. Given this and since apparently this
> is a temporary solution until we get bug 672050, I'd prefer if you just
> reused focusedRingShadow.

Sure.

Comment 17

[Dão Gottwald [:dao]]

Comment on attachment 548260 [details] [diff] [review]
V1.1

>--- a/browser/themes/pinstripe/browser/places/organizer.css
>+++ b/browser/themes/pinstripe/browser/places/organizer.css

>+#placesToolbar:-moz-system-metric(mac-graphite-theme) > toolbarbutton:-moz-focusring {
>+  border-color: @toolbarbuttonFocusedBorderColorGraphite@;
>+}

Please move :-moz-system-metric(mac-graphite-theme) to the toolbarbutton.

>--- a/toolkit/themes/pinstripe/mozapps/update/updates.css
>+++ b/toolkit/themes/pinstripe/mozapps/update/updates.css

>+.wizard-buttons:-moz-system-metric(mac-graphite-theme) button:-moz-focusring {
>+  border-color: @toolbarbuttonFocusedBorderColorGraphite@;
>+}

And to the button here.

Comment 18

[Stefan [:stefanh]]

Pushed to inbound with comment #17 addressed:
http://hg.mozilla.org/integration/mozilla-inbound/rev/a7da303efe28

Comment 19

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://hg.mozilla.org/mozilla-central/rev/a7da303efe28