IAccessible::accNavigate with NAVRELATION_* fails

VERIFIED FIXED in Firefox 22

Status

()

defect
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: Jamie, Assigned: surkov)

Tracking

({regression})

Trunk
mozilla24
x86_64
Windows 7
Points:
---

Firefox Tracking Flags

(firefox22 verified, firefox23 verified, firefox24 verified)

Details

Attachments

(2 attachments)

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.
Posted file Test case.
How can I trigger it?
I don't understand. You trigger it as per the str I gave above. I don't know how to trigger it otherwise.
(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.
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))
Posted patch patchSplinter Review
Assignee: nobody → surkov.alexander
Attachment #756425 - Flags: review?(trev.saunders)
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 :(
Attachment #756425 - Flags: review?(trev.saunders) → review+
(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
(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?
(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
https://hg.mozilla.org/mozilla-central/rev/fbccd02d5cff
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
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.
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 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
Attachment #756425 - Flags: approval-mozilla-aurora?
Verified fixed in Firefox 24.0a1 (2013-06-07).
Status: RESOLVED → VERIFIED
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.
Attachment #756425 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
(In reply to lsblakk@mozilla.com [: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 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
Attachment #756425 - Flags: approval-mozilla-beta?
Attachment #756425 - Flags: approval-mozilla-aurora?
Attachment #756425 - Flags: approval-mozilla-aurora-
Comment on attachment 756425 [details] [diff] [review]
patch

Approving to fix comment 13, given low risk and high reward for a11y users.
Attachment #756425 - Flags: approval-mozilla-beta?
Attachment #756425 - Flags: approval-mozilla-beta+
Attachment #756425 - Flags: approval-mozilla-aurora?
Attachment #756425 - Flags: approval-mozilla-aurora+
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).
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)
Verified as fixed on Firefox 24 beta 1:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20100101 Firefox/24.0
Keywords: verifyme
QA Contact: ioana.budnar
You need to log in before you can comment on or make changes to this bug.