Implement IA2 containing relations

RESOLVED FIXED in mozilla27

Status

()

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

People

(Reporter: surkov, Assigned: surkov)

Tracking

(Blocks: 1 bug, {access})

unspecified
mozilla27
x86
Windows 7
access
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

4 years ago
IA2_RELATION_CONTAINING_DOCUMENT
IA2_RELATION_CONTAINING_TAB_PANE
IA2_RELATION_CONTAINING_WINDOW
IA2_RELATION_CONTAINING_APPLICATION
(Assignee)

Updated

4 years ago
No longer blocks: 614570
Depends on: 614570
(Assignee)

Updated

4 years ago
Blocks: 368873
(Assignee)

Comment 1

4 years ago
(In reply to alexander :surkov from comment #0)

1) add constants into nsIAccessibleRelation
2) implement them in Accessible::RelationByType (see below)
3) add mapping into windows/msaa/IAccessibleRelation.h/cpp
4) add tests into mochitest/relations/test_tabbrowser.xul

> IA2_RELATION_CONTAINING_DOCUMENT

Accessible::Document()

> IA2_RELATION_CONTAINING_TAB_PANE

IServiceProvider::QueryService SID_IAccessibleContentDocument, move the logic into Accessible::RelationByType

> IA2_RELATION_CONTAINING_WINDOW

similar to previous item, you need to get root doc shell and then get associated document accessible

> IA2_RELATION_CONTAINING_APPLICATION

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

Comment 2

4 years ago
Hi I am interested in working on this bug,but it's my first time to work on with debug,can anybody guide me on how to get started with it?Thanks a lot.
(Assignee)

Comment 3

4 years ago
(In reply to MikeLing from comment #2)
> Hi I am interested in working on this bug,but it's my first time to work on
> with debug,can anybody guide me on how to get started with it?Thanks a lot.

If you don't have good c++ skills then it's better to start from [good first bug]. (Note, I answered you in bug 877985 as well)

Comment 4

4 years ago
(In reply to alexander :surkov from comment #3)
> (In reply to MikeLing from comment #2)
> > Hi I am interested in working on this bug,but it's my first time to work on
> > with debug,can anybody guide me on how to get started with it?Thanks a lot.
> 
> If you don't have good c++ skills then it's better to start from [good first
> bug]. (Note, I answered you in bug 877985 as well)

Thanks for reply,I think a some experience in C++ as a junior college student.And I also know some about python,Can you tell me more about the bug or how can I get the source code?Thanks a lot
(Assignee)

Comment 5

4 years ago
(In reply to MikeLing from comment #4)
> (In reply to alexander :surkov from comment #3)
> > (In reply to MikeLing from comment #2)
> > > Hi I am interested in working on this bug,but it's my first time to work on
> > > with debug,can anybody guide me on how to get started with it?Thanks a lot.
> > 
> > If you don't have good c++ skills then it's better to start from [good first
> > bug]. (Note, I answered you in bug 877985 as well)
> 
> Thanks for reply,I think a some experience in C++ as a junior college
> student.And I also know some about python,

ok, if you didn't work before in large projects then it's still worth to start from good first bugs. anyway up to you

> Can you tell me more about the bug

see comment #1 (use mxr.mozilla.com to search through sources)

> or how can I get the source code?Thanks a lot

https://developer.mozilla.org/en-US/docs/Simple_Firefox_build

Comment 6

4 years ago
(In reply to alexander :surkov from comment #5)
Sir,I've build the environment and get the source code already.I also check the code
from mozilla-source\mozilla-central\accessible\public\nsIAccessible.idl.
Should I add those arguments in comment0 to function  nsIAccessibleRelation getRelationByType(in unsigned long aRelationType) directly?Because I haven't use IAccessible2 API or file.idl before.So I really need help from you.Thank you very much.
(Assignee)

Comment 7

4 years ago
(In reply to MikeLing from comment #6)

> Should I add those arguments in comment0 to function  nsIAccessibleRelation
> getRelationByType(in unsigned long aRelationType) directly?

no, you should add new constants into nsIAccessibleRelation.idl

Comment 8

4 years ago
(In reply to alexander :surkov from comment #7)
I got some problem when I build mozilla with ./match build,It says ImportError: DLL load failed: %1 is not a valid Win32 application.but I'm sure that I put python path in my system path.What should I do?

Comment 9

4 years ago
(In reply to MikeLing from comment #8)
And I'm not  very clear about about new constants.
(Assignee)

Comment 10

4 years ago
(In reply to MikeLing from comment #8)
> (In reply to alexander :surkov from comment #7)
> I got some problem when I build mozilla with ./match build,It says
> ImportError: DLL load failed: %1 is not a valid Win32 application.but I'm
> sure that I put python path in my system path.What should I do?

it's better to ask question on #developers or #build

(In reply to MikeLing from comment #9)
> (In reply to MikeLing from comment #8)
> And I'm not  very clear about about new constants.

you need to add 4 constants like RELATION_CONTAINING_DOCUMENT (see comment #0), check nsIAccessibleRelation to see how other relation constants are defined there.

Comment 11

4 years ago
(In reply to alexander :surkov from comment #10)
Sorry,I fell it's too hard to me right now.
(Assignee)

Comment 12

4 years ago
(In reply to MikeLing from comment #11)
> (In reply to alexander :surkov from comment #10)
> Sorry,I fell it's too hard to me right now.

if it's your first time working with mozilla sources then it's worth to start from good first bugs like

https://bugzilla.mozilla.org/buglist.cgi?list_id=6719779&resolution=---&query_based_on=good-first-bug&status_whiteboard_type=substring&query_format=advanced&status_whiteboard=[good%20first%20bug]&component=Disability%20Access%20APIs&product=Core&known_name=good-first-bug
Created attachment 770904 [details] [diff] [review]
WIP

Initial patch minus tests. Feedback welcomed.
Assignee: nobody → andrew.quartey
Attachment #770904 - Flags: feedback?(surkov.alexander)
(Assignee)

Comment 14

4 years ago
Comment on attachment 770904 [details] [diff] [review]
WIP

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

please add navRelations stuffs

::: accessible/public/nsIAccessibleRelation.idl
@@ +114,5 @@
> +  * IAccessibleDocument interface.
> +  */
> +  const unsigned long RELATION_CONTAINING_DOCUMENT = 0x11;
> +
> +/**

nit: two spaces indent, here and below

@@ +125,5 @@
> +  */
> +  const unsigned long RELATION_CONTAINING_WINDOW = 0x13;
> +
> +/**
> +  * The target object is the containing application object. 

nit: whitespace in the end of line

::: accessible/src/generic/Accessible.cpp
@@ +2197,5 @@
> +                                       IID_IAccessible, (void**) &docAcc);
> +        if (SUCCEEDED(hr))
> +          return Relation(docAcc, mContent);
> +      }
> +      return  Relation();

it should be done in vise versa way, i.e. IServiceProvider should be implemented via RelationsByType()
Attachment #770904 - Flags: feedback?(surkov.alexander)
Created attachment 771468 [details] [diff] [review]
WIP

Fixed nits and added the navRelations bits. Also, since the implementations for |RELATION_CONTAINING_TAB_PANE| and |RELATION_CONTAINING_WINDOW| are nearly identical - they differ by only by the calls to GetSameTypeRootTreeItem and GetRootTreeItem- i can probably refactor that into a common function.

Patch sent to Try: https://tbpl.mozilla.org/?tree=Try&rev=99b9bc08e6a2
Attachment #770904 - Attachment is obsolete: true
Attachment #771468 - Flags: feedback?(surkov.alexander)
(Assignee)

Comment 16

4 years ago
Comment on attachment 771468 [details] [diff] [review]
WIP

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

you didn't change IServiceProvider implementation, was it intentional?
(Assignee)

Comment 17

4 years ago
Comment on attachment 771468 [details] [diff] [review]
WIP

canceling feedback request until comment #16 answered
Attachment #771468 - Flags: feedback?(surkov.alexander)
Unassigning myself to allow someone else to work on this.
Assignee: andrew.quartey → nobody
(Assignee)

Comment 19

4 years ago
Created attachment 814150 [details] [diff] [review]
patch

1) no window relation since it's currently MSAA specific, no easy way to add but AT has easy way to get the window having HWND
2) tab pane relation returns a content document what doesn't seem follow the spec. It's friendly with ServiceProvider corresponding IID and friendly to content in own process architecture. If this impl is rather a problem then fix it then
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #814150 - Flags: review?(trev.saunders)
(Assignee)

Comment 20

4 years ago
(In reply to alexander :surkov from comment #19)
> Created attachment 814150 [details] [diff] [review]
> patch
> 
> 1) no window relation since it's currently MSAA specific, no easy way to add
> but AT has easy way to get the window having HWND

or probably it makes sense to add a constant but return nullptr. Let me know what you think is right/better
Comment on attachment 814150 [details] [diff] [review]
patch

>+   * The target object is the containing document object which implements
>+   * IAccessibleDocument interface.
>+   */
>+  const unsigned long RELATION_CONTAINING_DOCUMENT = 0x11;

seems funny to talk about ia2 thing here

>+
>+  /**
>+   * The target object is the containing tab pane object.
>+   */
>+  const unsigned long RELATION_CONTAINING_TAB_PANE = 0x12;

I'd think it would be more useful to say tab document or something at least that would seem more consistant with other comments

>+
>+  /**
>+   * The target object is the containing application object.
>+   */
>+  const unsigned long RELATION_CONTAINING_APPLICATION = 0x14;

wouldn't application accessible be more consistant?

>+
>+  /**
>+   * The target object is the containing document object.
>+   */
>+  CONTAINING_DOCUMENT = 0x11,

same  for this file

btw why don't we put comments in map file and generate this one?
Attachment #814150 - Flags: review?(trev.saunders) → review+
(Assignee)

Comment 22

4 years ago
(In reply to Trevor Saunders (:tbsaunde) from comment #21)

> >+   * The target object is the containing application object.
> >+   */
> >+  const unsigned long RELATION_CONTAINING_APPLICATION = 0x14;
> 
> wouldn't application accessible be more consistant?

it seems all constant descriptions use 'object' is used instead 'accessible'

> btw why don't we put comments in map file and generate this one?

I like to have constants not generated
(In reply to alexander :surkov from comment #22)
> (In reply to Trevor Saunders (:tbsaunde) from comment #21)
> 
> > >+   * The target object is the containing application object.
> > >+   */
> > >+  const unsigned long RELATION_CONTAINING_APPLICATION = 0x14;
> > 
> > wouldn't application accessible be more consistant?
> 
> it seems all constant descriptions use 'object' is used instead 'accessible'

THAT'S SAID i THINK IT WOULD BE MORE NATURAL / CONSISTANT WITH OTHER THINGS TO USE ACCESSIBLE.

> > btw why don't we put comments in map file and generate this one?
> 
> I like to have constants not generated

WHY?
(Assignee)

Comment 24

4 years ago
(In reply to Trevor Saunders (:tbsaunde) from comment #23)

> THAT'S SAID i THINK IT WOULD BE MORE NATURAL / CONSISTANT WITH OTHER THINGS
> TO USE ACCESSIBLE.

I agree but if rename them then at once

> > > btw why don't we put comments in map file and generate this one?
> > 
> > I like to have constants not generated
> 
> WHY?

constants is a primary thing, map is a secondary thing, it seems to be a natural order of things
(Assignee)

Comment 25

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f85c0d3d7f31
(Assignee)

Updated

4 years ago
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/f85c0d3d7f31
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.