796190 - Right border of popup notification menu button becomes dark when focused

Status: RESOLVED FIXED

(Toolkit :: Themes, defect)

Description

[:Margaret Leibovic]

Attachment: screenshot

STR: Make a popup notification appear. Tab to main menu button action to give it focus.

Fixing this bug should just involve changing a CSS style somewhere around here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/pinstripe/global/notification.css#130

Comment 1

[Mario Alvarado [:marioalv]]

Attachment: Patch to fix the styles for the menu button of a popup notification

Hi.
This patch puts back the blue border displayed around the menu button in a pop up notification (Remember password, Activate all plugins, etc.). 
This blue border around the menu button didn't appear and the button element didnt' have focus.  I put back the blue border and gave the button focus, so the user can interact with the button.

Please let me know if something needs to be changed or done differently.

Thanks.

Comment 2

[Mario Alvarado [:marioalv]]

Attachment: Example of the menu button with the blue border re-established

Comment 3

[Mario Alvarado [:marioalv]]

Attachment: Example of the menu button with the blue border re-established

Comment 4

[Mario Alvarado [:marioalv]]

Attachment: Example of the menu button with the blue border re-established

Comment 5

[:Margaret Leibovic]

Comment on attachment 670907 [details] [diff] [review]
Patch to fix the styles for the menu button of a popup notification

Review of attachment 670907 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch! Unfortunately, I don't think this bug description was clear enough. The problem is just that when the main button is focused, we don't want that dark border to show up on the right side of it. We don't need to be changing the rules about what parts of the button get styled when focused (I'm sorry, it's confusing because our focus styles for these menubuttons are a bit of a hack).

::: toolkit/content/PopupNotifications.jsm
@@ +481,5 @@
> +
> +    // Give focus to the notification button
> +    if (this.panel.firstChild.button) {
> +      this.panel.firstChild.button.focus();
> +    }

I don't think we want to give focus to the button when the notification appears. We should get some UX feedback about this. In any case, this is out of the scope of this bug, so this change should go in a different patch.

::: toolkit/themes/pinstripe/global/notification.css
@@ +120,5 @@
>  .popup-notification-menubutton {
>    -moz-appearance: none;
>  }
>  
> +.popup-notification-menubutton:-moz-focusring,

This isn't what we want to do. Instead of changing the selector rule for when the focus style gets applied, we should change the rule that makes the style. Looking in this file more, it looks like this line might be our culprit:
http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/pinstripe/global/notification.css#154

For some reason that is appearing when the button is focused, but not otherwise. We may just need to make that lighter to match whatever new styles are being applied to these buttons (they used to be black, so I suspect this is a workaround style that's out of date now).

@@ +173,5 @@
>    border-top-left-radius: 0;
>    border-bottom-left-radius: 0;
>  }
>  
> +.popup-notification-menubutton:hover:active,

I don't think we need to be changing the :hover:active style.

Comment 6

[Mario Alvarado [:marioalv]]

Hi.
Thank you very much for your review.

At first, I tried to reproduce the bug, but the blue border did not appear in the button of the notification windows (at does not appear as of today after compiling the mozilla-central code).

So, I though I could give focus to the notification button so the blue border appears and then fix the blue. That's why I gave focus to the button and modified the styles.

Right now, my question is: how can I reproduce the bug, given the fact that the blue border does not appear around the button of a notification window?

I took this screenshot from my computer, after making a notification appear, without any changes to the mozilla-central code:
http://imageshack.us/a/img5/4866/screenshot20121016at142.png

Thanks.

Comment 7

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Try it with "full keyboard access" enabled? (You might have to change your system preferences, under keyboard.)

Comment 8

[Mario Alvarado [:marioalv]]

Attachment: Patch to fix the styles for the menu button of a popup notification

Comment 9

[Mario Alvarado [:marioalv]]

Comment on attachment 672106 [details] [diff] [review]
Patch to fix the styles for the menu button of a popup notification

Yep, "full keyboard access" did the trick.

Comment 10

[:Margaret Leibovic]

Comment on attachment 672106 [details] [diff] [review]
Patch to fix the styles for the menu button of a popup notification

Sorry for the slow review! This looks good!

This actually makes me wonder if all of these styles in here are still needed, but that shouldn't stop this fix from landing.

Comment 11

[:Margaret Leibovic]

This issue was caused by http://hg.mozilla.org/mozilla-central/diff/fc19d9b5a531/toolkit/themes/pinstripe/global/notification.css.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/91fc43bf582a

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/91fc43bf582a