741703 - stop using QueryInterface in CAccessibleHypertext

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

same as bug 736252:

1) do static_cast<nsAccessibleHypertext*>(this) instead do_QueryObject(this)
2) rename CAccessibleHypertext to ia2AccessibleHypertext

Comment 1

[Mark Capella [:capella]]

Attachment: Patch (v1)

First draft patch ... note that there is one do_QueryObject left in place, for a winAccessNode ... I think this is planned to be covered in bug648267 so I left this portion asis.

Comment 2

[alexander :surkov (:asurkov)]

Comment on attachment 613237 [details] [diff] [review]
Patch (v1)

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

you don't need CAccessibleHypertext::QueryInterface. I'll fix it and merge with patch from bug 539683 and land it.

Comment 3

[alexander :surkov (:asurkov)]

Comment on attachment 613237 [details] [diff] [review]
Patch (v1)

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

::: accessible/src/msaa/CAccessibleHypertext.cpp
@@ +51,4 @@
>  {
>    *ppv = NULL;
>    if (IID_IAccessibleHypertext == iid) {
> +    nsHyperTextAccessibleWrap* hyperAcc = static_cast<nsHyperTextAccessibleWrap*>(this);

it's a problem because nsIAccessibleHyperText can be not implemented by nsHyperTextAccessibleWrap. Trevor, ideas?

Comment 4

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #3)
> Comment on attachment 613237 [details] [diff] [review]
> Patch (v1)
> 
> Review of attachment 613237 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/src/msaa/CAccessibleHypertext.cpp
> @@ +51,4 @@
> >  {
> >    *ppv = NULL;
> >    if (IID_IAccessibleHypertext == iid) {
> > +    nsHyperTextAccessibleWrap* hyperAcc = static_cast<nsHyperTextAccessibleWrap*>(this);
> 
> it's a problem because nsIAccessibleHyperText can be not implemented by
> nsHyperTextAccessibleWrap. Trevor, ideas?

i'M KIND OF DISTRACTED SO NOT SURE i FOLLOW YOUR CONCERN, BUT IS THIS THE thing were nsHyperTextAccessible doesn't always QI to nsIAccessibleHyperText depending on role?

Comment 5

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #4)

> i'M KIND OF DISTRACTED SO NOT SURE i FOLLOW YOUR CONCERN, BUT IS THIS THE
> thing were nsHyperTextAccessible doesn't always QI to nsIAccessibleHyperText
> depending on role?

yes, http://mxr.mozilla.org/mozilla-central/source/accessible/src/html/nsHyperTextAccessible.cpp#102

Comment 6

[alexander :surkov (:asurkov)]

after talk on irc we can do:

nsARIAMap.h
namespace aria {
  bool IsTextRole(nsRoleMapEntry* aRoleMap) {
    http://mxr.mozilla.org/mozilla-central/source/accessible/src/html/nsHyperTextAccessible.cpp#102
  }
}

and then use aria::IsTextRole(mRoleMapEntry) for XPCOM and IA2 QueryInterface implementation.

Comment 7

[Mark Capella [:capella]]

Attachment: Patch (v2) very rough

I don't follow why the original patch had a problem, but trying to switch to your new roadmap.

I come up with the attached, where I've created the new function in nsAriaMap.h as I think you were looking for. Wrapping it in just aria was producing compile errors, so I wrapped it in mozilla / a11y / aria, and now get odd link errors.  So >yuck< either way.

Do we still plan to remove the function ia2AccessibleHypertext::QueryInterface as mentioned in comment#2? Or will we keep it but modify it to use the call to aria::IsTextRole(mRoleMapEntry)? I ask because I had a version of this I was playing with but couldn't compile/build due to failure to recognize the mRoleMapEntry var that I was trying to pass.

Finally, for my informatino, is XPCOM and IA2 QueryInterface the same thing? ie: Stuff in the msaa folder, versus mac and atk interfaces?

Comment 8

[alexander :surkov (:asurkov)]

Comment on attachment 613927 [details] [diff] [review]
Patch (v2) very rough

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

please fix nsHyperTextAccessible::QueryInterface to use new function

::: accessible/src/base/nsARIAMap.h
@@ +266,5 @@
> + * Determine if accessible is valid as a text role.
> + */
> +bool IsTextRole(nsRoleMapEntry* aRoleMap)
> +{
> +  if (aRoleMap &&

do namespace allows inlined functions?

::: accessible/src/msaa/CAccessibleHypertext.cpp
@@ +51,5 @@
>  {
>    *ppv = NULL;
>    if (IID_IAccessibleHypertext == iid) {
> +    nsHyperTextAccessibleWrap* hyperAcc = static_cast<nsHyperTextAccessibleWrap*>(this);
> +    if (hyperAcc->IsDefunct())

interface set shouldn't depend on object state so IsDefunct check

you should do aria::IsTextRole(acc->ARIARoleMap());

pls add ARIARoleMap method to nsAccessible

@@ +65,5 @@
>  
>  // IAccessibleHypertext
>  
>  STDMETHODIMP
> +ia2AccessibleHypertext::get_nHyperlinks(long *aHyperlinkCount)

could you pls fix type* name, here and below

Comment 9

[alexander :surkov (:asurkov)]

(In reply to Mark Capella [:capella] from comment #7)
> I don't follow why the original patch had a problem, but trying to switch to
> your new roadmap.

we shouldn't expose text interfaces on things like <div role="slider">

Comment 10

[Mark Capella [:capella]]

Attachment: Patch (v3)

Well, Ii got this far ... it builds and tests once again. I did get stuck trying to get it built into nsariamap.h and the aria namespace.

Comment 11

[alexander :surkov (:asurkov)]

Comment on attachment 614205 [details] [diff] [review]
Patch (v3)

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

::: accessible/src/base/nsAccessible.h
@@ +201,5 @@
> +   */
> +  inline nsRoleMapEntry* ARIARoleMap() const
> +  {
> +    return mRoleMapEntry;
> +  }

nsRoleMapEntry* ARIARoleMap() const
  { return mRoleMapEntry; }

::: accessible/src/msaa/CAccessibleHypertext.cpp
@@ +53,5 @@
>  {
>    *ppv = NULL;
>    if (IID_IAccessibleHypertext == iid) {
> +    nsHyperTextAccessibleWrap* hyperAcc = static_cast<nsHyperTextAccessibleWrap*>(this);
> +    if (hyperAcc->IsDefunct() || !IsTextRole(hyperAcc->ARIARoleMap()))

again you don't need IsDefunct check, cast it to nsAccessibleWrap

@@ +134,5 @@
> + * Determine if accessible is valid as a text role.
> + */
> +bool 
> +ia2AccessibleHypertext::IsTextRole(nsRoleMapEntry* aRoleMap)
> +{

if you put it as part of ia2AccessibleHypertext then you don't need to expose ARIARoleMap. You can just to cast to nsAccessibleWrap and use mRoleMapEntry

Also you should reuse this method for nsHypertextAccessible::QueryInterface (we should avoid to dupe the logic).

If due to some reason you don't want to put it into nsARIAMap then you can keep it on nsHyperTextAccessible so that you can use it in ia2AccessibleHypertext and in nsHyperTextAccessible::QueryInterface

::: accessible/src/msaa/CAccessibleHypertext.h
@@ +46,5 @@
>  
>  #include "CAccessibleText.h"
>  #include "AccessibleHypertext.h"
>  
> +class ia2AccessibleHypertext: public CAccessibleText,

nit: space before ':'

Comment 12

[Mark Capella [:capella]]

Attachment: Patch (v4)

Comment 13

[alexander :surkov (:asurkov)]

Comment on attachment 614295 [details] [diff] [review]
Patch (v4)

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

::: accessible/src/html/nsHyperTextAccessible.cpp
@@ +91,5 @@
>      NS_ADDREF_THIS();
>      return NS_OK;
>    }
>  
> +  // ARIA roles that these interfaces are not appropriate for

nit: missed dot

@@ +2392,5 @@
>    return NS_OK;
>  }
> +
> +/**
> + * Determine if accessible is valid as a text role.

you don't need a comment here (keep in header)

::: accessible/src/html/nsHyperTextAccessible.h
@@ +201,5 @@
>                                        nsIDOMNode **aEndNode,
>                                        PRInt32 *aEndOffset);
>  
> +  /**
> +   * Determine if accessible is valid as a text role.

perhaps: Return true if the used ARIA role (if any) allows the hypertext accessible to expose text interfaces.

::: accessible/src/msaa/CAccessibleHypertext.cpp
@@ +56,1 @@
>        return E_NOINTERFACE;

I think I'd prefer

if (hypertext->IsTextRole()) {
  *ppv = static_cast<>
  return S_OK;
}

return E_NOINTERFACE;

Comment 14

[alexander :surkov (:asurkov)]

Mark, you don't need new patch, I'll fix those nits before landing.

Comment 15

[Mark Capella [:capella]]

Attachment: Patch (v5)

Nits addressed ... build clean, test clean.

Comment 16

[alexander :surkov (:asurkov)]

(In reply to Mark Capella [:capella] from comment #15)
> Created attachment 614824 [details] [diff] [review]
> Patch (v5)
> 
> Nits addressed ... build clean, test clean.

ok, thanks, I pushed try server build. I'll land it tomorrow.

Comment 17

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2411a6a8e38c

Comment 18

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/2411a6a8e38c