595271 - Create focus styles for split menu buttons in popup (doorhanger) notifications

Status: VERIFIED FIXED

(Toolkit :: Themes, defect)

Description

[Alice0775 White]

Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b6pre)
Gecko/20100910 Firefox/4.0b6pre ID:20100910041829

The new password doorhanger notification's pulldown menuitem is not able to access by keyboard.

Should provide access key.

Steps to Reproduce:
1. Log in on a any website (newly password).
2. Try to access pulldown menuitem by keyboard

Actual Results:  
 I can not access submenu by keyboard

Expected Results:  
 I can access submenu by keyboard

Comment 1

[:Gavin Sharp [email: [e-mail]]]

You can tab to it and use alt+down, can't you? I tested this when I was writing the popupnotification system (though maybe only on Mac...).

Comment 2

[Alice0775 White]

I can not.
login page of https://bugzilla.mozilla.org/
after popup notification, press Tab key do not focus the button.
alt+down  focus menubar.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

We need a way to focus the popup (see bug 594586), but once that's done everything should already be keyboard accessible.

Comment 5

[:Margaret Leibovic]

Testing on Windows, I can tab to the button's main action when the popup has focus, but I can't tab to the dropdown button. Is there a special key for opening the dropdown menu?

On OSX, I can't tab to the button at all, so I don't think bug 594586 will completely fix this bug.

Comment 6

[:Margaret Leibovic]

