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

XWIKI-18838: Use ARIA landmarks for main page regions #2166

Merged
merged 22 commits into from
Sep 13, 2023

Conversation

Sereza7
Copy link
Contributor

@Sereza7 Sereza7 commented May 3, 2023

Jira: https://jira.xwiki.org/browse/XWIKI-18838
PR Changes:

  • Added main landmarks on pages.

Notes:

  • As of now, using the preview will create an accessibility violation (two main landmarks on the page, one inside an iframe in the other). AFAIK this is only on a few pages accessible by admins (e.g. color theme editing) so it's okay.
  • The element that was used to be the main landmark depends on the templates, since we don't want to include breadcrumbs in it. Typically, it was either: set on the mainContentArea if it doesn't start with a "top breadcrumb", or set on a new DOM element that contains the title and the content of the page (without the breadcrumb).
  • This PR is very much related to XWIKI-20696: Accessibility best-practice: use landmarks in view pages #2133 . The following views are made without the changes in the PR for XWIKI-20696. My solution for XWIKI-20696 as of now contains changes to the hierarchy macro and makes all breadcrumbs nav landmarks.

View:
Those screenshots were made with the landmark extension for Firefox.
You should look for the Main landmark indicated with a pink square on every screenshot
18838-accessDenied


18838-login


18838-editInline


18838-edit


18838-DoesNotExist


18838-FlamingoDelete


18838-FlamingoCreate


18838-flamingoCopy


18838-contentView


18838-moveAttachment

Sereza7 and others added 9 commits April 25, 2023 11:52
* Replaced all div with id mainContentArea by main elements
* Changed the position of the main content on pages where it needs to be more specific (e.g. view, now main only has the text, and ToS users don't need to go through the breadcrumb on every use)
* Removed the hierarchy from main on multiple pages.
* Fixed the main size on pdfoptions.vm
* Updated the test resources

Note: Conducted tests on the standards validator module, everything passes: https://up1.xwikisas.com/#RY7BYLLfd_ijp74PbDlFkg
@Sereza7
Copy link
Contributor Author

Sereza7 commented May 4, 2023

Ready for review

PR Changes:

  • Updated the test resources in xwiki-platform-tool-standards-validator

Note: Tests on the xwiki-platform-tool-standards-validator module after changes passed.

@Sereza7 Sereza7 marked this pull request as ready for review May 4, 2023 08:43
* Removed the condition on editinline main instantiation.

Note: It seems like the structure above the content is generated with another template  (or overwritten??) when AllowDocEdit is false.
@michitux
Copy link
Contributor

While I agree that it might be preferable to not to have the breadcrumbs in the main content I wonder if it is really a good idea to have on some pages <div id="mainContentArea"> while on others it is <main id="mainContentArea">. This creates quite an inconsistency that could lead to strange problems. Btw., I found this code which I think needs to be updated and also nicely highlights the problem with this inconsistency:

.findElementWithoutWaiting(By.xpath("//div[@id = 'mainContentArea']/pre[contains(@class, 'xwikierror')]"))

Also, see #1667, I think it would be nice to have a consistent jump target for the skip header link. Maybe we should re-consider the decision to (not) include the breadcrumbs in the main content area.

@Sereza7 Sereza7 marked this pull request as draft May 31, 2023 09:37
@Sereza7
Copy link
Contributor Author

Sereza7 commented Jun 6, 2023

Why we left the breadcrumb out of the main => https://forum.xwiki.org/t/accessibility-best-practice-landmarks/12051/10


I think the best solution is to move the mainContentArea to systematically follow the main tag name. This could mess up a lot of Page object selectors in tests. Thinking a bit more about it, there was barely any trouble with page objects when adding the main, so it should be ~= the same and not that much to change.
Anyways, those changes in the tree structure become massive and I'll go through a series of tests and manual viewing to check that it doesn't break anything (typically going wherever I took the screenshots in the PR starting message).

@michitux
Copy link
Contributor

michitux commented Sep 5, 2023

As I've just found it, here is another comment saying that breadcrumbs shouldn't be part of the main content: w3c/aria-practices#1469 (comment).

Sereza7 and others added 8 commits September 5, 2023 14:15
* Set the #mainContentArea on all the previously defined `main` nodes.
* Tried to fix some display issues

Note: This change in the architecture broke some presentation and there's still manual tests to see if the style fixes are enough.
@Sereza7
Copy link
Contributor Author

Sereza7 commented Sep 11, 2023

PR Updates

  • Removed the breadcrumbs from main on every template.
  • Improved comments
  • Homogenized changes

View

18838-delete
18838-notExisting
18838-editInline
18838-home
18838-copy

@Sereza7 Sereza7 marked this pull request as ready for review September 11, 2023 10:03
Copy link
Contributor

@michitux michitux left a comment

Choose a reason for hiding this comment

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

Looks good to me, but there is a conflict that prevents merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants