Closed Bug 873450 Opened 6 years ago Closed 6 years ago

Implement IA2 containing relations

Categories

(Core :: Disability Access APIs, defect)

x86
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: surkov, Assigned: surkov)

References

(Blocks 1 open bug)

Details

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

Attachments

(2 files, 1 obsolete file)

IA2_RELATION_CONTAINING_DOCUMENT
IA2_RELATION_CONTAINING_TAB_PANE
IA2_RELATION_CONTAINING_WINDOW
IA2_RELATION_CONTAINING_APPLICATION
No longer blocks: IA2_1.3
Depends on: IA2_1.3
Blocks: ia2
(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++]
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.
(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)
(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
(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
(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.
(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
(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?
(In reply to MikeLing from comment #8)
And I'm not  very clear about about new constants.
(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.
(In reply to alexander :surkov from comment #10)
Sorry,I fell it's too hard to me right now.
(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
Attached patch WIP (obsolete) — Splinter Review
Initial patch minus tests. Feedback welcomed.
Assignee: nobody → andrew.quartey
Attachment #770904 - Flags: feedback?(surkov.alexander)
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)
Attached patch WIPSplinter Review
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)
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 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
Attached patch patchSplinter Review
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)
(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+
(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?
(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
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/f85c0d3d7f31
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.