Closed Bug 1870783 Opened 7 months ago Closed 5 months ago

Expose correct role and relationships for popover

Categories

(Core :: Disability Access APIs, defect, P2)

defect

Tracking

()

RESOLVED FIXED
124 Branch
Tracking Status
firefox124 --- fixed

People

(Reporter: Jamie, Assigned: Jamie)

References

(Blocks 1 open bug)

Details

Attachments

(5 files)

Bug 1867811 exposes the expanded/collapsed state for popover control buttons. There are a few more things which need to be done for popover accessibility API support:

  1. A popover should get a minimum role of group.
  2. A popover should get an ispopup object attribute.
  3. A popover control button should (conditionally) have a details relationship referring to the popover, with an appropriate reverse relationship.
  4. A popover control button should get a details-roles object attribute if it has a details relation.
  5. A popover control button should only expose the expanded/collapsed state if it is not a descendant of its popover target.
  6. All popover control buttons should fire expanded state changes when the popover opens or closes. Currently, only the popover control button that is invoked (if any) fires a state change. This will almost certainly result in stale parent process and client caches.

See https://github.com/w3c/html-aam/pull/481 for more specific details.

Implementation notes:

  1. The minimum role transformation needs to happen after all other role stuff is done. Currently, we do that similar stuff in ARIATransformRole. However, we can't use that because that is also used for ARIARole(). So, I guess we'll need to have a separate method for minimum role.
  2. We should probably recreate the Accessible if the popover attribute is changed so that the role change is handled correctly.
  3. With regard to the cache and the details relation, we can't just add this to kRelationTypeAtoms because exposing the relation is conditional on whether it's a sibling, whether it already has a labelled/describedBy relation, etc. We're going to need to special case it slightly.
  4. We're going to need to find a way to get all control buttons for a given popover. This is needed to fire state change events on all control buttons when a popover opens/closes, to notify the cache of a relation change if the popover Accessible gets recreated, and to expose the detailsFor relation for LocalAccessible. That's going to be tricky because the popover may not necessarily have an id and our reverse relations are currently based only on ids.
Assignee: nobody → jteh
Blocks: 1769586

