Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Designer Accessibility] Insert peers in the DOM tree in the correct order #7514

Merged
merged 21 commits into from
Jun 28, 2022

Conversation

anna-dingler
Copy link
Contributor

@anna-dingler anna-dingler commented Jun 10, 2022

Related Issue

Progress towards #7436

Description

Instead of inserting designer peers to the bottom of the HTML tree, we must insert designer peers to mimic the order of the card.

To do so, I added a new method to find the correct placement before adding the new element to the designer surface.

How Verified

Verified manually on the adaptive cards website.

Outstanding Issues

  • Media element does not always update the heigh of the entire card correctly, so some of the peers overlap. I was able to repro on older version of the site as well.
  • Input elements should not be able to get focus outside of preview mode (ex: checkbox within Input.Toggle is still focusable)
Microsoft Reviewers: Open in CodeFlow

@ghost
Copy link

ghost commented Jun 10, 2022

Hi @anna-dingler. Thanks for helping make the AdaptiveCards JS renderer + tooling better. As additional verification, once the JS build succeeds, please go to the test site to test out your website/designer changes.

@anna-dingler anna-dingler marked this pull request as ready for review June 16, 2022 18:56
@anna-dingler anna-dingler changed the title [DRAFT] [Designer Accessibility] Insert peers to the HTML tree in the correct order [Designer Accessibility] Insert peers in the DOM tree in the correct order Jun 17, 2022
@ghost
Copy link

ghost commented Jun 23, 2022

Hi @anna-dingler. This pull request has had no recent activity for the past 5 days . Please take the necessary actions (review, address feedback or commit if reviewed already) to move this along.

@ghost
Copy link

ghost commented Jun 27, 2022

Staleness reset by anna-dingler

1 similar comment
@ghost
Copy link

ghost commented Jun 27, 2022

Staleness reset by anna-dingler

@dclaux
Copy link
Member

dclaux commented Jun 28, 2022

I'm not sure. This seems quite complicated. Wouldn't setting tabindex on each peer (in the right order) be enough?

@licanhua
Copy link
Contributor

licanhua commented Jun 28, 2022

I'm not sure. This seems quite complicated. Wouldn't setting tabindex on each peer (in the right order) be enough?

It's very interesting. Before dive into it, make sure it will not break any rule of tabindex

image

@dclaux
Copy link
Member

dclaux commented Jun 28, 2022

There are two choices (that I know of) to make navigation between DOM elements via the keyboard (or narration) happen in the intended order:

  • Either you rely on the order in which the elements are inserted into the DOM, and you don't use tabindex at all (except if something should explicitly not be focusable)
  • Or you entirely rely on tabindex, and the order in which the elements are inserted into the DOM doesn't matter

It definitely seems to me that the second option would yield a much simpler implementation. That said, I could be totally wrong.

@anna-dingler
Copy link
Contributor Author

I'm not sure. This seems quite complicated. Wouldn't setting tabindex on each peer (in the right order) be enough?

It's very interesting. Before dive into it, make sure it will not break any rule of tabindex

image

I've pinging @carlos-zamora to see if he has any context about this :)

@anna-dingler
Copy link
Contributor Author

There are two choices (that I know of) to make navigation between DOM elements via the keyboard (or narration) happen in the intended order:

  • Either you rely on the order in which the elements are inserted into the DOM, and you don't use tabindex at all (except if something should explicitly not be focusable)
  • Or you entirely rely on tabindex, and the order in which the elements are inserted into the DOM doesn't matter

It definitely seems to me that the second option would yield a much simpler implementation. That said, I could be totally wrong.

I spoke to @carlos-zamora (relaying the info here), and I think having the DOM tree in the correct order makes sense.

If we used a non-zero tabIndex, each element on the page would need to be assigned an index. We would also need to update tabIndex for each element every time we insert or remove elements; so the implementation wouldn't be simplified by much.

Additionally, it is best practice to use tabIndex=0 and have the elements follow the natural order.

@anna-dingler anna-dingler merged commit 1068436 into microsoft:main Jun 28, 2022
@anna-dingler anna-dingler deleted the accessibilityUpdates branch June 28, 2022 21:09
@compulim
Copy link
Contributor

compulim commented Jul 1, 2022

@anna-dingler thanks Anna for making the right choice. Having the correct DOM tree order makes much more sense as Adaptive Cards is an UI component and live in other apps.

If Adaptive Cards is its own app, yes, you can control tabindex and put whatever number in.

But Adaptive Cards is a UI component and hosted in apps that AC has no control/influence over. We should not use tabindex >= 1 because it will certainly conflict with the hosting app.

michaelfarnsworth pushed a commit to michaelfarnsworth/AdaptiveCards that referenced this pull request Nov 10, 2022
…order (microsoft#7514)

* Update popup background color for high contrast mode

* In progress: fix designer surface peer ordering

* In progress: use neighboring peer to find the correct place in the html tree

* Cherry-pick 'peers are tabbable, action buttons are not'

* Use parent peer instead of dropTarget

* Changes to account for card element actions (ex: add a column button)

* Merge in accessibility updates

* Resolve PR comments

* Find DOM neighbor for new ActionSet item

* Update addPeer method
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants