The default bug view has changed. See this FAQ.

nsAccUtils::GetMultiSelectableContainer should use nsDocAccessible::GetAccessible

RESOLVED FIXED in mozilla14

Status

()

Core
Disability Access APIs
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: surkov, Assigned: jhk)

Tracking

unspecified
mozilla14
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=surkov.alexander@gmail.com][lang=c++])

Attachments

(1 attachment, 8 obsolete attachments)

(Reporter)

Description

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

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

Comment 1

5 years ago
Created attachment 604775 [details] [diff] [review]
Patch
Attachment #604775 - Flags: feedback?(surkov.alexander)
(Reporter)

Comment 2

5 years ago
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
Attachment #604775 - Flags: feedback?(surkov.alexander)
(Assignee)

Comment 3

5 years ago
Created attachment 604936 [details] [diff] [review]
Patch
Assignee: nobody → jigneshhk1992
Attachment #604775 - Attachment is obsolete: true
Attachment #604936 - Flags: review?(surkov.alexander)
(Reporter)

Comment 4

5 years ago
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
}
(Assignee)

Comment 5

5 years ago
Created attachment 605722 [details] [diff] [review]
Patch
Attachment #604936 - Attachment is obsolete: true
Attachment #604936 - Flags: review?(surkov.alexander)
Attachment #605722 - Flags: review?(surkov.alexander)
(Reporter)

Comment 6

5 years ago
Comment on attachment 605722 [details] [diff] [review]
Patch

I'll do feedback, let's Eitan for review
Attachment #605722 - Flags: review?(surkov.alexander) → review?(eitan)
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.
Attachment #605722 - Flags: review?(eitan) → review+
(Assignee)

Comment 8

5 years ago
Created attachment 605746 [details] [diff] [review]
patch

Nit fixed.
Attachment #605722 - Attachment is obsolete: true
(Reporter)

Comment 9

5 years ago
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).
Attachment #605746 - Flags: feedback-
(Assignee)

Comment 10

5 years ago
Created attachment 606127 [details] [diff] [review]
patch
Attachment #605746 - Attachment is obsolete: true
Attachment #606127 - Flags: feedback?(surkov.alexander)
(Reporter)

Comment 11

5 years ago
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
Attachment #606127 - Flags: feedback?(surkov.alexander) → feedback-
(Assignee)

Comment 12

5 years ago
Created attachment 606191 [details] [diff] [review]
patch
Attachment #606127 - Attachment is obsolete: true
Attachment #606191 - Flags: feedback?(surkov.alexander)
(Reporter)

Comment 13

5 years ago
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
Attachment #606191 - Flags: feedback?(surkov.alexander)
(Assignee)

Comment 14

5 years ago
Created attachment 606242 [details] [diff] [review]
patch
Attachment #606191 - Attachment is obsolete: true
Attachment #606242 - Flags: feedback?(surkov.alexander)
(Reporter)

Comment 15

5 years ago
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
Attachment #606242 - Flags: feedback?(surkov.alexander) → feedback-
(Assignee)

Comment 16

5 years ago
Created attachment 606295 [details] [diff] [review]
Patch
Attachment #606242 - Attachment is obsolete: true
Attachment #606295 - Flags: feedback?(surkov.alexander)
(Reporter)

Comment 17

5 years ago
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?
Attachment #606295 - Flags: feedback?(surkov.alexander) → feedback+
(Assignee)

Comment 18

5 years ago
Created attachment 606307 [details] [diff] [review]
patch
Attachment #606295 - Attachment is obsolete: true
(Reporter)

Comment 19

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/7259e1073226
https://hg.mozilla.org/mozilla-central/rev/7259e1073226
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
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.
(Reporter)

Comment 22

5 years ago
(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.
You need to log in before you can comment on or make changes to this bug.