873450 - Implement IA2 containing relations

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

IA2_RELATION_CONTAINING_DOCUMENT
IA2_RELATION_CONTAINING_TAB_PANE
IA2_RELATION_CONTAINING_WINDOW
IA2_RELATION_CONTAINING_APPLICATION

Comment 1

[alexander :surkov (:asurkov)]

(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()

Comment 2

[MikeLing]

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.

Comment 3

[alexander :surkov (:asurkov)]

(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

[MikeLing]

(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

Comment 5

[alexander :surkov (:asurkov)]

(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

[MikeLing]

(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.

Comment 7

[alexander :surkov (:asurkov)]

(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

[MikeLing]

(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

[MikeLing]

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

Comment 10

[alexander :surkov (:asurkov)]

(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

[MikeLing]

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

Comment 12

[alexander :surkov (:asurkov)]

(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

Comment 13

[Andrew Quartey [:drexler]]

Attachment: WIP

Initial patch minus tests. Feedback welcomed.

Comment 14

[alexander :surkov (:asurkov)]

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()

Comment 15

[Andrew Quartey [:drexler]]

Attachment: 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

Comment 16

[alexander :surkov (:asurkov)]

Comment on attachment 771468 [details] [diff] [review]
WIP

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

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

Comment 17

[alexander :surkov (:asurkov)]

Comment on attachment 771468 [details] [diff] [review]
WIP

canceling feedback request until comment #16 answered

Comment 18

[Andrew Quartey [:drexler]]

Unassigning myself to allow someone else to work on this.

Comment 19

[alexander :surkov (:asurkov)]

Attachment: 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

Comment 20

[alexander :surkov (:asurkov)]

(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 21

[Trevor Saunders (:tbsaunde)]

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?

Comment 22

[alexander :surkov (:asurkov)]

(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

Comment 23

[Trevor Saunders (:tbsaunde)]

(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?

Comment 24

[alexander :surkov (:asurkov)]

(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

Comment 25

[alexander :surkov (:asurkov)]

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

Comment 26

[Phil Ringnalda (:philor)]

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