(In reply to comment #5)
> On OSX, I can't tab to the button at all, so I don't think bug 594586 will
> completely fix this bug.

I just realized that this was because I had full keyboard access disabled. With it enabled, I can tab to the button, and I can open the menupopup with the down arrow. So it looks like Windows is the platform that needs investigating.

Comment 7

[:Margaret Leibovic]

Attachment: patch

I discovered that the problem is that there are no focus styles for our custom split menu buttons, so you can't tell that you tabbed to the different parts of the button. There isn't a problem with the keyboard accessibility.

Dao, Stephen gave me these focus style colors for the different XP themes we support. If this seems out of scope for this bug, I can push the custom colors to a follow-up, and we can just use system colors for now (although Stephen said that there aren't currently system colors that provide the color we want).

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 495919 [details] [diff] [review]
patch

Yeah, please move it to a separate bug. The popup styling doesn't even target these other flavors in the first place.

Comment 9

[:Margaret Leibovic]

Attachment: patch v2

Because of bug 612181, I had to set focus border styles on .button-menubutton-button and .button-menubutton-dropmarker instead of .popup-notification-menubutton in winstripe.

On pinstripe, I had to set position: relative; on .button-menubutton-button so that the right side of the focus ring would appear on top of the dropmarker element (there was a similar issue in bug 597557).

The custom button styles are applied to the different luna color themes, but Highlight looks like it does a pretty good job on XP, so I don't know if we really need a follow-up bug to set the colors ourselves (and avoiding all those hard-coded colors would be a good idea). However, I'll leave it up to Stephen to decide if a follow-up is necessary.

Comment 10

[Dão Gottwald [:dao]]

Comment on attachment 496212 [details] [diff] [review]
patch v2

>+.popup-notification-menubutton:-moz-focusring,
>+.popup-notification-menubutton > .button-menubutton-button:-moz-focusring {
>+  box-shadow: 0 0 1px -moz-mac-focusring inset,
>+              0 0 4px 1px -moz-mac-focusring,
>+              0 0 2px 1px -moz-mac-focusring;
>+}

Use shared.inc's focusRingShadow here.

Comment 11

[Dão Gottwald [:dao]]

(In reply to comment #9)
> Because of bug 612181, I had to set focus border styles on
> .button-menubutton-button and .button-menubutton-dropmarker instead of
> .popup-notification-menubutton in winstripe.

Why would this be different for pinstripe?

Comment 12

[:Margaret Leibovic]

(In reply to comment #11)
> (In reply to comment #9)
> > Because of bug 612181, I had to set focus border styles on
> > .button-menubutton-button and .button-menubutton-dropmarker instead of
> > .popup-notification-menubutton in winstripe.
> 
> Why would this be different for pinstripe?

We're not setting a different focus border color for pinstripe (as per following the hud button styles), so we don't need to worry about overriding the border style that makes the vertical line between the two halves of the button (the box-shadow style rules are the same in both themes).

Comment 13

[:Margaret Leibovic]

Attachment: patch v3

Updated to use focusRingShadow.

Comment 14

[:Margaret Leibovic]

Updating the summary to reflect what this bug is actually fixing. (An access key already exists.)

Comment 15

[Dão Gottwald [:dao]]

(In reply to comment #7)
> I discovered that the problem is that there are no focus styles for our custom
> split menu buttons, so you can't tell that you tabbed to the different parts of
> the button. There isn't a problem with the keyboard accessibility.

I'm unable to open the button options on Windows as well as Linux.

Comment 16

[:Gavin Sharp [email: [e-mail]]]

Alt+Down with the dropdown button focused worked for me when I tried (can't test at the moment). The lack of focus styling makes it hard to tell when it's focused, though.

Comment 17

[:Margaret Leibovic]

(In reply to comment #13)
> Created attachment 496528 [details] [diff] [review]
> patch v3
> 
> Updated to use focusRingShadow.

Is there anything I can do to clarify this patch to help with review? I just want to try to clear this hard blocker! :)

Comment 18

[Dão Gottwald [:dao]]

Comment on attachment 496528 [details] [diff] [review]
patch v3

You're using :-moz-focusring in pinstripe but :focus in winstripe. Since this is about keyboard focus rather than mouse focus, you should be using :-moz-focusring in winstripe. Also, keyboard focus would better be indicated by a dotted line rather than the inset shadow.

Comment 19

[:Margaret Leibovic]

Attachment: patch v4

I came up with this after discussion with Stephen. I'll post a screenshot for him to ui-review.

The native dotted border style was already being applied to .button-menubutton-menu, but I had to move the padding to its .button-box to make it look right. Stephen and I also decided it was appropriate to add the dotted border to the dropmarker element when the main button is focused, since that's the element that does something in that situation.

Comment 20

[:Margaret Leibovic]

Attachment: screenshots

Comment 21

[Dão Gottwald [:dao]]

Comment on attachment 505563 [details] [diff] [review]
patch v4

>   .popup-notification-menubutton > .button-menubutton-dropmarker {
>-    padding: 9px 5px 8px;
>     width: auto;
>     height: auto;
>     list-style-image: url("chrome://global/skin/arrow/arrow-dn-sharp.gif");
>+    -moz-border-start: none;
>+  }
>+
>+  .popup-notification-menubutton > .button-menubutton-dropmarker > .dropmarker-icon {
>+    border: 1px dotted transparent;
>+    padding: 8px 3px 7px;
>+  }
>+
>+  .popup-notification-menubutton:-moz-focusring > .button-menubutton-dropmarker > .dropmarker-icon {
>+    border-color: ThreeDDarkShadow;
>   }

Can you make this an 'outline' on .button-menubutton-dropmarker rather than a 'border' on .dropmarker-icon? outline-offset should be negative and you shouldn't have to specify a color (so that it automatically uses the text color). Something like outline: 1px dotted; outline-offset: -3px;

>-  .popup-notification-menubutton > .button-menubutton-button:hover,
>-  .popup-notification-menubutton > .button-menubutton-dropmarker:hover {
>+  .popup-notification-menubutton:-moz-focusring > .button-menubutton-button,
>+  .popup-notification-menubutton:-moz-focusring > .button-menubutton-dropmarker,
>+  .popup-notification-menubutton > .button-menubutton-button:-moz-focusring {
>+%ifdef WINSTRIPE_AERO
>+    border-color: rgb(60,127,177);
>+    box-shadow: 0 0 3px rgb(0,178,222) inset;
>+%else
>+    border-color: Highlight;
>+    box-shadow: 0 0 3px Highlight inset;
>+%endif
>+  }

I'm not sure why you're adding this. The dotted focus ring fixes this bug just fine, doesn't it?

Comment 22

[Stephen Horlander [:shorlander]]

(In reply to comment #21)
> I'm not sure why you're adding this. The dotted focus ring fixes this bug just
> fine, doesn't it?

Highlighting the border is standard for Windows focus unless you are using Classic.

Comment 23

[Dão Gottwald [:dao]]

The XP screenshots at least don't strike me as native. The highlight for the whole button when the dropmarker is focused also seems not really helpful.

Comment 24

[:Margaret Leibovic]

Attachment: screenshot using outline

Using outline is nicer, but it's 1px off on the left because .button-menubutton-dropmarker has no left border. It doesn't look like we can specify a different outline-offest for different sides, so I'm not sure if there's a way to fix this. 

Also, I think we want to use ThreeDDarkShadow for the dotted border, since that's what the native buttons use for it.

Comment 25

[:Margaret Leibovic]

(In reply to comment #23)
> The XP screenshots at least don't strike me as native. The highlight for the
> whole button when the dropmarker is focused also seems not really helpful.

Looking more at the native buttons, I agree that for classic the blue highlighting doesn't fit in, but the native buttons in luna do have a blue focus style, so I think those look appropriate.

Focus for these menu buttons is kind of weird because tabbing to them will focus the whole button, so we apply a highlight style for the whole button, but you can only act on the dropmarker. Then a second tab will focus the main action sub-button. Gavin tried looking into an approach that would allow us to just focus the whole button, then have enter trigger the main action and alt+down trigger the dropdown, but it seemed too tricky to try implementing for Firefox 4.

Comment 26

[:Margaret Leibovic]

Also, maybe we should just cope with the non-native looking feel for some XP themes, since that will eventually be addressed by bug 509642.

Comment 27

[Stephen Horlander [:shorlander]]

(In reply to comment #26)
> Also, maybe we should just cope with the non-native looking feel for some XP
> themes, since that will eventually be addressed by bug 509642.

I would be ok with that. However I don't think we lose anything by having the highlight even if it is not necessarily native styling. It helps visually identify focused items since the dotted line is fairly subtle.

Comment 28

[:Margaret Leibovic]

Comment on attachment 505563 [details] [diff] [review]
patch v4

Re-requesting review, since Stephen approves of the highlight styles, and using outline instead of border creates an unwanted 1px space to the left of the dotted line.

Comment 29

[Dão Gottwald [:dao]]

(In reply to comment #24)
> Using outline is nicer, but it's 1px off on the left because
> .button-menubutton-dropmarker has no left border.

It had a left border before your patch removed it. Can you undo this?

(In reply to comment #25)
> (In reply to comment #23)
> > The XP screenshots at least don't strike me as native. The highlight for the
> > whole button when the dropmarker is focused also seems not really helpful.
> 
> Looking more at the native buttons, I agree that for classic the blue
> highlighting doesn't fit in, but the native buttons in luna do have a blue
> focus style, so I think those look appropriate.

The blue shadow you're adding looks quite different from native buttons. (Actually the whole thing doesn't look like a Luna button.)

> Focus for these menu buttons is kind of weird because tabbing to them will
> focus the whole button, so we apply a highlight style for the whole button, but
> you can only act on the dropmarker.

Right, so your styling highlights and unintentional implementation detail. Seems like we should avoid that.

Comment 30

[:Margaret Leibovic]

(In reply to comment #29)
> (In reply to comment #24)
> > Using outline is nicer, but it's 1px off on the left because
> > .button-menubutton-dropmarker has no left border.
> 
> It had a left border before your patch removed it. Can you undo this?

I exchanged the -moz-border-end: none; on .button-menubutton-button for -moz-border-start: none; on the .button-menubutton-dropmarker, so that when the button is focused, it has a border. The button does not look different, it's just that the vertical line is drawn on the button element instead of the dropmarker element (this is actually the way the button styles are implemented in pinstripe).

Comment 31

[:Margaret Leibovic]

Attachment: alternate patch

This patch just applies an dotted outline when the button is focused. I still had to tweak .button-menubutton-button so that the outline would be drawn properly on it (including switching that border style to apply to the button element instead of the dropdown element).

Comment 32

[:Margaret Leibovic]

Attachment: alternate patch screenshot

Stephen, what do you think of this?

Comment 33

[Dão Gottwald [:dao]]

Comment on attachment 505987 [details] [diff] [review]
alternate patch

>+  .popup-notification-menubutton:-moz-focusring {
>+    outline: 1px dotted ThreeDDarkShadow;
>+    outline-offset: -3px;
>+  }

I don't understand why you're drawing the dotted line on the whole button rather than just the dropmarker.

Comment 34

[:Margaret Leibovic]

I must have misunderstood you in comment 29:

> Right, so your styling highlights and unintentional implementation detail.
> Seems like we should avoid that.

I took this to mean you didn't want us to highlight the dropmarker. I can change my patch to do that, although I feel like it will look kind of strange to tab to the button and have an outline on the dropmarker, then tab again and have an outline on the main action button.

Comment 35

[Stephen Horlander [:shorlander]]

(In reply to comment #34)
> I must have misunderstood you in comment 29:
> 
> > Right, so your styling highlights and unintentional implementation detail.
> > Seems like we should avoid that.
> 
> I took this to mean you didn't want us to highlight the dropmarker. I can
> change my patch to do that, although I feel like it will look kind of strange
> to tab to the button and have an outline on the dropmarker, then tab again and
> have an outline on the main action button.

It it is a little odd, but I think outlining the whole button gives the wrong impression. You can't tell which side of the button is focused.

Comment 36

[Dão Gottwald [:dao]]

The "unintentional implementation detail" that you're highlighting is that the whole button is focused when you're really going to activate the dropdown. The user just needs to know that he's going to open the dropdown, not which element exactly is focused. The tab order is another unfortunate implementation detail, but hiding that would go against this bug's purpose, as Stephen just said, and should be addressed in a separate bug by changing the tab order.

Comment 37

[Stephen Horlander [:shorlander]]

Comment on attachment 505988 [details]
alternate patch screenshot

Looks good after addressing comment 33.

Comment 38

[:Margaret Leibovic]

Attachment: patch v6

Here's a patch that just adds the dotted line to the dropmarker. I had to use a border style instead of an outline style to avoid the issue from comment 24. I'll attach a screenshot of the difference between using a border on .dropmarker-icon and an outline on .button-menubutton-dropmarker.

Comment 39

[:Margaret Leibovic]

Attachment: border vs outline

Comment 40

[Dão Gottwald [:dao]]

Why exactly are you moving the border again? I don't seem to understand comment 30. If it's just consistency with pinstripe, that's not a goal.

Comment 41

[:Margaret Leibovic]

Attachment: patch v7

Ah, the border switch was necessary for when I was including the border/box-shadow highlight styles, but it's not necessary anymore. Even better, by not switching it, using outline on the dropmarker does the right thing now.

Comment 42

[Dão Gottwald [:dao]]

Comment on attachment 506568 [details] [diff] [review]
patch v7

I'd prefer pinstripe to highlight only the dropmarker rather than the whole button. Same reasons as for winstripe apply, except that the outer shadow looks less unexpected across the whole button.

Comment 43

[:Margaret Leibovic]

Attachment: patch for check-in

Updated pinstripe theme to highlight only the dropmarker.

Comment 44

[Frank Yan (:fryn)]

Pushed.

http://hg.mozilla.org/mozilla-central/rev/9d9dbd3c1a6d

While there is "Firefox 4.0b11" under Target Milestone, there doesn't seem to be a corresponding "mozilla2.0b11", so I'll leave that blank for now.

Comment 45

[Aleksei Perepelkin [:chaos]]

Verified with Mozilla/5.0 (X11; Linux i686; rv:2.0b12pre) Gecko/20110204 Firefox/4.0b12pre