Closed Bug 796190 Opened 8 years ago Closed 8 years ago

Right border of popup notification menu button becomes dark when focused

Categories

(Toolkit :: Themes, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: Margaret, Assigned: marioalv)

References

Details

(Whiteboard: [good first bug][mentor=margaret][lang=css])

Attachments

(5 files, 1 obsolete file)

Attached image 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
Component: Theme → Themes
Product: Firefox → Toolkit
Assignee: nobody → marioalv.mozilla
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.
Attachment #670907 - Flags: review?(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.
Attachment #670907 - Flags: review?(margaret.leibovic) → review-
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.
Try it with "full keyboard access" enabled? (You might have to change your system preferences, under keyboard.)
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.
Attachment #672106 - Flags: review? → review?(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.
Attachment #672106 - Flags: review?(margaret.leibovic) → review+
Attachment #670907 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/91fc43bf582a
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in before you can comment on or make changes to this bug.