594413 - Provide QueryService for main document accessible

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[David Bolter [:davidb] (NeedInfo me for attention)]

Requested by a major vendor.

Comment 1

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: WIP

Alexander, your early feedback appreciated.

Comment 2

[alexander :surkov (:asurkov)]

1) they aren't interested to get root document accessible for accessible in tab content
2) you should distinguish where's the accessible that QueryService() is called on so that you should return document accessible associated with tab if you're inside a tab content, and you should return root accessible if you're outside of tab content I think

Comment 3

[David Bolter [:davidb] (NeedInfo me for attention)]

OK if I change the call to GetSameTypeRootTreeItem I understand that will work for non-chrome documents. I'm uncertain how best to handle chrome docs.

Comment 4

[David Bolter [:davidb] (NeedInfo me for attention)]

Neil Deakin tells me that for all chrome documents that load in a tab, GetSameTypeRootTreeItem should do what we need.

Comment 5

[alexander :surkov (:asurkov)]

(In reply to comment #4)
> Neil Deakin tells me that for all chrome documents that load in a tab,
> GetSameTypeRootTreeItem should do what we need.

Ok, great. Btw, we need a mochitest for this.

Comment 6

[alexander :surkov (:asurkov)]

Btw, if we're going to fix accChild then we need to introduce document accessible tree, you could rely on it instead of docshell treeitems.

Comment 7

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: WIP 2

Alexander, feel free to rework this patch if you like.

Comment 8

[David Bolter [:davidb] (NeedInfo me for attention)]

Especially if you want to base it off your recent landing.

Comment 9

[alexander :surkov (:asurkov)]

It sounds it might return root document for accessible inside of root document, I'm not sure it's expected. We need to check this.

Comment 10

[alexander :surkov (:asurkov)]

I don't think *ppv = static_cast<IAccessible*>(docAcc); may return null

Comment 11

[alexander :surkov (:asurkov)]

(In reply to comment #9)
> It sounds it might return root document for accessible inside of root document,

yes, it is, http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#2601

Comment 12

[David Bolter [:davidb] (NeedInfo me for attention)]

Oh right, it looks like it stops at the first root, so could be an iframe. Drat!

Comment 13

[David Bolter [:davidb] (NeedInfo me for attention)]

Ignore my last comment it should work in that respect. I'm asking for details from the vendor.

Comment 14

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #10)
> I don't think *ppv = static_cast<IAccessible*>(docAcc); may return null

Hi Alexander, I'm not sure what you mean here?

(re: comment 9)
I can bail for non content type.

Comment 15

[alexander :surkov (:asurkov)]

(In reply to comment #14)
> (In reply to comment #10)
> > I don't think *ppv = static_cast<IAccessible*>(docAcc); may return null
> 
> Hi Alexander, I'm not sure what you mean here?

that only means that static_cast<> doens't return null until it's operand is not null.

> 
> (re: comment 9)
> I can bail for non content type.

I think it's right but check with AT

Comment 16

[David Bolter [:davidb] (NeedInfo me for attention)]

Note to drivers: I marked this blocking because it will mitigate some painful fall out from bug 130078.

Comment 17

[David Bolter [:davidb] (NeedInfo me for attention)]

I'm set up with a test app to debug this now. It looks pretty good! Still need to iron things out with the vendor who requested this.

So far I've confirmed that the document returned is indeed what we want. Tested various pages including about:addons.

Comment 18

[David Bolter [:davidb] (NeedInfo me for attention)]

Status: currently one vendor is happy with this but still testing.
Note: I think we should probably rename the SID to: SID_IAccessibleRootDocument if the current implementation sticks.

Comment 19

[David Bolter [:davidb] (NeedInfo me for attention)]

(Or even SID_IAccessibleSameTypeRoot)

Comment 20

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: patch

OK this is the vendor approved content document only version. Chrome root not wanted for the purposes of virtual buffer.

Comment 21

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 476843 [details] [diff] [review]
patch

>+    // We want the root for content only
>+    PRInt32 itemType;
>+    root->GetItemType(&itemType);
>+    if (itemType != nsIDocShellTreeItem::typeContent)

Hmm I guess I could do:
if (!root->IsContent())

Comment 22

[David Bolter [:davidb] (NeedInfo me for attention)]

I mean if (!IsContent()), but I like what is in the patch actually.

Comment 23

[Marco Zehe (:MarcoZ)]

Comment on attachment 476843 [details] [diff] [review]
patch

Does GetItemType also return the content doc of the to most doc if dcalled from within an iframe? Just wanting to make sure we cover all cases.

Comment 24

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #23)
> Comment on attachment 476843 [details] [diff] [review]
> patch
> 
> Does GetItemType also return the content doc of the to most doc if dcalled from
> within an iframe?

Yes.

Comment 25

[David Bolter [:davidb] (NeedInfo me for attention)]

Marco actually I assumed you were asking about GetSameTypeRootTreeItem. GetSameTypeRootTreeItem returns the top most 'browser tab' document even if called from within an iframe. GetItemType returns type content for all my tests, including xul documents loaded in tabs.

Comment 26

[Marco Zehe (:MarcoZ)]

(In reply to comment #25)
> Marco actually I assumed you were asking about GetSameTypeRootTreeItem.
> GetSameTypeRootTreeItem returns the top most 'browser tab' document even if
> called from within an iframe. GetItemType returns type content for all my
> tests, including xul documents loaded in tabs.

OK, so I need to make sure something like Add-Ons Manager does actually not get rendered as a virtual document in JAWS or WE. It does not in NVDA, so we're OK there, at least with a regular nightly.

Comment 27

[Marco Zehe (:MarcoZ)]

Comment on attachment 476843 [details] [diff] [review]
patch

r=me, this looks correct. Note that I cannot actually test until I have builds of either JAWS or wE that utilize this mechanism.

Comment 28

[alexander :surkov (:asurkov)]

Comment on attachment 476843 [details] [diff] [review]
patch


>   static const GUID IID_SimpleDOMDeprecated = {0x0c539790,0x12e4,0x11cf,0xb6,0x61,0x00,0xaa,0x00,0x4c,0xd6,0xd8};
>+
>+  // Provide a special service ID for getting the content document accessible.
>+  static const GUID SID_IAccessibleContentDocument = {0xa5d8e1f3,0x3571,0x4d8f,0x95,0x21,0x07,0xed,0x28,0xfb,0x07,0x2e};

is that the name that you and AT are fine? Iirc we don't have a "content document accessible" term so it's not way to use to describe this uuid.

>+
>+    nsCOMPtr<nsIDocShellTreeItem> root;
>+    docShellTreeItem->GetSameTypeRootTreeItem(getter_AddRefs(root));
>+    if (!root)
>+      return E_UNEXPECTED;
>+
>+    // We want the root for content only
>+    PRInt32 itemType;
>+    root->GetItemType(&itemType);
>+    if (itemType != nsIDocShellTreeItem::typeContent)
>+      return E_NOINTERFACE;

I bet we must to have good commenting on this, especially if addons manager is treated differently by some reason.


I would like to have good description of IAccessibleContentDocument and nice description of this code magic with some clarification why it returns the things that we expect.


>+    nsDocAccessible *docAcc = nsAccUtils::GetDocAccessibleFor(root);

nit: fix styling

>+    if (!docAcc)
>+      return E_UNEXPECTED;
>+
>+    *ppv = static_cast<IAccessible*>(docAcc);
>+    if (*ppv == nsnull)
>+      return E_UNEXPECTED;

iirc, I said this early, static_cast<> can't return null pointer if docAcc is not null.

I'd like to look at new patch.

Comment 29

[David Bolter [:davidb] (NeedInfo me for attention)]

> >+  static const GUID SID_IAccessibleContentDocument = {0xa5d8e1f3,0x3571,0x4d8f,0x95,0x21,0x07,0xed,0x28,0xfb,0x07,0x2e};
> 
> is that the name that you and AT are fine?

Yes.

> Iirc we don't have a "content
> document accessible" term so it's not way to use to describe this uuid.

We don't have a special name for what we return at all. I'm welcome to ideas here. How about "the accessible for the main document inside the browser tab"?

> I bet we must to have good commenting on this, especially if addons manager is
> treated differently by some reason.

It works for about:addons - is that what you mean? But I agree more commenting wouldn't hurt.

> I would like to have good description of IAccessibleContentDocument and nice
> description of this code magic with some clarification why it returns the
> things that we expect.

Sure.

> 
> 
> >+    nsDocAccessible *docAcc = nsAccUtils::GetDocAccessibleFor(root);
> 
> nit: fix styling

Will do.

> 
> >+    if (!docAcc)
> >+      return E_UNEXPECTED;
> >+
> >+    *ppv = static_cast<IAccessible*>(docAcc);
> >+    if (*ppv == nsnull)
> >+      return E_UNEXPECTED;
> 
> iirc, I said this early, static_cast<> can't return null pointer if docAcc is
> not null.
> 

Ah I understand your point now. I will remove the redundant check.

Comment 30

[alexander :surkov (:asurkov)]

(In reply to comment #29)
> > >+  static const GUID SID_IAccessibleContentDocument = {0xa5d8e1f3,0x3571,0x4d8f,0x95,0x21,0x07,0xed,0x28,0xfb,0x07,0x2e};
> > 
> > is that the name that you and AT are fine?
> 
> Yes.
> 
> > Iirc we don't have a "content
> > document accessible" term so it's not way to use to describe this uuid.
> 
> We don't have a special name for what we return at all. I'm welcome to ideas
> here. How about "the accessible for the main document inside the browser tab"?

yes, but not exactly. If addons manager is handled especially then this description doesn't work or should be clarified.

> > I bet we must to have good commenting on this, especially if addons manager is
> > treated differently by some reason.
> 
> It works for about:addons - is that what you mean? But I agree more commenting
> wouldn't hurt.

right, chrome documents, we don't need virtual buffer for about:addons but we need it for error pages for example if I get right.

Comment 31

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: patch 1.1

The vendor prefers consistency here (and I agree with this). So with this patch we continue to always return the accessible that corresponds the root accessible for the browser tab document regardless of what content it has. The name of the SID is approved. I've added comments to describe what we are doing. I've removed the redundant null check.

Alexander, if you can r+ we should land this for beta7 if still possible.

Comment 32

[alexander :surkov (:asurkov)]

Comment on attachment 477946 [details] [diff] [review]
patch 1.1

>diff --git a/accessible/src/msaa/nsAccessNodeWrap.cpp b/accessible/src/msaa/nsAccessNodeWrap.cpp
>--- a/accessible/src/msaa/nsAccessNodeWrap.cpp
>+++ b/accessible/src/msaa/nsAccessNodeWrap.cpp
>@@ -129,23 +129,69 @@ STDMETHODIMP nsAccessNodeWrap::QueryInte
>    
>   (reinterpret_cast<IUnknown*>(*ppv))->AddRef(); 
>   return S_OK;
> }
> 
> STDMETHODIMP
> nsAccessNodeWrap::QueryService(REFGUID guidService, REFIID iid, void** ppv)
> {
>+  *ppv = nsnull;
>+
>   static const GUID IID_SimpleDOMDeprecated = {0x0c539790,0x12e4,0x11cf,0xb6,0x61,0x00,0xaa,0x00,0x4c,0xd6,0xd8};
>+
>+  // Provide a special service ID for getting the root accessible for the
>+  // browser tab that contains this accessible object. If this accessible object
>+  // is not inside a browser tab then the service fails with E_NOINTERFACE.
>+  // A use case for this is for screen readers that need to switch context or
>+  // 'virtual buffer' when focus moves from one browser tab area to another.

"root accessible" is reserved term, "root accessible for the browser tab" means document accessible for Firefox UI window that contains this browser tab. Just remove "root" word.

>+
>+    // Make sure this is a document.
>+    nsDocAccessible *docAcc = nsAccUtils::GetDocAccessibleFor(root);

again, please change styling, should be
nsDocAccessible* docAcc =

otherwise is good, r=me, please make sure to add tests in separate patch or bug.

Comment 33

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: patch to land

Thanks Alexander. Hanging the final patch here for when the tree opens up a bit more.

Comment 34

[alexander :surkov (:asurkov)]

the patch should have beta7 blocking status

Comment 35

[David Bolter [:davidb] (NeedInfo me for attention)]

Landed as http://hg.mozilla.org/mozilla-central/rev/abd177c94e23

Thanks all.