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.

Sign up for GitHub

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

Jump to bottom

added route change announcement #4812

Merged
merged 6 commits into from
Mar 25, 2021
Merged

added route change announcement #4812

merged 6 commits into from
Mar 25, 2021

Conversation

AlexAndBear
Copy link
Contributor

@AlexAndBear AlexAndBear commented Mar 16, 2021 •
edited
Loading

Description

Enhancement: Implement live region updates on route changes

Related Issue

Motivation and Context

How Has This Been Tested?

  • test environment:
  • test case 1:
  • test case 2:
  • ...

Screenshots (if appropriate):

image

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

Open tasks:

  • ...

@owncloud owncloud deleted a comment from update-docs bot Mar 16, 2021
@owncloud owncloud deleted a comment from CLAassistant Mar 16, 2021
@AlexAndBear AlexAndBear marked this pull request as ready for review March 16, 2021 16:13
@AlexAndBear AlexAndBear force-pushed the a11y-region-update branch 3 times, most recently from a0192a4 to 60b81ae Compare March 17, 2021 10:11
@AlexAndBear
Copy link
Contributor Author

@kulmann i18n interpolate introduced

@AlexAndBear AlexAndBear requested a review from kulmann March 19, 2021 13:20
kulmann
kulmann approved these changes Mar 19, 2021
View reviewed changes
Copy link
Member

@kulmann kulmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💪

@kulmann kulmann requested a review from marcus-herrmann March 19, 2021 13:21
@kulmann
Copy link
Member

kulmann commented Mar 19, 2021

Added @marcus-herrmann as reviewer, since I lack experience with the live region updates and cannot tell if this is sufficient. Code looks good. Could you take a look @marcus-herrmann ?

marcus-herrmann
marcus-herrmann approved these changes Mar 19, 2021
View reviewed changes
Copy link
Contributor

@marcus-herrmann marcus-herrmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. But is it possible to see this working live soon?

@kulmann
Copy link
Member

kulmann commented Mar 19, 2021

LGTM. But is it possible to see this working live soon?

As soon as it's merged it will be grabbed by the next auto-reset on web.owncloud.com. Happens at least once per day afaik. So if we can merge it today it would be visible by tomorrow.

@marcus-herrmann
Copy link
Contributor

Ok. So web.owncloud.com is newer than ocis-a11y.owncloud.com then. Interestingly I see a different side wide search function there (compared to ocis-a11y where I audited). Also, the files table fix doesn't seem to be on "web."

@kulmann
Copy link
Member

kulmann commented Mar 19, 2021

Ok. So web.owncloud.com is newer than ocis-a11y.owncloud.com then. Interestingly I see a different side wide search function there (compared to ocis-a11y where I audited). Also, the files table fix doesn't seem to be on "web."

ocis doesn't have search, that's why it's disabled there. Files table is still not merged. ;-) The PR is close to being merged though.

@marcus-herrmann
Copy link
Contributor

ocis doesn't have search, that's why it's disabled there. Files table is still not merged. ;-) The PR is close to being merged though.

Oh I see. But if ocis-a11y is not representative (at least in the regards of search), it is not ideal as a basis for an accessibility audit?

@marcus-herrmann
Copy link
Contributor

Oh I see. But if ocis-a11y is not representative (at least in the regards of search), it is not ideal as a basis for an accessibility audit?

Because I see a WCAG 3.3.2 violation there: https://www.w3.org/TR/WCAG20-TECHS/H32.html (sorry for hijacking the ticket)

JanAckermann and others added 2 commits March 25, 2021 19:44
Jan Ackermann and others added 2 commits March 25, 2021 19:44
pascalwengerter
pascalwengerter approved these changes Mar 25, 2021
View reviewed changes
Copy link
Contributor

@pascalwengerter pascalwengerter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 🔥

@pascalwengerter
Copy link
Contributor

Works well for me, locally. @marcus-hermann can this be merged and is your concerns regarding a possible WCAG 3.3.2 violation (I guess regarding the search submit, but not sure if related to route change announcements?) tracked somewhere already?

@kulmann
Copy link
Member

kulmann commented Mar 25, 2021

Merging, as the comment about search was not related to the announcer. Question about whether or not it is tracked somewhere is still valid though. 😅

@kulmann kulmann merged commit 703b06d into master Mar 25, 2021
@delete-merged-branch delete-merged-branch bot deleted the a11y-region-update branch March 25, 2021 21:53
@marcus-herrmann
Copy link
Contributor

Works well for me, locally. @marcus-hermann can this be merged and is your concerns regarding a possible WCAG 3.3.2 violation (I guess regarding the search submit, but not sure if related to route change announcements?) tracked somewhere already?

@pascalwengerter: Sorry, this is unrelated to this ticket, so it would be absurd to have been a merge blocker. I will create a new issue, though, since this could/will be critical for WCAG/BITV but was not part of the version I audited.

@pascalwengerter
Copy link
Contributor

Works well for me, locally. @marcus-hermann can this be merged and is your concerns regarding a possible WCAG 3.3.2 violation (I guess regarding the search submit, but not sure if related to route change announcements?) tracked somewhere already?

@pascalwengerter: Sorry, this is unrelated to this ticket, so it would be absurd to have been a merge blocker. I will create a new issue, though, since this could/will be critical for WCAG/BITV but was not part of the version I audited.

Was more of a rethoric question ;) thank you & looking forward to the new ticket (and more information on what's to be fixed)!

@marcus-herrmann marcus-herrmann mentioned this pull request Mar 26, 2021
Closed
@marcus-herrmann
Copy link
Contributor

Was more of a rethoric question ;) thank you & looking forward to the new ticket (and more information on what's to be fixed)!

Regarding that: #4867

@fschade fschade mentioned this pull request Mar 29, 2021
Closed
17 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pascalwengerter pascalwengerter pascalwengerter approved these changes

@kulmann kulmann kulmann approved these changes

@marcus-herrmann marcus-herrmann marcus-herrmann approved these changes

Assignees

@AlexAndBear AlexAndBear

@JammingBen JammingBen

Labels
Category:Enhancement Add new functionality Topic:Accessibility
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Audit #1 – General Overview when logged in – Route change don't cause live region updates
5 participants
@AlexAndBear @kulmann @marcus-herrmann @pascalwengerter @JammingBen