726072 - nsAccUtils::GetMultiSelectableContainer should use nsDocAccessible::GetAccessible

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

For that nsAccUtils::GetMultiSelectableContainer should take nsAccessible* instead of nsIContent*

http://mxr.mozilla.org/mozilla-central/ident?i=GetMultiSelectableContainer&tree=mozilla-central&filter=

Comment 1

[Jignesh Kakadiya [:jhk]]

Attachment: Patch

Comment 2

[alexander :surkov (:asurkov)]

Comment on attachment 604775 [details] [diff] [review]
Patch

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

::: accessible/src/base/nsAccessible.cpp
@@ +1085,5 @@
>    if (IsDefunct())
>      return NS_ERROR_FAILURE;
>  
>    if (State() & states::SELECTABLE) {
> +    nsDocAccessible* mDoc = Document();

Document() returns mDoc :)

@@ +1086,5 @@
>      return NS_ERROR_FAILURE;
>  
>    if (State() & states::SELECTABLE) {
> +    nsDocAccessible* mDoc = Document();
> +    nsAccessible* multiSelect;

you should initialize it by nsnull

@@ +1088,5 @@
>    if (State() & states::SELECTABLE) {
> +    nsDocAccessible* mDoc = Document();
> +    nsAccessible* multiSelect;
> +    if(mDoc) {
> +      nsAccessible* acc = mDoc->GetAccessible(mContent);

except the document accessible object and non primary node accessible cases it returns *this*. I don't see anything bad to try a document accessible (for example when document is an item of selectable container, sounds weird but valid). And we shouldn't get anything bad for non primary node accessible (GetMultiSelectableContainer returns null in this case)

@@ +1122,5 @@
> +    nsAccessible* multiSelect;
> +    if(mDoc) {
> +      nsAccessible* acc = mDoc->GetAccessible(mContent);
> +      multiSelect = nsAccUtils::GetMultiSelectableContainer(acc);
> +    }

same here

Comment 3

[Jignesh Kakadiya [:jhk]]

Attachment: Patch

Comment 4

[alexander :surkov (:asurkov)]

Comment on attachment 604936 [details] [diff] [review]
Patch

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

::: accessible/src/base/nsAccUtils.cpp
@@ +391,2 @@
>  {
> +  if (aAccessible) {

no need null check, you always pass *this*

::: accessible/src/base/nsAccessible.cpp
@@ +1085,5 @@
>    if (IsDefunct())
>      return NS_ERROR_FAILURE;
>  
>    if (State() & states::SELECTABLE) {
> +    nsAccessible* multiSelect = nsAccUtils::GetMultiSelectableContainer(this);

it makes sense to make GetMultiSelectableContainer to take State() as an argument
also I wonder if we need GetMultiSelectableContainer at all, we could do

nsAccessible* multiSelect = nsAccUtils::GetSelectableContainer();
if (multiSelect && multiSelect->State() & states::MULTISLECTABLE) {
  // bla
}

Comment 5

[Jignesh Kakadiya [:jhk]]

Attachment: Patch

Comment 6

[alexander :surkov (:asurkov)]

Comment on attachment 605722 [details] [diff] [review]
Patch

I'll do feedback, let's Eitan for review

Comment 7

[Eitan Isaacson [:eeejay]]

Comment on attachment 605722 [details] [diff] [review]
Patch

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

Looks good. Just one nit below.

::: accessible/src/base/nsAccessible.cpp
@@ +1089,5 @@
> +    nsAccUtils::GetSelectableContainer(this, State());
> +  if (!multiSelect) {
> +    return aSelect ? TakeFocus() : NS_ERROR_FAILURE;
> +  }
> +  if (multiSelect && multiSelect->State() & states::MULTISELECTABLE) {

Don't need to null-check multiSelect again.

Comment 8

[Jignesh Kakadiya [:jhk]]

Attachment: patch

Nit fixed.

Comment 9

[alexander :surkov (:asurkov)]

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

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

::: accessible/src/base/nsAccessible.cpp
@@ +1085,5 @@
>    if (IsDefunct())
>      return NS_ERROR_FAILURE;
>  
> +  nsAccessible* multiSelect =
> +    nsAccUtils::GetSelectableContainer(this, State());

you should name it as 'select'

@@ +1089,5 @@
> +    nsAccUtils::GetSelectableContainer(this, State());
> +  if (!multiSelect) {
> +    return aSelect ? TakeFocus() : NS_ERROR_FAILURE;
> +  }
> +  if (multiSelect->State() & states::MULTISELECTABLE) {

wrong:
1) you should takeFocus if select is not mutliselectable
2) I'd say you shouldn't takeFocus when there's no selectable container (I think selectable state check prevented this case).

Comment 10

[Jignesh Kakadiya [:jhk]]

Attachment: patch

Comment 11

[alexander :surkov (:asurkov)]

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

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

::: accessible/src/base/nsAccessible.cpp
@@ +1096,5 @@
> +                                   NS_LITERAL_STRING("true"), true);
> +        }
> +        return mContent->UnsetAttr(kNameSpaceID_None,
> +                                   nsGkAtoms::aria_selected, true);
> +      }

I'd prefer early return here

@@ +1120,1 @@
>      return SetSelected(true);

not equivalent, SetSelected was called when select is not multiselectable

Comment 12

[Jignesh Kakadiya [:jhk]]

Attachment: patch

Comment 13

[alexander :surkov (:asurkov)]

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

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

::: accessible/src/base/nsAccessible.cpp
@@ +1097,4 @@
>          return mContent->SetAttr(kNameSpaceID_None,
>                                   nsGkAtoms::aria_selected,
>                                   NS_LITERAL_STRING("true"), true);
>        }

it seems you misread me, I meant to return early to get rid else statement for non multiselectable item

Comment 14

[Jignesh Kakadiya [:jhk]]

Attachment: patch

Comment 15

[alexander :surkov (:asurkov)]

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

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

::: accessible/src/base/nsAccessible.cpp
@@ +1104,1 @@
>        return aSelect ? TakeFocus() : NS_ERROR_FAILURE;

I wonder why do you need else after return?

@@ +1115,5 @@
>      return NS_ERROR_FAILURE;
>  
> +  nsAccessible* select =
> +    nsAccUtils::GetSelectableContainer(this, State());
> +  if (select ) {

space after select

Comment 16

[Jignesh Kakadiya [:jhk]]

Attachment: Patch

Comment 17

[alexander :surkov (:asurkov)]

Comment on attachment 606295 [details] [diff] [review]
Patch

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

f=me, thanks

::: accessible/src/base/nsAccessible.cpp
@@ +1080,2 @@
>  
>  /* void removeSelection (); */

btw, while you are here, please remove this comment

@@ +1080,5 @@
>  
>  /* void removeSelection (); */
>  NS_IMETHODIMP nsAccessible::SetSelected(bool aSelect)
>  {
>    // Add or remove selection

this comment is sort of useless, makes sense to remove it

@@ +1102,2 @@
>      }
> +    return aSelect ? TakeFocus() : NS_ERROR_FAILURE;

empty line before this line please?

Comment 18

[Jignesh Kakadiya [:jhk]]

Attachment: patch

Comment 19

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7259e1073226

Comment 20

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/7259e1073226

Comment 21

[Francesco Lodolo [:flod]]

Not sure if it's ok to ask here, but could it be that this bug caused bug 736837 (accesskeys not working anymore on OS X)?

I found this bug searching for ui.key.generalAccessKey on central, and this is the only .cpp file modified in the last days.

Comment 22

[alexander :surkov (:asurkov)]

(In reply to Francesco Lodolo [:flod] from comment #21)
> Not sure if it's ok to ask here, but could it be that this bug caused bug
> 736837 (accesskeys not working anymore on OS X)?

it should be caused by something else since this patch is pure accessibility patch (inside the accessible folder) and accessibility is not a part of default build on OS X. On the other hand accesskeys implementation is something outside the accessibility module.