Lorien Hu (:lsocks) Assignee
	Description • 9 years ago

      No description provided.

	alexander :surkov (:asurkov)
	Comment 2 • 9 years ago

Comment on attachment 8612529 [details] [diff] [review]
merged mozPopupButtonAccessible, mozButtonAccessible into one class

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

r=me

::: accessible/mac/AccessibleWrap.mm
@@ -65,5 @@
>      case roles::SPLITBUTTON:
>      case roles::TOGGLE_BUTTON:
>      {
>        // if this button may show a popup, let's make it of the popupbutton type.
> -      return HasPopup() ? [mozPopupButtonAccessible class] : 

AccessibleWrap::HasPopup() is not used anymore, you can remove it

::: accessible/mac/mozActionElements.mm
@@ +58,5 @@
>    NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NIL;
>  
> +  if ([attribute isEqualToString:NSAccessibilityChildrenAttribute]) {
> +    if ([self hasPopup])
> +      return [self children];    

pls remove whitespaces

@@ +87,5 @@
> +  if ([self isEnabled]) {
> +    if ([self hasPopup])
> +      return [NSArray arrayWithObjects:NSAccessibilityPressAction,
> +                                 NSAccessibilityShowMenuAction,
> +                                 nil];

something wrong with indentation, I guess NSAcc.. should be aligned

@@ +102,5 @@
>  
> +  if ([self hasPopup]) {
> +    if ([action isEqualToString:NSAccessibilityShowMenuAction])
> +      return @"show menu";
> +  }

I would move this after pressaction check

@@ +119,5 @@
>  - (void)accessibilityPerformAction:(NSString*)action 
>  {
>    NS_OBJC_BEGIN_TRY_ABORT_BLOCK;
>  
> +  if ([self hasPopup] && ![self isEnabled])

I believe isEnabled check should be applied to mozButtonAcc too

@@ +150,5 @@
> +{
> +  AccessibleWrap* accWrap = [self getGeckoAccessible];
> +  if (!accWrap)
> +    return false;
> +  return (accWrap->NativeState() & mozilla::a11y::states::HASPOPUP);

I would prefer return acc && (acc->NativeState() & )

@@ -188,5 @@
> -                                                  NSAccessibilityHelpAttribute,
> -                                                  NSAccessibilityEnabledAttribute, // required
> -                                                  NSAccessibilityFocusedAttribute, // required
> -                                                  NSAccessibilityTitleAttribute, // required for popupmenus, and for menubuttons with a title
> -                                                  NSAccessibilityChildrenAttribute, // required

this one is missed on mozButtonAccessible

	Lorien Hu (:lsocks) Assignee
	Comment 4 • 9 years ago

Carry r=surkov

	Pulsebot
	Comment 5 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/6fda72452cf9

	Carsten Book [:Tomcat]
	Comment 6 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/6fda72452cf9