Note: There are a few cases of duplicates in user autocompletion which are being worked on.

clean up nsIAccessible action methods

RESOLVED FIXED in mozilla15

Status

()

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

People

(Reporter: surkov, Assigned: Oussama BADR)

Tracking

(Blocks: 1 bug, {access})

unspecified
mozilla15
x86
Mac OS X
access
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

6 years ago
numActions -> actionCount

not sure whether we should rename getActionName to getActionNameAt and others.
(Reporter)

Comment 1

5 years ago
Trevor, what do you think about adding 'at' postfix to method names?
(In reply to alexander :surkov from comment #1)
> Trevor, what do you think about adding 'at' postfix to method names?

I don't have much of an opinion either way.  Treating the actions like an array feels funny to me in general, but given windows and linux both want it it that way I guess we should go with that api like that.
(Reporter)

Comment 3

5 years ago
Yep, it's sort of weird. So let's do not do extra work we aren't sure about.

So do numActions -> actionCount for now.
Whiteboard: [good first bug][mentor=trev.saunders@gmail.com][lang=c++]
(Assignee)

Comment 4

5 years ago
Alexander, Can you put more details?
are you talking about renaming action methods in nsIAccessible( getActionName; getActionDescription; doAction) by putting "At" as a  postfix ?
(Reporter)

Comment 5

5 years ago
(In reply to Oussama BADR from comment #4)
> Alexander, Can you put more details?
> are you talking about renaming action methods in nsIAccessible(
> getActionName; getActionDescription; doAction) by putting "At" as a  postfix
> ?

no, just change numActions to actionCount (per comment #3)
No longer blocks: 672506
(Assignee)

Comment 6

5 years ago
Ok, you can assign me in, thanks in advance!
(Reporter)

Updated

5 years ago
Assignee: nobody → oussamabadr
(Assignee)

Comment 7

5 years ago
Created attachment 625902 [details] [diff] [review]
Patch to fix Bug 675879.
Attachment #625902 - Flags: review?(surkov.alexander)
(Reporter)

Comment 8

5 years ago
Comment on attachment 625902 [details] [diff] [review]
Patch to fix Bug 675879.

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

you need to change UUID of nsIAccessible interface and you didn't changed implementation (look for GetNumActions method)
Attachment #625902 - Flags: review?(surkov.alexander)
(Assignee)

Comment 9

5 years ago
Created attachment 626419 [details] [diff] [review]
Patch V2 to fix Bug 675879.
Attachment #626419 - Flags: review?(surkov.alexander)
(Reporter)

Comment 10

5 years ago
Comment on attachment 626419 [details] [diff] [review]
Patch V2 to fix Bug 675879.

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

you didn't change all instances of 'numActions' in mochitests folder (for example, actions/test_general.html), canceling review, I need to look at next patch.

::: accessible/public/nsIAccessible.idl
@@ +289,5 @@
>  
>    /**
>     * The number of accessible actions associated with this accessible
>     */
> +  readonly attribute PRUint8 actionCount;

you didn't change the uuid of nsIAccessible interface, please do that

::: accessible/src/base/nsAccessible.cpp
@@ +1867,5 @@
>  
>    *aActionCount = ActionCount();
>    return NS_OK;
>  }
> + 

whitespace on empty line
Attachment #626419 - Flags: review?(surkov.alexander)
(Reporter)

Updated

5 years ago
Attachment #625902 - Attachment is obsolete: true
Over the weekend an issue occurred with the BMO database which resulted in duplication of dependencies. The dependency issue may have resulted in "Depends On" and "Blocks" values being removed while updating a bug. This issue should now be resolved, however dependencies may need to be manually restored to some bugs.

This bug had dependencies removed during the failure period and will need verification that the dependency removal(s) were intentional. Please help out by taking a look at this bug and adding anything back that was mistakenly removed.
(Assignee)

Comment 12

5 years ago
> >    /**
> >     * The number of accessible actions associated with this accessible
> >     */
> > +  readonly attribute PRUint8 actionCount;
> 
> you didn't change the uuid of nsIAccessible interface, please do that

I didn't match what I have to do for UUID, I have found this line for UUID in nsIAccessible interface :
...
[scriptable, uuid(46d422d1-c92f-4536-bdef-f77bc8350ec7)]
interface nsIAccessible : nsISupports
...
really sorry, but I have lost my fingers in!!!
(Assignee)

Updated

5 years ago
Attachment #626419 - Attachment is obsolete: true
(Reporter)

Comment 13

5 years ago
(In reply to Oussama BADR from comment #12)

> [scriptable, uuid(46d422d1-c92f-4536-bdef-f77bc8350ec7)]
> interface nsIAccessible : nsISupports

yes, you need to replace this uuid on new one
(Assignee)

Comment 14

5 years ago
Created attachment 627150 [details] [diff] [review]
Patch V3 to fix Bug 675879.
Attachment #627150 - Flags: review?(surkov.alexander)
(Assignee)

Comment 15

5 years ago
(In reply to alexander :surkov from comment #10)
> Comment on attachment 626419 [details] [diff] [review]
> -----------------------------------------------------------------
> 
> you didn't change all instances of 'numActions' in mochitests folder (for
> example, actions/test_general.html), canceling review, I need to look at
> next patch.
> 

to check out 'numActions' instances in all accessible folders, I have used 'grep' command in my Ubuntu OS instead of Mozilla mxr web tool, the last one didn't display all occurrences!
(Reporter)

Comment 16

5 years ago
Comment on attachment 627150 [details] [diff] [review]
Patch V3 to fix Bug 675879.

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

::: accessible/tests/mochitest/actions/test_general.html
@@ +45,5 @@
>        testActions(actionsArray);
>  
>        getAccessible("onclick_img").takeFocus();
> +is(getAccessible("link1").actionCount, 1, "links should have one action");
> +is(getAccessible("link2").actionCount, 1, "link with onclick handler should have 1 action");

please make proper indentation while you are here
Attachment #627150 - Flags: review?(surkov.alexander) → review+
(Assignee)

Comment 17

5 years ago
Created attachment 627171 [details] [diff] [review]
Patch V4 to fix Bug 675879.

...with right indentations!
Attachment #627171 - Flags: review?(surkov.alexander)
(Reporter)

Comment 18

5 years ago
Comment on attachment 627171 [details] [diff] [review]
Patch V4 to fix Bug 675879.

you don't need to rerequest review for minor changes.

I'll land the patch, thank you for the fix
Attachment #627171 - Flags: review?(surkov.alexander) → review+
(Assignee)

Comment 19

5 years ago
welcome!

but, why did I change the UUID of nsIAccessible interface?
(Reporter)

Comment 20

5 years ago
(In reply to Oussama BADR from comment #19)
> welcome!
> 
> but, why did I change the UUID of nsIAccessible interface?

Let's you have a c++ extension that use that interface, you do query interface by passing that UUID then you get back an interface pointer and you call a method. Since interface was changed then you may end up with calling wrong code or crash. You need to read how methods calls are implemented in c++ for more information. If you changed UUID then that QueryInterface will return null pointer.
(Assignee)

Comment 21

5 years ago
thks for explanations!!

may be I should find out a new bug, but (I think) I'm not ready to do it, so please led me to the next bug!
(Reporter)

Updated

5 years ago
Attachment #627150 - Attachment is obsolete: true
(Reporter)

Updated

5 years ago
Blocks: 672506
(Reporter)

Comment 22

5 years ago
(In reply to Oussama BADR from comment #21)

> may be I should find out a new bug, but (I think) I'm not ready to do it, so
> please led me to the next bug!

thank you, I cc'ed you on bug 672514.
https://hg.mozilla.org/mozilla-central/rev/ca31a899ce4e
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
(Reporter)

Updated

5 years ago
Depends on: 838076
You need to log in before you can comment on or make changes to this bug.