Last Comment Bug 873450 - Implement IA2 containing relations
: Implement IA2 containing relations
Status: RESOLVED FIXED
[mentor=surkov.alexander@gmail.com][l...
: access
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: x86 Windows 7
: -- normal (vote)
: mozilla27
Assigned To: alexander :surkov
:
: alexander :surkov
Mentors:
Depends on: IA2_1.3
Blocks: ia2
  Show dependency treegraph
 
Reported: 2013-05-17 06:03 PDT by alexander :surkov
Modified: 2013-10-25 19:12 PDT (History)
4 users (show)
surkov.alexander: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP (7.84 KB, patch)
2013-07-03 11:04 PDT, Andrew Quartey [:drexler]
no flags Details | Diff | Splinter Review
WIP (10.10 KB, patch)
2013-07-04 13:08 PDT, Andrew Quartey [:drexler]
no flags Details | Diff | Splinter Review
patch (12.21 KB, patch)
2013-10-07 09:29 PDT, alexander :surkov
tbsaunde+mozbugs: review+
Details | Diff | Splinter Review

Description alexander :surkov 2013-05-17 06:03:42 PDT
IA2_RELATION_CONTAINING_DOCUMENT
IA2_RELATION_CONTAINING_TAB_PANE
IA2_RELATION_CONTAINING_WINDOW
IA2_RELATION_CONTAINING_APPLICATION
Comment 1 alexander :surkov 2013-05-26 18:47:52 PDT
(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 2013-06-02 16:13:50 PDT
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 2013-06-02 20:00:21 PDT
(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 2013-06-02 22:06:37 PDT
(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 2013-06-03 01:11:47 PDT
(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 2013-06-03 08:38:43 PDT
(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 2013-06-03 08:52:44 PDT
(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 2013-06-03 17:05:52 PDT
(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 2013-06-03 17:37:20 PDT
(In reply to MikeLing from comment #8)
And I'm not  very clear about about new constants.
Comment 10 alexander :surkov 2013-06-03 18:34:41 PDT
(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 2013-06-04 07:54:40 PDT
(In reply to alexander :surkov from comment #10)
Sorry,I fell it's too hard to me right now.
Comment 12 alexander :surkov 2013-06-04 08:07:29 PDT
(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] 2013-07-03 11:04:09 PDT
Created attachment 770904 [details] [diff] [review]
WIP

Initial patch minus tests. Feedback welcomed.
Comment 14 alexander :surkov 2013-07-03 16:41:25 PDT
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] 2013-07-04 13:08:48 PDT
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
Comment 16 alexander :surkov 2013-07-07 07:03:18 PDT
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 2013-07-15 06:38:56 PDT
Comment on attachment 771468 [details] [diff] [review]
WIP

canceling feedback request until comment #16 answered
Comment 18 Andrew Quartey [:drexler] 2013-09-28 12:11:38 PDT
Unassigning myself to allow someone else to work on this.
Comment 19 alexander :surkov 2013-10-07 09:29:09 PDT
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
Comment 20 alexander :surkov 2013-10-07 09:30:48 PDT
(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) 2013-10-16 13:04:35 PDT
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 2013-10-17 18:00:42 PDT
(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) 2013-10-23 14:14:46 PDT
(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 2013-10-23 14:25:54 PDT
(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 26 Phil Ringnalda (:philor) 2013-10-25 19:12:18 PDT
https://hg.mozilla.org/mozilla-central/rev/f85c0d3d7f31

Note You need to log in before you can comment on or make changes to this bug.