Last Comment Bug 767756 - try implementing ISimpleDOMNode with a tear off
: try implementing ISimpleDOMNode with a tear off
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All Windows 7
: -- normal (vote)
: mozilla20
Assigned To: alexander :surkov
:
Mentors:
Depends on: 837586
Blocks: 814569 816011 917973
  Show dependency treegraph
 
Reported: 2012-06-23 20:14 PDT by Trevor Saunders (:tbsaunde)
Modified: 2013-09-18 11:31 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (36.55 KB, patch)
2012-08-09 21:29 PDT, Andrew Quartey [:drexler]
no flags Details | Diff | Review
patch (38.54 KB, patch)
2012-08-10 23:09 PDT, Andrew Quartey [:drexler]
mzehe: feedback-
Details | Diff | Review
WIP (40.45 KB, patch)
2012-09-12 16:05 PDT, Andrew Quartey [:drexler]
no flags Details | Diff | Review
wip rebased with some fiddly com bits fixed up (40.15 KB, patch)
2012-10-24 14:15 PDT, Trevor Saunders (:tbsaunde)
no flags Details | Diff | Review
wip3 (39.67 KB, patch)
2012-11-08 07:04 PST, alexander :surkov
no flags Details | Diff | Review
patch (65.57 KB, patch)
2012-11-22 21:31 PST, alexander :surkov
tbsaunde+mozbugs: review+
mzehe: feedback+
Details | Diff | Review

Description Trevor Saunders (:tbsaunde) 2012-06-23 20:14:52 PDT
The end result here is that there will be a seperate class inheriting and implementing ISimpleDOMNode, and nsAccessNodeWrap won't inherit or implement it directly.
I don't have a great idea what to name this new class.
after creating the new class and making it inherit ISimpleDOMNode move the methods from nsAccessNodeWrap to it.
Then figure out what member variables you can reasonably add to the new class to implement all the methods, you might well be able to get by with just an nsINode* but you might want more I'm not absolutely sure.
Then modify the methods to deal with the new members.
rename MakeAccessNode() and update it so it deals with the fact an nsAccessNodeWrap isn't a ISimpleDOMNode.
I think the final thing you need to do here is modify the QueryInterface() on nsAccessNodeWrap so that it creates a new instance of the new class when asked for ISimpleDOMNode interface.
You also need to implement a QueryInterface() on the new class that calls back to the original object if the requested interface isn't ISimpleDOMNode.

