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

VERIFIED FIXED in mozilla2.0b11

Status

()

Toolkit
Themes
VERIFIED FIXED
7 years ago
6 years ago

People

(Reporter: Alice0775 White, Assigned: Margaret)

Tracking

Trunk
mozilla2.0b11
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

(Whiteboard: [hardblocker][fx4-fixed-bugday][doorhanger])

Attachments

(2 attachments, 10 obsolete attachments)

(Reporter)

Description

7 years ago
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
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...).
Summary: [password doorhanger]I can not access pulldown menuitem by keyboard, Provide access key → hard to access popup notification pulldown/alternate menu items with the keyboard (no access key)
(Reporter)

Comment 2

7 years ago
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.
Blocks: 594586
blocking2.0: --- → final+
We need a way to focus the popup (see bug 594586), but once that's done everything should already be keyboard accessible.
(Assignee)

Updated

7 years ago
Duplicate of this bug: 611684

Updated

7 years ago
Summary: hard to access popup notification pulldown/alternate menu items with the keyboard (no access key) → hard to access popup (doorhanger) notification pulldown/alternate menu items with the keyboard (no access key)
(Assignee)

Updated

7 years ago
Assignee: nobody → margaret.leibovic
(Assignee)

Comment 5

7 years ago
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.
Component: General → Disability Access
OS: Windows 7 → All
QA Contact: general → disability.access
Hardware: x86 → All
(Assignee)

Comment 6

7 years ago
(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.
(Assignee)

Comment 7

7 years ago
Created attachment 495919 [details] [diff] [review]
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).
Attachment #495919 - Flags: review?(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.
Attachment #495919 - Flags: review?(dao)
(Assignee)

Comment 9

7 years ago
Created attachment 496212 [details] [diff] [review]
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.
Attachment #495919 - Attachment is obsolete: true
Attachment #496212 - Flags: review?(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.
(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?
(Assignee)

Comment 12

7 years ago
(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).
(Assignee)

Comment 13

7 years ago
Created attachment 496528 [details] [diff] [review]
patch v3

Updated to use focusRingShadow.
Attachment #496212 - Attachment is obsolete: true
Attachment #496528 - Flags: review?(dao)
Attachment #496212 - Flags: review?(dao)
(Assignee)

Updated

7 years ago
Whiteboard: [needs review dao]
blocking2.0: final+ → betaN+
(Assignee)

Comment 14

7 years ago
Updating the summary to reflect what this bug is actually fixing. (An access key already exists.)
Summary: hard to access popup (doorhanger) notification pulldown/alternate menu items with the keyboard (no access key) → Create focus styles for split menu buttons in popup (doorhanger) notifications
(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.
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.
Whiteboard: [needs review dao] → [needs review dao][hardblocker]
(Assignee)

Comment 17

7 years ago
(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! :)
Whiteboard: [needs review dao][hardblocker] → [needs review dao][hardblocker][has patch][needs review 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.
Attachment #496528 - Flags: review?(dao) → review-

Updated

7 years ago
Component: Disability Access → Themes
Product: Firefox → Toolkit
QA Contact: disability.access → themes
Target Milestone: Firefox 4.0 → ---
(Assignee)

Comment 19

7 years ago
Created attachment 505563 [details] [diff] [review]
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.
Attachment #496528 - Attachment is obsolete: true
Attachment #505563 - Flags: review?(dao)
(Assignee)

Comment 20

7 years ago
Created attachment 505564 [details]
screenshots
Attachment #505564 - Flags: ui-review?(shorlander)
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?
Attachment #505563 - Flags: review?(dao) → review-
(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.
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.
(Assignee)

Comment 24

7 years ago
Created attachment 505576 [details]
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.
(Assignee)

Comment 25

7 years ago
(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.
(Assignee)

Comment 26

7 years ago
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.
(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.
(Assignee)

Comment 28

7 years ago
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.
Attachment #505563 - Flags: review- → review?(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.

Updated

7 years ago
Attachment #505563 - Flags: review?(dao) → review-
(Assignee)

Comment 30

7 years ago
(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).
(Assignee)

Comment 31

7 years ago
Created attachment 505987 [details] [diff] [review]
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).
Attachment #505987 - Flags: review?(dao)
(Assignee)

Comment 32

7 years ago
Created attachment 505988 [details]
alternate patch screenshot

Stephen, what do you think of this?
Attachment #505988 - Flags: ui-review?(shorlander)
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.
Attachment #505987 - Flags: review?(dao) → review-
(Assignee)

Comment 34

7 years ago
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.
(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.
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 on attachment 505988 [details]
alternate patch screenshot

Looks good after addressing comment 33.
Attachment #505988 - Flags: ui-review?(shorlander) → ui-review+
Attachment #505564 - Flags: ui-review?(shorlander)
(Assignee)

Comment 38

7 years ago
Created attachment 506468 [details] [diff] [review]
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.
Attachment #505563 - Attachment is obsolete: true
Attachment #505564 - Attachment is obsolete: true
Attachment #505576 - Attachment is obsolete: true
Attachment #505987 - Attachment is obsolete: true
Attachment #506468 - Flags: review?(dao)
(Assignee)

Comment 39

7 years ago
Created attachment 506470 [details]
border vs outline

Updated

7 years ago
Whiteboard: [needs review dao][hardblocker][has patch][needs review dao] → [needs review dao][hardblocker][has patch]
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.
(Assignee)

Comment 41

7 years ago
Created attachment 506568 [details] [diff] [review]
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.
Attachment #506468 - Attachment is obsolete: true
Attachment #506470 - Attachment is obsolete: true
Attachment #506568 - Flags: review?(dao)
Attachment #506468 - Flags: review?(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.
Attachment #506568 - Flags: review?(dao) → review+
(Assignee)

Comment 43

7 years ago
Created attachment 506591 [details] [diff] [review]
patch for check-in

Updated pinstripe theme to highlight only the dropmarker.
Attachment #506568 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Keywords: checkin-needed
Whiteboard: [needs review dao][hardblocker][has patch] → [hardblocker][can land]

Comment 44

7 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [hardblocker][can land] → [hardblocker]

Updated

7 years ago
Target Milestone: --- → mozilla2.0b11
Status: RESOLVED → VERIFIED
Whiteboard: [hardblocker] → [hardblocker][fx4-fixed-bugday]
Verified with Mozilla/5.0 (X11; Linux i686; rv:2.0b12pre) Gecko/20110204 Firefox/4.0b12pre
Whiteboard: [hardblocker][fx4-fixed-bugday] → [hardblocker][fx4-fixed-bugday][doorhanger]

Updated

6 years ago
Depends on: 636849
You need to log in before you can comment on or make changes to this bug.