796955 - remove mInvalidationList from DocAccessible

Status: NEW

(Core :: Disability Access APIs, defect)

Description

[Trevor Saunders (:tbsaunde)]

recreating subtrees sucks

Comment 1

[Trevor Saunders (:tbsaunde)]

Attachment: wip

This  almost works.  The basic idea is to update the reverse map from id
to to what maps to them on DOM tree modifications.

One remaining issue is xbl bindings.  Sadly there currently is no
notification that they've been attached to the tree so we get reverse
relations were the idref is on a node in the binding wrong, we don't
have a relation at all.

I discussed ways of dealing with these reverse relations and the
somewhat related issue of accessible creation depending on them with
Boris a couple days ago, and I thing the conclussions were roughly the
following.

- having to create accessibles only because an idref points at the
  relevent DOM node, and having to do reverse relations pointing back at
the refering node sucks.

We'd rather not degrad perf in the case a11y isn't on by handling this
cache in attribute parsing / adding nodes to the tree.

- the approach in this patch of using the existing nsIDocumentObserver
  a11y has for each document seems reasonablish given we want to solve
the problem.  We could probably add a notification to tell a11y a xbl
binding has been attached.

There's also of course the approaches I think bz would like of either
not supporting this stuff or atleast not caching it.

I'm worried about backwards compat and stuff and worse user experience
if we remove this stuff I'm pretty sure atleast a couple screen readers
use reverse relations, and I have no idea what else does.

I don't think removing the cache is really workable because we can't
aford to crawl the DOM tree looking for idrefs during a11y tree update,
and if we do that looking for reverse relations on say the html spec
page we'll probably be pretty janky atleast for screen readers, and
whatever else decides to use reverse relations.

While totally removing support sounds nice I don't think we can do it
even if only because Alex won't like it.

However since xbl is aparently chrome only I think we should be able to
say that in chrome you should mkae both directions of the relation
explicit  in most cases @for / @control I think xul should be fine with
aria-labelledby, and we can certainly add a non standard aria-labelfor
to chrome for the other cases where someone insists on using something
other than a label as a label.  Then I think it would be fine for non
explicit reverse relations where the refering node is in a xbl anon
content binding to not provide reverse relations.  I think I'd like to
remove support for implicit reverse relations at all in chrome
documents, but I can leave that fight for another day.

to look

Comment 2

[alexander :surkov (:asurkov)]

mInvalidationList is not bad since it's just lazy accessible creation. It's bad that we invalidate all children (instead inserting the created accessible) during invalidation list processing but it's not related with mInvalidationList approach. 

I think I'm fine if we cache IDs during DOM tree creation. However I like the same time that our IDs cache is accessible tree oriented: we update it whenever accessible tree is changed.

Comment 3

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #2)
> mInvalidationList is not bad since it's just lazy accessible creation. It's
> bad that we invalidate all children (instead inserting the created
> accessible) during invalidation list processing but it's not related with
> mInvalidationList approach. 

I don't claim InvalidateChildren() and mInvalidationList are terribly entagled, but I do still claim mInvalidationList is a bad thing.  just lazy accessible creation isn't that bad, but atelast for me accessible tree creation is complex enough that its hare to reason about.  So I think anything we can do to make it simpler is a good thing.  I'd say stopping lazy creation is one of these things that makes it simpler and easier to reason about.

On the other hand even if we did make the lazy creation more efficent it would still be worse than doing it up front because we have to possible remove a nolonger valid accessible and then add a new moving the existing children around possibly not terrible, but still pointless if we could have done the work earlier.

> I think I'm fine if we cache IDs during DOM tree creation. However I like
> the same time that our IDs cache is accessible tree oriented: we update it
> whenever accessible tree is changed.

I think keeping it up to date with DOM sort of makes more sense since its a cache of data about the DOm.  However I actually believe the right thing to do is to fix the spec and make the compat breaking changes to have script / html provide reverse relations so we don't need cache sooner rather than later.

Comment 4

[alexander :surkov (:asurkov)]

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

> I don't claim InvalidateChildren() and mInvalidationList are terribly
> entagled, but I do still claim mInvalidationList is a bad thing.  just lazy
> accessible creation isn't that bad, but atelast for me accessible tree
> creation is complex enough that its hare to reason about.  So I think
> anything we can do to make it simpler is a good thing.  I'd say stopping
> lazy creation is one of these things that makes it simpler and easier to
> reason about.

it seems what we should do here is bug 690417

Comment 5

[alexander :surkov (:asurkov)]

Trev, getting back to your bug 501580 comment #8, why can't this patch keep "XBL nonsense"?

Comment 6

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #5)
> Trev, getting back to your bug 501580 comment #8, why can't this patch keep
> "XBL nonsense"?

I'm not sure what question is.  Look at comment 1 maybe?

