Last Comment Bug 726072 - nsAccUtils::GetMultiSelectableContainer should use nsDocAccessible::GetAccessible
: nsAccUtils::GetMultiSelectableContainer should use nsDocAccessible::GetAccess...
Status: RESOLVED FIXED
[good first bug][mentor=surkov.alexan...
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla14
Assigned To: Jignesh Kakadiya [:jhk]
:
: alexander :surkov
Mentors:
Depends on:
Blocks: 725572
  Show dependency treegraph
 
Reported: 2012-02-10 10:37 PST by alexander :surkov
Modified: 2012-03-20 00:30 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (3.73 KB, patch)
2012-03-11 08:34 PDT, Jignesh Kakadiya [:jhk]
no flags Details | Diff | Splinter Review
Patch (3.45 KB, patch)
2012-03-12 07:56 PDT, Jignesh Kakadiya [:jhk]
no flags Details | Diff | Splinter Review
Patch (4.10 KB, patch)
2012-03-14 06:22 PDT, Jignesh Kakadiya [:jhk]
eitan: review+
Details | Diff | Splinter Review
patch (4.08 KB, patch)
2012-03-14 07:34 PDT, Jignesh Kakadiya [:jhk]
surkov.alexander: feedback-
Details | Diff | Splinter Review
patch (4.35 KB, patch)
2012-03-14 23:51 PDT, Jignesh Kakadiya [:jhk]
surkov.alexander: feedback-
Details | Diff | Splinter Review
patch (4.28 KB, patch)
2012-03-15 06:59 PDT, Jignesh Kakadiya [:jhk]
no flags Details | Diff | Splinter Review
patch (4.38 KB, patch)
2012-03-15 09:01 PDT, Jignesh Kakadiya [:jhk]
surkov.alexander: feedback-
Details | Diff | Splinter Review
Patch (4.42 KB, patch)
2012-03-15 11:10 PDT, Jignesh Kakadiya [:jhk]
surkov.alexander: feedback+
Details | Diff | Splinter Review
patch (4.51 KB, patch)
2012-03-15 11:38 PDT, Jignesh Kakadiya [:jhk]
no flags Details | Diff | Splinter Review

Description alexander :surkov 2012-02-10 10:37:34 PST
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] 2012-03-11 08:34:04 PDT
Created attachment 604775 [details] [diff] [review]
Patch
Comment 2 alexander :surkov 2012-03-11 19:03:03 PDT
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] 2012-03-12 07:56:41 PDT
Created attachment 604936 [details] [diff] [review]
Patch
Comment 4 alexander :surkov 2012-03-12 08:10:17 PDT
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] 2012-03-14 06:22:29 PDT
Created attachment 605722 [details] [diff] [review]
Patch
Comment 6 alexander :surkov 2012-03-14 06:30:50 PDT
Comment on attachment 605722 [details] [diff] [review]
Patch

I'll do feedback, let's Eitan for review
Comment 7 Eitan Isaacson [:eeejay] 2012-03-14 07:13:11 PDT
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] 2012-03-14 07:34:37 PDT
Created attachment 605746 [details] [diff] [review]
patch

Nit fixed.
Comment 9 alexander :surkov 2012-03-14 07:54:16 PDT
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] 2012-03-14 23:51:31 PDT
Created attachment 606127 [details] [diff] [review]
patch
Comment 11 alexander :surkov 2012-03-15 06:30:26 PDT
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] 2012-03-15 06:59:39 PDT
Created attachment 606191 [details] [diff] [review]
patch
Comment 13 alexander :surkov 2012-03-15 07:02:39 PDT
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] 2012-03-15 09:01:42 PDT
Created attachment 606242 [details] [diff] [review]
patch
Comment 15 alexander :surkov 2012-03-15 11:06:27 PDT
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] 2012-03-15 11:10:41 PDT
Created attachment 606295 [details] [diff] [review]
Patch
Comment 17 alexander :surkov 2012-03-15 11:17:10 PDT
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] 2012-03-15 11:38:05 PDT
Created attachment 606307 [details] [diff] [review]
patch
Comment 20 Marco Bonardo [::mak] 2012-03-16 06:33:22 PDT
https://hg.mozilla.org/mozilla-central/rev/7259e1073226
Comment 21 Francesco Lodolo [:flod] (mostly out of office until Dec 19) 2012-03-18 07:10:28 PDT
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 2012-03-20 00:30:17 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.