877532 - IAccessible::accNavigate with NAVRELATION_* fails

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect)

Description

[James Teh [:Jamie]]

Str:
1. Open the attached test case.
2. Retrieve the IAccessible for the first table.
3. Call IAccessible::accNavigate(NAVRELATION_LABELLED_BY, CHILDID_SELF).
Expected: The caption accessible should be returned.
Actual: E_FAIL.

This works in Firefox 21 but is broken in Firefox 22 and beyond.

Comment 1

[James Teh [:Jamie]]

Attachment: Test case.

Comment 2

[alexander :surkov (:asurkov)]

How can I trigger it?

Comment 3

[James Teh [:Jamie]]

I don't understand. You trigger it as per the str I gave above. I don't know how to trigger it otherwise.

Comment 4

[alexander :surkov (:asurkov)]

(In reply to James Teh [:Jamie] from comment #3)
> I don't understand. You trigger it as per the str I gave above. I don't know
> how to trigger it otherwise.

I hoped there's a tool or perhaps I could use NVDA for that. I'm pretty sure the bug is on MSAA layer because core logic is covered by automated testing.

Comment 5

[James Teh [:Jamie]]

I haven't actually implemented any functionality which uses this yet. However, here's how you can trigger it using the NVDA Python console.

You're using a Windows vm on a Mac, so you can't use the NVDA key, which is a bit painful, so you first need to temporarily bind a key you can use to contextually activate the NVDA Python console:
1. Right click the NVDA icon on the system tray, then Tools -> Python console.
2. Type these lines (you'll need to paste each line separately):
import globalCommands
globalCommands.commands.bindGesture("kb:control+alt+z", "activatePythonConsole")

Now the actual steps to trigger:
1. Open the test case.
2. Make sure focus is on the document.
3. Press control+alt+z.
4. Type:
focus.getChild(2).IAccessibleObject.accNavigate(0x1003, 0)
Expected: Output should say something like: <comtypes.client.lazybind.Dispatch object at 0x05C29290>
Actual: Output says something like:
Traceback (most recent call last):
  File "<console>", line 1, in <module>
COMError: (-2147467259, 'Unspecified error', (None, None, None, 0, None))

Comment 6

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 7

[Trevor Saunders (:tbsaunde)]

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

>-  if (xpRelation) {
>+  if (xpRelation != -1) {

why not < 0?

I'd prefer we checked it was less than the max relation, but we don't have a constant for that anymore :(

Comment 8

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #7)
> Comment on attachment 756425 [details] [diff] [review]
> patch
> 
> >-  if (xpRelation) {
> >+  if (xpRelation != -1) {
> 
> why not < 0?

it's the same, why does it matter?

> I'd prefer we checked it was less than the max relation, but we don't have a
> constant for that anymore :(

we live under assumption constants are not negative, so it should be ok

Comment 9

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #8)
> (In reply to Trevor Saunders (:tbsaunde) from comment #7)
> > Comment on attachment 756425 [details] [diff] [review]
> > patch
> > 
> > >-  if (xpRelation) {
> > >+  if (xpRelation != -1) {
> > 
> > why not < 0?
> 
> it's the same, why does it matter?

just treating -1 special looks funny

> > I'd prefer we checked it was less than the max relation, but we don't have a
> > constant for that anymore :(
> 
> we live under assumption constants are not negative, so it should be ok

sure but what's the point in calling RelationByType() if you know  the relation type you want doesn't actually exist?

Comment 10

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #9)
> (In reply to alexander :surkov from comment #8)
> > (In reply to Trevor Saunders (:tbsaunde) from comment #7)
> > > Comment on attachment 756425 [details] [diff] [review]
> > > patch
> > > 
> > > >-  if (xpRelation) {
> > > >+  if (xpRelation != -1) {
> > > 
> > > why not < 0?
> > 
> > it's the same, why does it matter?
> 
> just treating -1 special looks funny

so you prefer to have

if (xpRelation >= 0) {
  Relation rel = RelationByType();
}

> > > I'd prefer we checked it was less than the max relation, but we don't have a
> > > constant for that anymore :(
> > 
> > we live under assumption constants are not negative, so it should be ok
> 
> sure but what's the point in calling RelationByType() if you know  the
> relation type you want doesn't actually exist?

no point but we don't do that since we return early

Comment 11

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fbccd02d5cff

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fbccd02d5cff

Comment 13

[James Teh [:Jamie]]

Any chance of getting this backported? Otherwise, proper support for table captions in NVDA will work for Firefox 21 but not for 22 or 23.

Comment 14

[Marco Zehe (:MarcoZ)]

Alex, what bug is this a regression from? The keyword indicates that it is, but I don't see any dependency. It would help the beta and aurora approval cases if we knew where this came from.

Comment 15

[alexander :surkov (:asurkov)]

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

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 873453
User impact if declined: NVDA and some AT are partially broken
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): no
String or IDL/UUID changes made by this patch: no

Comment 16

[James Teh [:Jamie]]

Verified fixed in Firefox 24.0a1 (2013-06-07).

Comment 17

[Lukas Blakk [:lsblakk] use ?needinfo]

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

The bug that the regression is from landed only on mozilla 24, so this shouldn't need to be uplifted to 23.

Comment 18

[alexander :surkov (:asurkov)]

(In reply to [e-mail] [:lsblakk] from comment #17)
> Comment on attachment 756425 [details] [diff] [review]
> patch
> 
> The bug that the regression is from landed only on mozilla 24, so this
> shouldn't need to be uplifted to 23.

sorry a wrong bug number was specified, it was supposed to be bug 527461.

Comment 19

[alexander :surkov (:asurkov)]

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

[Approval Request Comment]
Bug caused by (feature/regressing bug #):  bug 527461
User impact if declined: NVDA and some AT are partially broken
Testing completed (on m-c, etc.): no
Risk to taking this patch (and alternatives if risky): no
String or IDL/UUID changes made by this patch:no

Comment 20

[Alex Keybl [:akeybl]]

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

Approving to fix comment 13, given low risk and high reward for a11y users.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f191b6f1b424
https://hg.mozilla.org/releases/mozilla-beta/rev/45e0653fe06e

Comment 22

[Ioana (away)]

Verified as fixed with Firefox 22.0 (RC - 20130618035212) on a Windows 7 x64 VM running on Mac OSX 10.7.5. I used the steps in comment 5 and got the expected output (with a different address than the example, but that should be expected).

Comment 23

[Ioana (away)]

Verified as fixed under the conditions in comment 22 on:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:23.0) Gecko/20100101 Firefox/23.0 (20130725195523)

Comment 24

[Ioana (away)]

Verified as fixed on Firefox 24 beta 1:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20100101 Firefox/24.0