589399 - add pseudo HyperLinkAccessible interface

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Attachment: patch

see bug 574336 comment #5.

Side note: StartOffset/EndOffset implementation much quicker than existing one.

Comment 1

[alexander :surkov (:asurkov)]

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-1759beb176a5

Comment 2

[alexander :surkov (:asurkov)]

Attachment: patch2

fixed linux build errors

Comment 3

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

I'm wondering if there is a better C++ pattern we can use here.

Comment 4

[alexander :surkov (:asurkov)]

(In reply to comment #3)
> I'm wondering if there is a better C++ pattern we can use here.

what do you keep in mind?

Comment 5

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

Comment on attachment 468102 [details] [diff] [review]
patch2

Neil, what do you think?

Comment 6

[[e-mail]]

Comment on attachment 468102 [details] [diff] [review]
patch2

I don't think those reinterpret_casts are safe; I'd stick to do_QueryObject.

Or was there something else that you wanted my feedback on?

Comment 7

[alexander :surkov (:asurkov)]

(In reply to comment #6)
> Comment on attachment 468102 [details] [diff] [review]
> patch2
> 
> I don't think those reinterpret_casts are safe; I'd stick to do_QueryObject.

you're right. I've fixed this already.

> Or was there something else that you wanted my feedback on?

I think the question was if proposed architecture is good and there's not better and also easy solution.

Comment 8

[alexander :surkov (:asurkov)]

Attachment: patch3

Comment 9

[[e-mail]]

Comment on attachment 469756 [details] [diff] [review]
patch3

Do you mean better than adding lots of virtual methods to your base class so that you don't have to call QueryInterface? The only thing better than that is if only one of your subclasses needs to implement the virtual methods; in that case you call one virtual method AsHyperlink() which returns the subclass or null if the accessible isn't a hyperlink, and then the virtual methods become normal methods on the subclass. For example:

XPCOM code:
class nsBar: public nsFoo, nsIBar {
  NS_DECL_NSIBAR
};
nsCOMPtr<nsIBar> bar(do_QueryInterface(foo));
if (bar) {
  rv = bar->Bar(this);
  rv = bar->Baz(this);
}

DeCOM code:
class nsFoo {
  virtual bool IsBar() { return false; }
  virtual void Bar(nsBaz*) { NS_NOTREACHED };
  virtual void Baz(nsBaz*) { NS_NOTREACHED };
};
class nsBar: public nsFoo {
  virtual bool IsBar() { return true; }
  virtual void Bar(nsBaz*);
  virtual void Baz(nsBaz*);
};
if (foo->IsBar()) {
  foo->Bar(this);
  foo->Baz(this);
}

DeVirtual code:
class nsBar;
class nsFoo {
  virtual nsBar* AsBar() { return nsnull; }
};
class nsBar: public nsFoo {
  virtual nsBar* AsBar() { return this; }
  void Bar(nsBaz*);
  void Baz(nsBaz*);
};
nsBar *bar = foo->AsBar();
if (bar) {
  bar->Bar(this);
  bar->Baz(this);
}

Having said that, I don't think that this applies in this particular case!

Comment 10

[alexander :surkov (:asurkov)]

Yes, some subclasses need to provide own implementation of these virtual methods. Other alternative could be a new abstact class and inherit the base class from it, it's similar to xpcom version. But while we're working with base class directly (nsAccessible) it sounds that it isn't much nicer than the chosen approach since all these methods are "visible" when I have pointer to base class.

Comment 11

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

Comment on attachment 469756 [details] [diff] [review]
patch3

> // readonly attribute boolean nsIAccessibleHyperLink::valid
> NS_IMETHODIMP
> nsAccessible::GetValid(PRBool *aValid)
> {
>   NS_ENSURE_ARG_POINTER(aValid);
>-  PRUint32 state = nsAccUtils::State(this);
>-  *aValid = (0 == (state & nsIAccessibleStates::STATE_INVALID));
>-  // XXX In order to implement this we would need to follow every link
>-  // Perhaps we can get information about invalid links from the cache
>-  // In the mean time authors can use role="link" aria-invalid="true"
>-  // to force it for links they internally know to be invalid
>+
>+  if (IsDefunct())
>+    return NS_ERROR_FAILURE;
>+
>+  *aValid = IsValid();
>   return NS_OK;
> }

We might want to set *aValid = PR_FALSE before the IsDefunct check.

Comment 12

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

Comment on attachment 469756 [details] [diff] [review]
patch3

Will the vtable for all our nsAccessible's now be larger? I'm unsure of the memory impact here.

Comment 13

[alexander :surkov (:asurkov)]

(In reply to comment #11)
> > NS_IMETHODIMP
> > nsAccessible::GetValid(PRBool *aValid)

> We might want to set *aValid = PR_FALSE before the IsDefunct check.

right, thank you for the catch.

(In reply to comment #12)
> Comment on attachment 469756 [details] [diff] [review]
> patch3
> 
> Will the vtable for all our nsAccessible's now be larger? I'm unsure of the
> memory impact here.

yes but not per instance

Comment 14

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

Right the vtable is shared by the virtual classes, okay.

OK still going...

Comment 15

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

Comment on attachment 469756 [details] [diff] [review]
patch3

r+a=me (sorry again for the delay - I was on the fence about this but I think it may actually also help us morph accessibles on the fly if we need to do that in ARIA 2.0)

Let's not land this until after beta5 is tagged. (Which should be later today).

Comment 16

[alexander :surkov (:asurkov)]

(In reply to comment #15)
> Comment on attachment 469756 [details] [diff] [review]
> patch3
> 
> r+a=me (sorry again for the delay - I was on the fence about this but I think
> it may actually also help us morph accessibles on the fly if we need to do that
> in ARIA 2.0)

It might be a problem for COM (MSAA and IA2)

> Let's not land this until after beta5 is tagged. (Which should be later today).

Sure, but we can't break it more than it is :)

Comment 17

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

Darn COM identity rules :(

Comment 18

[alexander :surkov (:asurkov)]

landed on 2.0 - http://hg.mozilla.org/mozilla-central/rev/82ed1b11d8f1