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

Use navigation role for TOC #499

Merged
merged 1 commit into from Nov 9, 2015
Merged

Use navigation role for TOC #499

merged 1 commit into from Nov 9, 2015

Conversation

marcoscaceres
Copy link
Member

No description provided.

@plehegar
Copy link
Member

LGTM

plehegar added a commit that referenced this pull request Nov 9, 2015
@plehegar
Merge pull request #499 from w3c/fix_aria_role_for_toc
6bb9146 
Use navigation role for TOC
@plehegar plehegar merged commit 6bb9146 into develop Nov 9, 2015
@anssiko
Copy link
Member

anssiko commented Nov 13, 2015

This change breaks Pubrules Checker, see e.g.:

https://labs.w3.org/pubrules/?url=https%3A%2F%2Flabs.w3.org%2Fspec-generator%2F%3Ftype%3Drespec%26url%3Dhttps%3A%2F%2Fw3c.github.io%2Fmanifest%2F%3FspecStatus%3DWD%3BshortName%3Dappmanifest&profile=WD-Echidna&validation=simple-validation&noRecTrack=false&informativeOnly=false&echidnaReady=true&patentPolicy=pp2004&processDocument=2015

The error is:

[561@129] Bad value “navigation” for attribute “role” on element “ul”.

This means Echidna now refuses to publish documents authored with ReSpec.

An obvious fix would be to revert this change or fix Pubrules checker, whichever is right.

@stevefaulkner
Copy link

role=navigation on ul is non-conforming in HTML
http://w3c.github.io/html-aria/#ul

suggest adding role=navigation to

and
no role on the

@marcoscaceres
Copy link
Member Author

This is partially my fault. I had a PR sitting there that I should have closed, and PLH merged it.

I'll back that out tomorrow.

Sent from my iPhone

On Nov 13, 2015, at 9:33 PM, stevefaulkner notifications@github.com wrote:

role=navigation on ul is non-conforming in HTML
http://w3c.github.io/html-aria/#ul

suggest adding role=navigation to

and
no role on the

@halindrome
Copy link
Contributor

I'm up - I will do it now. Actually, I will move it to the section element
unless that conflicts with the new stylesheets. Hang on.

On Fri, Nov 13, 2015 at 4:49 AM, Marcos Caceres notifications@github.com
wrote:

This is partially my fault. I had a PR sitting there that I should have
closed, and PLH merged it.

I'll back that out tomorrow.

Sent from my iPhone

On Nov 13, 2015, at 9:33 PM, stevefaulkner notifications@github.com
wrote:

role=navigation on ul is non-conforming in HTML
http://w3c.github.io/html-aria/#ul

suggest adding role=navigation to


and
no role on the

—
Reply to this email directly or view it on GitHub
#499 (comment).

Shane McCarron
halindrome@gmail.com

@halindrome
Copy link
Contributor

I have a fix for this that should resolve he echidna issue. Note that I personally don't feel this change is necessary at all. role="directory" which we had on there before is also correct. This is a static table of contents.

halindrome added a commit that referenced this pull request Nov 13, 2015
@halindrome
Moved navigation role to section around toc.
ac2b886 
Fixes issue with echidna as pointed out in
PR #499
halindrome added a commit that referenced this pull request Nov 13, 2015
@halindrome
Ensured aria role for toc is added to nav
98290f1 
Noticed that PR #499 changed the role to navigation.  This
PR changes the wrapper for TOCs to nav when using the new
experimental style sheets.  Added code to support both of these.
However, note that when using an nav element, the role of
navigation may be superfluous.  Opinion @stevefaulkner ?
@marcoscaceres
Copy link
Member Author

Agree about directory. I did some reading after I proposed the old PR and reached the same conclusion... But then never got around to closing the PR. We need a way of automatically checking changes against echidna.

Sent from my iPhone

On Nov 13, 2015, at 11:09 PM, Shane McCarron notifications@github.com wrote:

I have a fix for this that should resolve he echidna issue. Note that I personally don't feel this change is necessary at all. role="directory" which we had on there before is also correct. This is a static table of contents.

—
Reply to this email directly or view it on GitHub.

@anssiko
Copy link
Member

anssiko commented Nov 13, 2015

With 98290f1 getting a different kind of error:

[561@46] Bad value “navigation” for attribute “role” on element “section”.

@halindrome
Copy link
Contributor

Of course it complains. Seriously? @marcoscaceres what do you propose? Just back it out? Revert it to directory? @stevefaulkner do you have an opinion?

@marcoscaceres
Copy link
Member Author

Yeah, let's back it out please. We can experiment outside the main branch.

Sent from my iPhone

On Nov 14, 2015, at 12:16 AM, Shane McCarron notifications@github.com wrote:

Of course it complains. Seriously? @marcoscaceres what do you propose? Just back it out? Revert it to directory? @stevefaulkner do you have an opinion?

—
Reply to this email directly or view it on GitHub.

@halindrome
Copy link
Contributor

Okay. On it.

@marcoscaceres
Copy link
Member Author

Thanks, Shane, for handling this. You are a life saver!

Sent from my iPhone

On Nov 14, 2015, at 12:26 AM, Shane McCarron notifications@github.com wrote:

Okay. On it.

—
Reply to this email directly or view it on GitHub.

@stevefaulkner
Copy link

@halindrome indeed the conformance requirements disallowed navigation on section, which seemed wonky as its default role is region and it allowed contentinfo and search, so have updated to allow all landmarks.
http://w3c.github.io/html-aria/#section

@plehegar plehegar deleted the fix_aria_role_for_toc branch December 3, 2015 20:21
@marcoscaceres marcoscaceres restored the fix_aria_role_for_toc branch February 10, 2016 04:37
@marcoscaceres marcoscaceres deleted the fix_aria_role_for_toc branch February 27, 2016 16:43
@marcoscaceres marcoscaceres restored the fix_aria_role_for_toc branch March 2, 2016 02:47
@marcoscaceres marcoscaceres deleted the fix_aria_role_for_toc branch March 7, 2016 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@marcoscaceres @plehegar @anssiko @stevefaulkner @halindrome