1189108 - LinkableAccessible actionable parent and action count return wrong result when event listeners are changed

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Lorien Hu (:lsocks)]

mActionAcc, mIsLink, and mIsonclick are set only when a LinkAccessible is created and never updated. Currently we only recreate the subtree when a listener is removed/added by setting an onclick attribute. If an accessible gains a click listener in another way, children LinkableAccessibles won't have the right values for these fields and ActionCount() plus any functions that use mActionAcc will be incorrect.

Comment 1

[Lorien Hu (:lsocks)]

Attachment: Don't cache action info

Comment 2

[Lorien Hu (:lsocks)]

Attachment: Don't cache action info

don't recreate accessibles on onclick attribute change as event listener changes will handle it

Comment 3

[Trevor Saunders (:tbsaunde)]

Comment on attachment 8641223 [details] [diff] [review]
Don't cache action info

># HG changeset patch
># User Lorien Hu <[e-mail]>
># Date 1438282119 14400
>#      Thu Jul 30 14:48:39 2015 -0400
># Node ID 5a5454042e1b67cd8341e657213544065e57533d
># Parent  c883a1dc4e6efd12e3345f9776526b3cfee54383
>Bug 1189108 - Walk up tree to get LinkableAccessible actions instead of caching

it'd be nice to have a test for the event handler case this fixes

>+void
>+LinkableAccessible::ActionWalk(Accessible** aActionAcc, bool* aIsLink, bool* aIsOnclick)

So, some alternative ways to do this

Accessible* ActionAcc(bool* aLink = nullptr, bool* aOnclick = nullptr);

then you wouldn't always need the dummy args, and could merge the functions.

Or from the department of use templates wherever possible

template<bool WantLinks = true, bool WantOnclicks = true>
Accessible*
ActionWalk()
{
if (WantOnclick && nsCoreUtils::HasClickListener(this)) {
  ....
}

...
}

which should generally be faster I think since in all the cases you only want one of onclicks or links you don't do the other checks.  On the other hand templates.  It would be nicer if this could get around the constness issues, but I'm not sure that's possible with default template args.

> void
> LinkableAccessible::Shutdown()

might as well remove it

>-  // Accessible
>-  virtual void BindToParent(Accessible* aParent, uint32_t aIndexInParent) override;
>-  virtual void UnbindFromParent() override;

I believe this is the only override so you could devirtualize these now

A> 
>     function removeOnClickAttr(aID)
>     {
>       this.__proto__ = new textLeafUpdate(aID, false);
>+      this.eventSeq.push(new invokerChecker(EVENT_SHOW, this.node));

what is this change for?

Comment 4

[Lorien Hu (:lsocks)]

Attachment: updated do not cache action info

>> it'd be nice to have a test for the event handler case this fixes

This bug only happens if we remove onclick from DocAccessible::UpdateAccessibleOnAttrChange (which is necessary to avoid dupe recreation with bug 1175913). The first test will already fail with that change if BaseAccessible isn't updated with it.

>> what is this change for?

I added the show event to make sure that the accessible is getting recreated instead of just destroyed

Comment 5

[Lorien Hu (:lsocks)]

Attachment: updated do not cache action info

Comment 6

[alexander :surkov (:asurkov)]

I'm somewhat concerned that we have to traverse the tree up for every accessible states request for every text node. That shouldn't affect on IA2-only screen readers but may hit MSAA's ones. If we aren't going to get rid of accessible recreation for linkables at all then I'd say ideally we should store the info in context bits. Are we?

Comment 7

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #6)
> I'm somewhat concerned that we have to traverse the tree up for every
> accessible states request for every text node. That shouldn't affect on
> IA2-only screen readers but may hit MSAA's ones. If we aren't going to get
> rid of accessible recreation for linkables at all then I'd say ideally we
> should store the info in context bits. Are we?

That's really hard to parse.
I guess you are asking if you can cache somehow.  I don't think there's a reasonable way to get invalidation to work.  I think you basically have to walk the whole subtree on attribute changes and clear the cache.  Other wise checking the cache is valid means you need to walk all the way up to the document (though you might save some time on checking if there's click listeners and link states).  Now that traverse the tree and update caches is probably better than recreation perf wise, but it seems like a premature optimization.

Given the vast majority of clients are dictionaries and stuff that probably never get states at all it seems better to risk some slow down in getting states to get rid of recreation that's not really needed.

If always walking the tree turns out to be too slow we could probably cache the link and onclick state for a particular accessible and keep that up to date.  That might be enough faster even with the walk of the tree, but  that also feels like premature optimization.

Comment 8

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #7)
> (In reply to alexander :surkov from comment #6)
> > I'm somewhat concerned that we have to traverse the tree up for every
> > accessible states request for every text node. That shouldn't affect on
> > IA2-only screen readers but may hit MSAA's ones. If we aren't going to get
> > rid of accessible recreation for linkables at all then I'd say ideally we
> > should store the info in context bits. Are we?
> 
> That's really hard to parse.
> I guess you are asking if you can cache somehow.  I don't think there's a
> reasonable way to get invalidation to work.  I think you basically have to
> walk the whole subtree on attribute changes and clear the cache.  Other wise
> checking the cache is valid means you need to walk all the way up to the
> document (though you might save some time on checking if there's click
> listeners and link states).  Now that traverse the tree and update caches is
> probably better than recreation perf wise, but it seems like a premature
> optimization.

my point is if we do keep accessible recreation then we should be able to stick with current approach. Caching stuff is a nice thing but you don't have to deal with it here.

> Given the vast majority of clients are dictionaries and stuff that probably
> never get states at all it seems better to risk some slow down in getting
> states to get rid of recreation that's not really needed.

We shouldn't regress screen reader user experience though.

> If always walking the tree turns out to be too slow we could probably cache
> the link and onclick state for a particular accessible and keep that up to
> date.  That might be enough faster even with the walk of the tree, but  that
> also feels like premature optimization.

do you have ideas how to measure how slower we get?

Comment 9

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #8)
> (In reply to Trevor Saunders (:tbsaunde) from comment #7)
> > (In reply to alexander :surkov from comment #6)
> > > I'm somewhat concerned that we have to traverse the tree up for every
> > > accessible states request for every text node. That shouldn't affect on
> > > IA2-only screen readers but may hit MSAA's ones. If we aren't going to get
> > > rid of accessible recreation for linkables at all then I'd say ideally we
> > > should store the info in context bits. Are we?
> > 
> > That's really hard to parse.
> > I guess you are asking if you can cache somehow.  I don't think there's a
> > reasonable way to get invalidation to work.  I think you basically have to
> > walk the whole subtree on attribute changes and clear the cache.  Other wise
> > checking the cache is valid means you need to walk all the way up to the
> > document (though you might save some time on checking if there's click
> > listeners and link states).  Now that traverse the tree and update caches is
> > probably better than recreation perf wise, but it seems like a premature
> > optimization.
> 
> my point is if we do keep accessible recreation then we should be able to
> stick with current approach. Caching stuff is a nice thing but you don't
> have to deal with it here.

but if you keep recreating then you run into test failures, and arguably more importantly it will mean you recreate a bunch of stuff you don't need to which is bad.

> > Given the vast majority of clients are dictionaries and stuff that probably
> > never get states at all it seems better to risk some slow down in getting
> > states to get rid of recreation that's not really needed.
> 
> We shouldn't regress screen reader user experience though.

but we don't have any evidence that will actually happen, and we will regress something if we change anything.

So if we continue to handle the caching of link state with tree recreation we'll regress perf for everybody if we start recreating the tree when event handlers are used as well as attributes.  However we need to do that to fix bugs where the cache gets out of date.  So the choice is basically maybe regress perf for some windows screen readers, or regress it for everybody.

> > If always walking the tree turns out to be too slow we could probably cache
> > the link and onclick state for a particular accessible and keep that up to
> > date.  That might be enough faster even with the walk of the tree, but  that
> > also feels like premature optimization.
> 
> do you have ideas how to measure how slower we get?

I guess you could measure average time of LinkState(), but I'm not convinced its worth doing, and if you did it I'm not sure how you'd know what's an important change vs a ignorable one.

So I think I've convinced myself taking the perf hit to LinkableAccessible stuff is something we probably need to do.

Comment 10

[alexander :surkov (:asurkov)]

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

> > my point is if we do keep accessible recreation then we should be able to
> > stick with current approach. Caching stuff is a nice thing but you don't
> > have to deal with it here.
> 
> but if you keep recreating then you run into test failures,

can you elaborate?

> and arguably
> more importantly it will mean you recreate a bunch of stuff you don't need
> to which is bad.

I'm up to not have recreation, but I get confused whether we do recreation or not. Or do we make it sometimes and sometimes not?

> > > Given the vast majority of clients are dictionaries and stuff that probably
> > > never get states at all it seems better to risk some slow down in getting
> > > states to get rid of recreation that's not really needed.
> > 
> > We shouldn't regress screen reader user experience though.
> 
> but we don't have any evidence that will actually happen, and we will
> regress something if we change anything.

accessible states is a known bottleneck, that's why I would have some numbers before making the change.

> I guess you could measure average time of LinkState(), but I'm not convinced
> its worth doing, and if you did it I'm not sure how you'd know what's an
> important change vs a ignorable one.
> 
> So I think I've convinced myself taking the perf hit to LinkableAccessible
> stuff is something we probably need to do.

I think I'm good if you take review then.

Comment 11

[Lorien Hu (:lsocks)]

Attachment: updated do not cache action info

Comment 12

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #10)
> (In reply to Trevor Saunders (:tbsaunde) from comment #9)
> 
> > > my point is if we do keep accessible recreation then we should be able to
> > > stick with current approach. Caching stuff is a nice thing but you don't
> > > have to deal with it here.
> > 
> > but if you keep recreating then you run into test failures,
> 
> can you elaborate?

I haven't investigated myself, but I think it might just be test_textleaf.html where setting both role and onclick gets two reorder events because the onclick causes recreation after its already happened for the role change.


> 
> > and arguably
> > more importantly it will mean you recreate a bunch of stuff you don't need
> > to which is bad.
> 
> I'm up to not have recreation, but I get confused whether we do recreation
> or not. Or do we make it sometimes and sometimes not?

I don't understnad the question.

if we listen for event listener changes as well as attribute changes then we would do more recreation in the case elements are already accessible, but need to be recreated so action acc info cache is updated.

> > > > Given the vast majority of clients are dictionaries and stuff that probably
> > > > never get states at all it seems better to risk some slow down in getting
> > > > states to get rid of recreation that's not really needed.
> > > 
> > > We shouldn't regress screen reader user experience though.
> > 
> > but we don't have any evidence that will actually happen, and we will
> > regress something if we change anything.
> 
> accessible states is a known bottleneck, that's why I would have some
> numbers before making the change.

Well, tree updating is too.  Which makes me realize this may be worth doing on those grounds since we devirtualize BindToParent, and we could probably get rid of it all togehter making things a bit simpler.

Comment 13

[Trevor Saunders (:tbsaunde)]

Comment on attachment 8643233 [details] [diff] [review]
updated do not cache action info

>   LinkableAccessible(nsIContent* aContent, DocAccessible* aDoc) :
>-  AccessibleWrap(aContent, aDoc),
>-  mActionAcc(nullptr),
>-  mIsLink(false),
>-  mIsOnclick(false)
>+  AccessibleWrap(aContent, aDoc)
> {
> }

not related, but inline it?

> LinkableAccessible::TakeFocus()
> {
>-  if (mActionAcc)
>-    mActionAcc->TakeFocus();
>-  else
>+  if (Accessible* actionAcc = ActionWalk()) {
>+    actionAcc->TakeFocus();
>+  }
>+  else {

} else {

>   Accessible::Value(aValue);
>-  if (!aValue.IsEmpty())
>+  if (!aValue.IsEmpty()) {
>     return;
>+  }
> 
>-  if (aValue.IsEmpty() && mIsLink)
>-    mActionAcc->Value(aValue);
>+  bool isLink;
>+  Accessible* actionAcc = ActionWalk(&isLink);
>+

blank line not needed

>+  if (aValue.IsEmpty() && isLink) {

the only way to get here is by aValue being empty right?

>+    ActionWalk(&isLink, &isOnclick);
>+
>+    if (isLink) {

no blank line?

>       aName.AssignLiteral("jump");
>-    else if (mIsOnclick)
>+    }
>+    else if (isOnclick) {

} else {

Comment 14

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #12)
> > > but if you keep recreating then you run into test failures,
> > 
> > can you elaborate?
> 
> I haven't investigated myself, but I think it might just be
> test_textleaf.html where setting both role and onclick gets two reorder
> events because the onclick causes recreation after its already happened for
> the role change.

I'd be interesting to know why they don't get coalesced

> > > and arguably
> > > more importantly it will mean you recreate a bunch of stuff you don't need
> > > to which is bad.
> > 
> > I'm up to not have recreation, but I get confused whether we do recreation
> > or not. Or do we make it sometimes and sometimes not?
> 
> I don't understnad the question.
> 
> if we listen for event listener changes as well as attribute changes then we
> would do more recreation in the case elements are already accessible, but
> need to be recreated so action acc info cache is updated.

you mean you don't recreate accessibles in some cases to avoid duping recreations?

> > accessible states is a known bottleneck, that's why I would have some
> > numbers before making the change.
> 
> Well, tree updating is too.  Which makes me realize this may be worth doing
> on those grounds since we devirtualize BindToParent, and we could probably
> get rid of it all togehter making things a bit simpler.

If you propagated the info into subtrees (assuming you still do recreation) then both tree creation and states calculation were performant, and we were sure that the change will not harm AT.

Comment 15

[Lorien Hu (:lsocks)]

Attachment: updated do not cache action info with nit fixes

carry r=tbsaunde

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ef291c497173

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ef291c497173