Comment 7

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #6)
> (In reply to alexander :surkov from comment #5)
> > Trev, getting back to your bug 501580 comment #8, why can't this patch keep
> > "XBL nonsense"?
> 
> I'm not sure what question is.

Basically I'd like to get back to this bug. I agree InvalidateList complicates the tree creation logic and having a relations cache for content has own benefits.

>  Look at comment 1 maybe?

it outlines number of approaches, I wanted to understand what break this patch and is there a nice way to keep backward compatibility.

Comment 8

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #7)
> (In reply to Trevor Saunders (:tbsaunde) from comment #6)
> > (In reply to alexander :surkov from comment #5)
> > > Trev, getting back to your bug 501580 comment #8, why can't this patch keep
> > > "XBL nonsense"?
> > 
> > I'm not sure what question is.
> 
> Basically I'd like to get back to this bug. I agree InvalidateList
> complicates the tree creation logic and having a relations cache for content
> has own benefits.

ok

> >  Look at comment 1 maybe?
> 
> it outlines number of approaches, I wanted to understand what break this
> patch and is there a nice way to keep backward compatibility.

I think its just the part of that comment saying we don't get notified through ContentInserted() / COntentRemoved() for changes in the xbl binding, so that part of cache wouldn't get updated.

Comment 9

[alexander :surkov (:asurkov)]

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

> I think its just the part of that comment saying we don't get notified
> through ContentInserted() / COntentRemoved() for changes in the xbl binding,
> so that part of cache wouldn't get updated.

I see. What approaches can we take here to workaround it?

Comment 10

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #9)
> (In reply to Trevor Saunders (:tbsaunde) from comment #8)
> 
> > I think its just the part of that comment saying we don't get notified
> > through ContentInserted() / COntentRemoved() for changes in the xbl binding,
> > so that part of cache wouldn't get updated.
> 
> I see. What approaches can we take here to workaround it?

I guess same options as comment 1?

Comment 11

[alexander :surkov (:asurkov)]

ok, then could you get back to this patch?

Comment 12

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #11)
> ok, then could you get back to this patch?

maybe? I'd really rather not do any of those things though.

Comment 13

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #12)
> (In reply to alexander :surkov from comment #11)
> > ok, then could you get back to this patch?
> 
> maybe? I'd really rather not do any of those things though.

would it be fast enough if we do a cache on content and accessible mutations both? what alternatives do we have?

Comment 14

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #13)
> (In reply to Trevor Saunders (:tbsaunde) from comment #12)
> > (In reply to alexander :surkov from comment #11)
> > > ok, then could you get back to this patch?
> > 
> > maybe? I'd really rather not do any of those things though.
> 
> would it be fast enough if we do a cache on content and accessible mutations
> both? what alternatives do we have?

depends what you consider fast enough, also then the cache would have this only sort of up to date issue.

I'm still tempted to break the API and require the html to provide the reverse relations.

Comment 15

[alexander :surkov (:asurkov)]

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

> > would it be fast enough if we do a cache on content and accessible mutations
> > both? what alternatives do we have?
> 
> depends what you consider fast enough,

If it takes a certain percent of time of accessible object creation then I would try to find something else I think

> also then the cache would have this
> only sort of up to date issue.

details?

> I'm still tempted to break the API and require the html to provide the
> reverse relations.

that'd be easier solution for engineering but not acceptable I afraid, so I'd rather focus on other alternatives.

Do you have in your mind a more or less ready approach to try, the one that would preserve backward compatibility? Would you like to implement it?

Comment 16

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #15)
> (In reply to Trevor Saunders (:tbsaunde) from comment #14)
> 
> > > would it be fast enough if we do a cache on content and accessible mutations
> > > both? what alternatives do we have?
> > 
> > depends what you consider fast enough,
> 
> If it takes a certain percent of time of accessible object creation then I
> would try to find something else I think

well, its going to take > 0 time, my question was how much greater than 0 is acceptable.

> > also then the cache would have this
> > only sort of up to date issue.
> 
> details?

Well, if you update the cache for normal content on dom changes but only update cache for xbl content on a1`1y tree changes then for say content insertions there will be period where reflow hasn't happened yet and so normal content's cache will be up to date but cache for xbl stuff won't be.  On the other hand I'm not sure if that will matter because xbl bindings is sometimes sync and sometimes not, and I'm not sure the out of dateness even matters.

> > I'm still tempted to break the API and require the html to provide the
> > reverse relations.
> 
> that'd be easier solution for engineering but not acceptable I afraid, so
> I'd rather focus on other alternatives.

I'm less convinced its not acceptable, but maybe.

> Do you have in your mind a more or less ready approach to try, the one that
> would preserve backward compatibility? Would you like to implement it?

I think my prefered alternative may be to add some special call into a11y when xbl binding is attached, but that seems pretty gross and I can't really say I'm excited to do it.

Comment 17

[alexander :surkov (:asurkov)]

If you prefer adding notifications into xbl code then it's fine and actually I like the approach more than double caching thing (content + accessible tree mutations).

Personally I feel a bit overswamped by wanted a11y improvements so I would really appreciate if you take it, having you involved here is a good way to keep our user even more happier.