844023 - group info might not be properly updated when flat trees mutate

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

spun off bug 527461 comment #28.

<div role="tree">
  <div role="treeitem"></div>
  <div role="group">
    <div role="treeitem"></div>
  </div>
</div>

1) receive group position for second item
2) remove first item

a) level of second item is not updated (still 2)
b) if you get node_child_of on second item then we crash

Comment 1

[alexander :surkov (:asurkov)]

Jonathan, this one may be quite interesting. Probably you will have some ideas.

Comment 2

[Jonathan Wei [:jwei]]

Attachment: Invalidate group info and refresh when necessary

When an Accessible is added to or removed from the accessible tree, we invalidate the GroupInfo of all its parent's children recursively.  If the GroupInfo of an Accessible we want to access is invalidated, we recalculate the set size and its position in set upon access.

Comment 3

[Jonathan Wei [:jwei]]

Attachment: Invalidate group info and refresh when necessary v2

Cleaned up a bit of code.  Also re-excluded a state flag in this patch that was causing a hang in some AccessFu test suites added recently.

Comment 4

[alexander :surkov (:asurkov)]

Trev, ping

Comment 5

[Trevor Saunders (:tbsaunde)]

Comment on attachment 8387783 [details] [diff] [review]
Invalidate group info and refresh when necessary v2

> AccGroupInfo::AccGroupInfo(Accessible* aItem, role aRole) :
>-  mPosInSet(0), mSetSize(0), mParent(nullptr)
>+  mPosInSet(0), mSetSize(0), mParent(nullptr), mSelf(aItem), mRole(aRole)

mItem might be clearer than mSelf

>+Accessible::SetDirtyGroupInfo(bool aIsDirty)

might as well inline it

btw I'm not sure this is better than nulling out mGroupInfo but whatever we have the bits and maybe its a win.

>+Accessible::InvalidateChildrenGroupInfo()
>+{
>+  uint32_t length = mChildren.Length();
>+  for (uint32_t i = 0; i < length; i++) {
>+    Accessible* child = mChildren[i];
>+    child->SetDirtyGroupInfo(true);
>+    child->InvalidateChildrenGroupInfo();

I'm not sure this realy needs to be recursive

>+   */
>+  bool HasDirtyGroupInfo() const { return mStateFlags & eGroupInfoDirty; }

might as well inline it too

Comment 6

[Jonathan Wei [:jwei]]

Try: https://tbpl.mozilla.org/?tree=Try&rev=1da50d67e178
Inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/23b83cff50a4

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/23b83cff50a4