865588 - tear off ISimpleDOMText

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 1

[Trevor Saunders (:tbsaunde)]

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

>+++ b/accessible/src/windows/msaa/ServiceProvider.cpp
>@@ -18,19 +18,18 @@
> 
> #include "ISimpleDOMNode_i.c"
> 
> namespace mozilla {
> namespace a11y {
> 
> IMPL_IUNKNOWN_QUERY_HEAD(ServiceProvider)
>   IMPL_IUNKNOWN_QUERY_IFACE(IServiceProvider)
>-  return mAccessible->QueryInterface(aIID, aInstancePtr);
>-A11Y_TRYBLOCK_END
>-  }
>+IMPL_IUNKNOWN_QUERY_TAIL_AGGREGATED(mAccessible)

isn't it a bug that this returns the accessibles IUnknown not the one for IServiceProvider?

>+IMPL_IUNKNOWN_QUERY_HEAD(TextLeafAccessibleWrap)
>+  if (aIID == IID_ISimpleDOMText) {
>     statistics::ISimpleDOMUsed();
>-    *ppv = static_cast<ISimpleDOMText*>(this);
>-  } else {
>-    return AccessibleWrap::QueryInterface(iid, ppv);
>+    *aInstancePtr = static_cast<ISimpleDOMText*>(new sdnTextAccessible(this));

I don't think you need the cast since it only inherits from ISimpleDOMText

>+
>+/**
>+ * Implement ISimpleDOMText as a tear off.

funny wording for the description of a class maybe "Wrap TextLeafAccessible so we can expose ISimpleDOMText as a native  interface with a teer off" or something?

>+  nsCOMPtr<nsIDOMNode> DOMNode(do_QueryInterface(mAccessible->GetContent()));
>   DOMNode->GetNodeValue(nodeValue);
>   if (nodeValue.IsEmpty())I think there's a version of that method on nsINode now so it would be nice to file a bug or just get rid of the nsIDOMNode

>copy to accessible/src/windows/sdn/sdnTextAccessible.h
>--- a/accessible/src/windows/msaa/TextLeafAccessibleWrap.h
>+++ b/accessible/src/windows/sdn/sdnTextAccessible.h
>-#include "TextLeafAccessible.h"
>+//#include "TextLeafAccessible.h"
> #include "ISimpleDOMText.h"
>-#include "nsRect.h"
>+//#include "nsRect.h"

any reason for it?

>+#include "AccessibleWrap.h"

I guess you need that because of inline ctor / dtor and refptr? :(

>+  sdnTextAccessible(AccessibleWrap* aAccessible) : mAccessible(aAccessible) {};
>+  virtual ~sdnTextAccessible() {}

class is final so no need for it to be virtual

>+  nsRefPtr<AccessibleWrap> mAccessible;

I'm not really a fan, but holding onto content is probably hard, and its not really any worse than what we had.

Comment 2

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #1)
 
> isn't it a bug that this returns the accessibles IUnknown not the one for
> IServiceProvider?

I checked and it goes with tear off implementation in ATL

> >-    return AccessibleWrap::QueryInterface(iid, ppv);
> >+    *aInstancePtr = static_cast<ISimpleDOMText*>(new sdnTextAccessible(this));
> 
> I don't think you need the cast since it only inherits from ISimpleDOMText

vt address is the same with the object address? what about IUnknown?

I added it to stay consistent and be on safe side.

> >+
> >+/**
> >+ * Implement ISimpleDOMText as a tear off.
> 
> funny wording for the description of a class maybe "Wrap TextLeafAccessible
> so we can expose ISimpleDOMText as a native  interface with a teer off" or
> something?

ok

> >+  nsCOMPtr<nsIDOMNode> DOMNode(do_QueryInterface(mAccessible->GetContent()));
> >   DOMNode->GetNodeValue(nodeValue);
> >   if (nodeValue.IsEmpty())I think there's a version of that method on nsINode now so it would be nice to file a bug or just get rid of the nsIDOMNode

a bug should be ok

> >+#include "AccessibleWrap.h"
> 
> I guess you need that because of inline ctor / dtor and refptr? :(

I guess so but since this header is included only in one file then maybe it doesn't matter.
> 
> >+  nsRefPtr<AccessibleWrap> mAccessible;
> 
> I'm not really a fan, but holding onto content is probably hard, and its not
> really any worse than what we had.

same as in IServiceProvider, I think to have a bug 678429 for this

Comment 3

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/96e445cf42fb

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/96e445cf42fb