there's a little bit of risk here we could break somebody, but since this should all follow COM rules we should be fine.  Note this should make each accessible a word smaller by removing a vtable, so its certainly worth trying.
Comment 1 alexander :surkov 2012-06-27 23:32:22 PDT
(In reply to Trevor Saunders (:tbsaunde) from comment #0)

> there's a little bit of risk here we could break somebody, but since this
> should all follow COM rules we should be fine.

we shouldn't break anybody since MakeAccessNode creates an instance everytime (if no accessible). I had a wip in my queue but was never about to finish it.

About naming: iirc I named it as sdnAccessible
Comment 2 Andrew Quartey [:drexler] 2012-08-09 21:29:58 PDT
Created attachment 650784 [details] [diff] [review]
patch

Locally tested patch. Sent to Try: https://tbpl.mozilla.org/?tree=Try&rev=fcecf5ab2c4c
Comment 3 Trevor Saunders (:tbsaunde) 2012-08-09 22:37:31 PDT
Comment on attachment 650784 [details] [diff] [review]
patch


this is a good start, first I have some general comments

I know they're artifacts, but it would be nice if you used AsContent() and IsContent() instead of QI.
another artifact, but try to avoid nsIDOM* interfaces when you can, they're slow.

>--- /dev/null
>+++ b/accessible/src/msaa/SDNAccessible.cpp
>@@ -0,0 +1,471 @@
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this

nit, add vim / emacs mode lines

>+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>+
>+#include "SDNAccessible.h"
>+#include "ISimpleDOMNode_i.c"
>+
>+#include "nsAccessNodeWrap.h"
>+#include "DocAccessibleWrap.h"
>+#include "AccessibleWrap.h"

DocAccessibleWrap inherits from AccessibleWrap so no reason to explicitly include it.

>+  
>+#include "nsAttrName.h"
>+#include "nsCoreUtils.h"

keep it with other a11y headers.

>+#include "nsIContent.h"
>+#include "nsIDOMHTMLElement.h"
>+#include "nsINode.h"
>+#include "nsIDOMCSSStyleDeclaration.h"
>+#include "nsServiceManagerUtils.h"
>+#include "nsWinUtils.h"

same

>+SDNAccessible::SDNAccessible(nsINode* aNode, DocAccessible* aDoc) :
>+  mNode(aNode), mDoc(aDoc)
>+{
>+}
>+
>+SDNAccessible::~SDNAccessible()
>+{
>+}
>+
>+bool
>+SDNAccessible::IsDocumentNode()
>+{
>+  return mNode && mNode->IsNodeOfType(nsINode::eDOCUMENT);
>+}

nit, define them inline they're simple

>+SDNAccessible::QueryInterface(REFIID aIID, void** ppv)
>+{
>+  *ppv = nullptr;
>+
>+  if (IID_IUnknown == aIID || IID_ISimpleDOMNode == aIID) {

in case of IUnknown you should return IUnknown* from accessible you are torn off from if there is one.  I'm not sure if anyone cares, but that's the COM rule.

>+    *ppv = static_cast<ISimpleDOMNode*>(this);

you shouldn't need the cast

>+  } else {
>+    return E_NOINTERFACE;
>+  }

nit, no braces

>+  (reinterpret_cast<IUnknown*>(*ppv))->AddRef();

in this case you might as well just do AddRef()

>+ISimpleDOMNode*
>+SDNAccessible::MakeSimpleDOMNode(nsINode* aNode)
>+{
>+  if (!aNode)
>+    return NULL;

why not nullptr?

>+  
>+  SDNAccessible* sdnAcc = NULL;
>+
>+  ISimpleDOMNode *sdnNode = NULL;
>+  Accessible* acc = mDoc->GetAccessible(aNode);
>+  if (acc) {
>+    IAccessible *msaaAccessible = nullptr;
>+    acc->GetNativeInterface((void**)&msaaAccessible); // addrefs
>+    msaaAccessible->QueryInterface(IID_ISimpleDOMNode, (void**)&sdnNode); // addrefs
>+    msaaAccessible->Release(); // Release IAccessible

you should be able to just static cast to AccessibleWrap and then QI for ISimpleDOMNode

or actually you could just call GetNode() and Document() and create the SDNNode yourself.

>+  }
>+  else {

} else {

>+    nsCOMPtr<nsIContent> content(do_QueryInterface(aNode));
>+    if (!content) {
>+      NS_NOTREACHED("The node is a document which is not accessible!");
>+      return NULL;
>+    }
>+
>+    sdnAcc = new SDNAccessible(aNode, mDoc); 
>+    if (!sdnAcc)
>+      return NULL;

new is infalable so remove the check, also declare the variable at first use.

>+    sdnNode = static_cast<ISimpleDOMNode*>(sdnAcc);

nit, you shouldn't need the cast

>+SDNAccessible::get_language(BSTR __RPC_FAR *aLanguage)
>+{
>+  A11Y_TRYBLOCK_BEGIN
>+
>+  if (mDoc) {
>+    *aLanguage = NULL;
>+
>+    nsCOMPtr<nsIContent> content(do_QueryInterface(mNode));
>+    nsAccessNode accNode(content, mDoc);
>+    nsAutoString language;
>+    accNode.Language(language);  

it might be safe now, but putting ref counted objects on the stack isn't a good idea.  I'm not really sure what the right way to fix that stuff is, but creating a temporary accessNode on the heap is certainly fine for now.

>+SDNAccessible::get_localInterface(void __RPC_FAR *__RPC_FAR *localInterface)
>+{
>+  A11Y_TRYBLOCK_BEGIN
>+
>+  *localInterface = static_cast<ISimpleDOMNode*>(this);

at this point it almost might as well be null, but I'd say not upcasting may be better here, Alex do you have an opinion?

>+  NS_ADDREF_THIS();

we don't usually  use xpcom things with mscom things just because its weird, and I don't see a reason not to do a plain AddRef() here.

>+#include "AccessibleWrap.h"

what do you use it for?

>+class SDNAccessible : public ISimpleDOMNode
>+{
>+public:
>+  SDNAccessible(nsINode* aNode, DocAccessible* aDoc);
>+
>+  virtual ~SDNAccessible();

make the class final and the destructor non virtual.

>+  DocAccessible* mDoc;
>+  nsINode* mNode;

you need to hold strong refs since you don't control the life cycle of this object third parties do.

> STDMETHODIMP nsAccessNodeWrap::QueryInterface(REFIID iid, void** ppv)
> {
>   *ppv = nullptr;
>+  SDNAccessible* sdnAcc = NULL;

please declaare stuff where you use it.

>   if (IID_IUnknown == iid) {
>-    *ppv = static_cast<ISimpleDOMNode*>(this);
>+    sdnAcc = new SDNAccessible(GetNode(), mDoc);

you should just static cast this to the first interface to resolve ambiguity about which IUnknow to upcast to.
Comment 4 alexander :surkov 2012-08-09 22:42:54 PDT
Comment on attachment 650784 [details] [diff] [review]
patch

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

please prepare try server build and ask feedback from Marco Zehe (let me know if you need a help)

general notes:
1) fix styling (no tabs, two spaces indent, no spaces in the end of line and on empty lines), make your editor to fix it or fix them manually (splitter review shows them).
2) SDNAccessible -> sdnAccessible, keep it under mozilla::a11y namespace
3) move new files into widnows/sdn folder
4) sdnAccesible::QueryInterface should get back the IAccessible if requested
Comment 5 alexander :surkov 2012-08-09 22:45:16 PDT
Comment on attachment 650784 [details] [diff] [review]
patch

eventually I've changed Trevor's r+ to r?

I think we'd need another patch with comments addressed so canceling requests.
Comment 6 Andrew Quartey [:drexler] 2012-08-10 23:09:02 PDT
Created attachment 651098 [details] [diff] [review]
patch

Mainly fixed the nits and styling issues in addition to extra null-checks alongside logic changes. Also, i had compilation issues using strong references for the DocAccessible and nsINode members so fthem as it is whilst i await feedback on changes so far.
Comment 7 Andrew Quartey [:drexler] 2012-08-11 07:03:05 PDT
Looks OK on try: https://tbpl.mozilla.org/?tree=Try&rev=a2dbbd179b6b.
Comment 8 Andrew Quartey [:drexler] 2012-08-16 22:43:16 PDT
feedback: ping
Comment 9 Marco Zehe (:MarcoZ) 2012-08-17 08:59:41 PDT
Comment on attachment 651098 [details] [diff] [review]
patch

JAWS 13.0 does not see any content in its virtual buffer with this content. The virtual cursor is active, and when landing on a form control, it automatically switches to forms mode, but it does not get any content.
Comment 10 alexander :surkov 2012-08-19 22:45:21 PDT
Did the try server build contains QueryInterface fix?
Comment 11 Trevor Saunders (:tbsaunde) 2012-09-09 15:16:09 PDT
Andrew status?  We should get this working soon, I can make some time to work on it if you like.
Comment 12 Andrew Quartey [:drexler] 2012-09-09 15:23:21 PDT
I halted to take care of some outstanding gfx and dom bugs i had but it's top priority now. Currently, i made the changes you suggested regarding the QI but still cant get neither JAWS nor NVDA to work with it. The latter just closes(crashes?) the browser. Focusing solely on this, i think we should get it done this week. Also JAWS aside is there any other alternative since the demo version i use is proving difficult.
Comment 13 Trevor Saunders (:tbsaunde) 2012-09-09 16:25:41 PDT
(In reply to Andrew Quartey [:drexler] from comment #12)
> I halted to take care of some outstanding gfx and dom bugs i had but it's
> top priority now. Currently, i made the changes you suggested regarding the
> QI but still cant get neither JAWS nor NVDA to work with it. The latter just
> closes(crashes?) the browser. Focusing solely on this, i think we should get

that's rather odd, afaik nvda doesn't use ISimpleDOM at all, so this should be pretty unlikely to effect them.

If you post the patch I can take a look and see if I can find something.

> it done this week. Also JAWS aside is there any other alternative since the
> demo version i use is proving difficult.

I think only jaws and window eyes use ISimpleDOM stuff.
Comment 14 Andrew Quartey [:drexler] 2012-09-12 16:05:23 PDT
Created attachment 660602 [details] [diff] [review]
WIP

This passes my local tests but when i try to use either Window-Eyes or JAWS, the browser is abruptly closed. I've tried locating the 'entry point' of these readers so that i can try debug and see what's going on but to no avail. Suggestions appreciated.
Comment 15 Trevor Saunders (:tbsaunde) 2012-09-12 20:53:32 PDT
Comment on attachment 660602 [details] [diff] [review]
WIP

>+sdnAccessible::QueryInterface(REFIID aIID, void** ppv)
>+{
>+  *ppv = nullptr;
>+
>+  if (IID_IUnknown == aIID) {
>+    *ppv = static_cast<IUnknown*>(this);
>+    AddRef();
>+    return S_OK;
>+  }

this breaks the com rule that when you QueryInterface() from IUknown to IFoo and then QueryInterface() on the result back to IUnknown the two IUknonwn* you have should be the same.

>+
>+  if (IID_ISimpleDOMNode == aIID) {
>+    *ppv = this;
>+    AddRef();
>+    return S_OK;
>+  }
>+
>+  if (IID_IAccessible == aIID && mDoc) {
>+    AccessibleWrap* acc = static_cast<AccessibleWrap*>(mDoc);
>+    return acc->QueryInterface(aIID, ppv);
>+  }

that's just wrong, the accessible you want is contained the mDoc DocAccessible, but it could very well be a child.

I think QueryInterface() here should look like this

if (aIid == IID_ISimpleDOMNode) {
  *aOutInterface = this;
  AddRef();
  return S_OK;
}

Accessible* acc = mDoc->GetAccessible(mNode);
if (acc)
  return acc->QueryInterface(aIid, aOutInterface);

if (aIid == IID_IUnknown) {
  *aOutInterface = this;
  AddRef();
  return S_OK;
}

return E_NOINTERFACE;
}

I think that is broken for the case that this SDNAccessible was gotten by crawling the tree of SDNAccessibles, and just happens to be for a node that has an accessible.  However To fix that we can add a bool field to SDNAccessible, to say if the accessible was created by an accessible.  However I suspect nobody will notice, this little querk.
Comment 16 Trevor Saunders (:tbsaunde) 2012-10-24 14:15:43 PDT
Created attachment 674825 [details] [diff] [review]
wip rebased with some fiddly com bits fixed up

there are a couple little places this changes behaviour, but should be very largely the same behaviour as before.

 Some of these seem more like bug fixes than anything else to me, but I think we can revert them all to the previous behaviour if we find they break someone.

- if MakeAccessNode() is called with a document node we now make a sdn accessible for the doc accessible with mNode == document node so things that used to use mContent which was the body will now operate on the document node itself, but that seems fine because we should also create a seperate sdn accessible for the body
- we can have two sdn accessibles for the same node, which means sdn1 unique id != sdn2 unique id does not imply node1 != node2 We could solve that by using the address of the accessible we are torn off from or the dom node insstead of ourselves as the id.

In any case it would be nice if someone on windows could make sure this builds that would be nice, I think you may have to rebase some of the node api usage GetNodeParent() became GetParentNode() and maybe some other similar changes.
Comment 17 alexander :surkov 2012-10-24 23:53:12 PDT
Trev, did you file a try build for Marco's testing?
Comment 18 Trevor Saunders (:tbsaunde) 2012-10-25 12:21:12 PDT
(In reply to alexander :surkov from comment #17)
> Trev, did you file a try build for Marco's testing?

no, see my comment, I was hoping someone would feel like making sure it builds locally, but I guess I can play wake a mole with try or boot my windows vm if no victim shows up.
Comment 19 alexander :surkov 2012-10-26 05:28:16 PDT
Ok, I'll finish it
Comment 20 alexander :surkov 2012-11-08 07:04:20 PST
Created attachment 679659 [details] [diff] [review]
wip3

it's built but it doesn't work
Comment 21 Trevor Saunders (:tbsaunde) 2012-11-18 01:43:19 PST
(In reply to alexander :surkov from comment #20)
> Created attachment 679659 [details] [diff] [review]
> wip3
> 
> it's built but it doesn't work

I wonder if the problem is you can't get IServiceProvider from SDNAccessible if so we could make that a tear off two.
Comment 22 alexander :surkov 2012-11-18 22:38:06 PST
(In reply to Trevor Saunders (:tbsaunde) from comment #21)

> > it's built but it doesn't work
> 
> I wonder if the problem is you can't get IServiceProvider from SDNAccessible
> if so we could make that a tear off two.

I might be wrong but iirc new code is equivalent to existing one. But I think that build was broken at all due to some reason, I need to give it an another spin.
Comment 23 Trevor Saunders (:tbsaunde) 2012-11-18 22:54:01 PST
(In reply to alexander :surkov from comment #22)
> (In reply to Trevor Saunders (:tbsaunde) from comment #21)
> 
> > > it's built but it doesn't work
> > 
> > I wonder if the problem is you can't get IServiceProvider from SDNAccessible
> > if so we could make that a tear off two.
> 
> I might be wrong but iirc new code is equivalent to existing one. But I

its crazy, but seems so.

> think that build was broken at all due to some reason, I need to give it an
> another spin.

ok, would be pretty nice if we could get this figured out, there are some other things I want to make faster that depend on this because they want to move things from nsAccessNode to Accessible
Comment 24 alexander :surkov 2012-11-22 07:11:33 PST
here's a try build, Marco, can you try - http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/surkov.alexander@gmail.com-e39393f5887d/
Comment 25 alexander :surkov 2012-11-22 21:31:42 PST
Created attachment 684594 [details] [diff] [review]
patch

try server build: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/surkov.alexander@gmail.com-33496b06ac06

Marco, JAWS + WE please
Comment 26 alexander :surkov 2012-11-22 22:08:11 PST
(Don't forget to mark bug 750025 fixed when the patch is landed)
Comment 27 Marco Zehe (:MarcoZ) 2012-11-26 01:28:12 PST
Comment on attachment 684594 [details] [diff] [review]
patch

The latest try build works with both JAWS and Window-Eyes, and also works fine with NVDA still. No crashes encountered, and no weirdness on the pages I tested with either of the commercial screen readers. This looks good.
Comment 28 Trevor Saunders (:tbsaunde) 2012-11-26 18:53:24 PST
Comment on attachment 684594 [details] [diff] [review]
patch

>+  *aNode = NULL;

nulllptr? here and other places

>+  if (!aLanguage)
>+    return E_INVALIDARG;
>   *aLanguage = NULL;

nit, blank line here and similar places

>+++ b/accessible/src/windows/sdn/sdnAccessible.h
>@@ -1,163 +1,116 @@
> /* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

nit, add a vim line while your here.

>+#include "ISimpleDOMNode.h"
>+#include "AccessibleWrap.h"

that's for the macros? that's another reason it'd be nice to put them in there own header.

>+class sdnAccessible MOZ_FINAL : public ISimpleDOMNode

we may not want to block inheritance if we want to seperate ISimpleDOMDocument / Text as well, but removing the final should be easy enough.

>+public:
>+  sdnAccessible(nsINode* aNode) : mNode(aNode) { }
>+  ~sdnAccessible() { }

you could remove this and let the compiler provide it as the default if you like.
Comment 29 alexander :surkov 2012-11-26 19:11:50 PST
(In reply to Trevor Saunders (:tbsaunde) from comment #28)
> >+  *aNode = NULL;
> 
> nulllptr? here and other places

these are COM stuff, thus we use COM style here.

> >+  if (!aLanguage)
> >+    return E_INVALIDARG;
> >   *aLanguage = NULL;
> 
> nit, blank line here and similar places

that's like a stamp/template, a thing we should have but don't want to pay any attention, that's why I keep these close to each other to make them treated as one block.

> >+++ b/accessible/src/windows/sdn/sdnAccessible.h
> >@@ -1,163 +1,116 @@
> > /* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> 
> nit, add a vim line while your here.

ok

> >+#include "ISimpleDOMNode.h"
> >+#include "AccessibleWrap.h"
> 
> that's for the macros? that's another reason it'd be nice to put them in
> there own header.

file a bug?

> >+class sdnAccessible MOZ_FINAL : public ISimpleDOMNode
> 
> we may not want to block inheritance if we want to seperate
> ISimpleDOMDocument / Text as well, but removing the final should be easy
> enough.

true, I think I just didn't change that from your patch. I'll remove it.

> >+public:
> >+  sdnAccessible(nsINode* aNode) : mNode(aNode) { }
> >+  ~sdnAccessible() { }
> 
> you could remove this and let the compiler provide it as the default if you
> like.

I don't mind to keep it explicitly.
Comment 31 Ryan VanderMeulen [:RyanVM] 2012-11-27 16:28:30 PST
https://hg.mozilla.org/mozilla-central/rev/19eb532fad5a

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