(In reply to James Teh [:Jamie] from comment #1)

  1. We're going to need to find a way to get all control buttons for a given popover. This is needed to fire state change events on all control buttons when a popover opens/closes, to notify the cache of a relation change if the popover Accessible gets recreated, and to expose the detailsFor relation for LocalAccessible.

To achieve this, I think we'll need to introduce a new thing similar to our dependent ids table (mDependentIDsHashes). Rather than mapping from ids to AttrRelProviders:

nsClassHashtable<nsStringHashKey, AttrRelProviders>

we will map from nsIContent (the target) to AttrRelProviders:

nsTHashMap<nsCOMPtr<nsIContent>, AttrRelProviders>

One possible problem is that we need to clean this table up when the page resets popoverTargetElements. We clean the id table up in DocAccessible::AttributeWillChange. However, it looks like we set the elements before we set the attribute, which might mean AttributeWillChange gets called too late. Hopefully, we can just flip those operations around if that turns out to be a problem.

We will still use the ids table when the attribute contains idrefs, but we'll use this new table when the attribute is non-null but empty and nsGenericHTMLElement::GetPopoverTargetElement returns elements.

A consideration is that nodes can be moved into different scopes. Tracking and verifying moves seems like annoying overhead for an obscure edge case. Instead, before we use a reference, we need to make sure that the reference is still valid in its current scope - i.e. that it is a descendant of any of the source element's shadow-including ancestors - just as GetAttrAssociatedElement does.

We can later reuse this dependent content table for ARIA element reflection, bug 1769586.

(In reply to James Teh [:Jamie] from comment #1)

  1. With regard to the cache and the details relation, we can't just add this to kRelationTypeAtoms because exposing the relation is conditional on whether it's a sibling, whether it already has a labelled/describedBy relation, etc.

An alternative is that we could use the aria-details key itself. Thus far, we've kept aria-foo keys specifically for relations coming directly from ARIA. However, there's no reason we can't bend that slightly. That would allow us to rely on the logic in LocalAccessible, which also means we don't duplicate that in RemoteAccessible. This also means we don't pointlessly cache the popover relation if aria-details is set.

A popover can have multiple invoker buttons.
Previously, we only fired a state change event on the button which was invoked.
This meant that the cached expanded/collapsed state of any other invokers was stale.
To facilitate this:

  1. Add popovertarget to DocAccessible's kRelationAttrs so that we track reverse relationships.
  2. When an Accessible is shown or hidden, if it is a popover, use RelatedAccIterator to get all the invokers and fire appropriate state change events on each of them.
  3. This also means we can get rid of nsAccessibilityService::PopovertargetMaybeChanged, since this explicit notification from DOM is no longer useful.

Even when there is a valid popovertarget, per the spec, these relations must not be exposed in certain cases.
This requires some special case code.
The code for calculating the DETAILS relation for popover invokers has been encapsulated in its own function.
We also use this to validate the reverse DETAILS_FOR relation on popovers.

Normally, when pushing cached relations to the parent process, we use IDRefsIterator so that we don't push implicit reverse relations.
For popover invokers, this wouldn't validate the other conditions.
To avoid duplicate special case code in RemoteAccessible, we use RelationByType for the DETAILS relation instead of IDRefsIterator when pushing the cache.
This leverages the LocalAccessible special case code for popover invokers.
This is fine for the DETAILS relation because nothing ever exposes an implicit reverse DETAILS relation.

Attachment #9377087 - Attachment description: Bug 1870783 part 4: Expose DETAILS and DETAILS_FOR relations for popovers and their invokers. → Bug 1870783 part 5: Expose DETAILS and DETAILS_FOR relations for popovers and their invokers.

(In reply to James Teh [:Jamie] from comment #5)

One possible problem is that we need to clean this table up when the page resets popoverTargetElements. We clean the id table up in DocAccessible::AttributeWillChange. However, it looks like we set the elements before we set the attribute, which might mean AttributeWillChange gets called too late. Hopefully, we can just flip those operations around if that turns out to be a problem.

This is a problem and my proposed solution won't work for three reasons:

  1. If popoverTargetElement is already set and it is reset to a different element, that won't fire an attribute change because the attribute value remains the empty string (no change).
  2. If we flip the setting of elements and attribute around, that breaks adding a new target, since the target element won't be set yet in AttributeChanged.
  3. In all cases, the elements are either set or not yet set both in AttributeWillChange and AttributeChanged, since those methods only deal with the content string attribute. That breaks detection of the expandable state change.

I think we're going to need new notifications for changes to explicitly set attr-elements, both before the change (WillChange) and after the change (Changed).

Blocks: 1879255

I've split support for the .popoverTargetElement WebIDL attribute into bug 1879255, as it's somewhat trickier. I don't think this first stack of patches will need to change to accommodate that other work.

No longer blocks: 1769586
Pushed by jteh@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1cd9878bf455
part 1: Expose a minimum role of GROUPING for popovers. r=eeejay
https://hg.mozilla.org/integration/autoland/rev/e8cfecb6b54f
part 2: Expose ispopup object attribute. r=eeejay
https://hg.mozilla.org/integration/autoland/rev/cf1659ce5c53
part 3: Do not expose expanded/collapsed state for a popover invoker if the popover is an ancestor of the invoker. r=eeejay
https://hg.mozilla.org/integration/autoland/rev/e581dc5deba4
part 4: Fire state change events on all popover invokers. r=eeejay
https://hg.mozilla.org/integration/autoland/rev/70d8667a1834
part 5: Expose DETAILS and DETAILS_FOR relations for popovers and their invokers. r=eeejay
Regressions: 1880063
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: