-
Notifications
You must be signed in to change notification settings - Fork 355
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
Updates to example and coverage report scripts #1859
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed a couple weird things in the generated results, but otherwise it looks good!
coverage/index.html
Outdated
<li><a href="../aria-practices.html#slider">Slider</a></li> | ||
<li><a href="../aria-practices.html#slidertwothumb">Slider (Multi-Thumb)</a></li> | ||
<li><a href="../aria-practices.html#aria_lh_search">Search</a></li> | ||
<li><a href="../aria-practices.html#aria_lh_search">Search</a></li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's something slightly odd here and in a couple other places, where it seems like it's generating duplicate links for several examples (here, and in contentinfo/complementary, and maybe others). I can take a closer look at why that's happening later, if you haven't already gotten to it by then 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@smhigley
Seems to be related to landmarks, thanks for finding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@smhigley
Found the problem and fixed the issues, thank you for pointing them out. Let me know if you see any other issues.
coverage/index.html
Outdated
</td> | ||
<td><a href="../examples/landmarks/contentinfo.html">Contentinfo Landmark</a> | ||
<td><a href="..\examples\checkbox\checkbox-2\checkbox-2.html">Checkbox (Mixed-State)</a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it looks like the links are using backslash instead of forward-slash now, and I'm not sure why. They should probably use forward slash, but it'd be good to also make sure it won't change based on the OS it's run on.
Also, same as above -- I'm happy to poke at it in a bit, if you haven't already fixed it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@smhigley
I think the problem is I generated the index.html
file on my Windows 10 computer, so I have now used my Mac and it seems to have fixed this problem.
@carmacleod |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really cool, @jongund !
Approving as-is, but I'm not really a fan of the <abbr title="none" style="color: gray">-</abbr>
in some of the tables. I think it looks weird (i.e. what is that funny little grey thing with the line and 2 dots?), plus the hyphen is hard to hover over because it's so tiny. I would prefer either just -
as in the other tables, or "none", or just leave the cell empty. Would love to know what value @mcking65 prefers in empty table cells.
This looks great @jongund.
suggested code change: or |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Taking a look at the preview of the indexes at:
https://raw.githack.com/w3c/aria-practices/update-example-index/examples/index.html
I see quite a few missing entries and some extra entries. It would be helpful to have an organized approach to validating the veracity of the indexes. It may take several people.
Here's a few that are easy to spot.
The list for menuitem looks incomplete. It includes only:
- Editor Menubar
- Navigation Menubar
The list for spinbutton looks incomplete. It includes only toolbar but not the spinbutton examples.
The list for role "tablist" looks incomplete. It doesn't include the carousel that has a tablist.
The list for toolbar looks incomplete. It doesn't include the listbox examples that have a toolbar.
Two examples are listed for the "list" role. List role is not used in these:
- Tabs with Automatic Activation
- Tabs with Manual Activation
4 examples are listed for the "log" role. None of these use the "log" role:
- Date Picker Combobox
- Alert Dialog
- Date Picker Dialog
- Modal Dialog
Five examples are listed for role="table" but only the "Table" example actually uses it:
- Date Picker Combobox
- Date Picker Dialog
- Data Grid
- Table
- Treegrid Email Inbox
There are only 12 states and properties listed in the state and property index. So, most are missing.
The list for aria-controls includes only 3 examples. I'm pretty certain there are more.
The list for aria-disabled has only one example. Accordion includes it (at least for now). I think we used it in the editor menubar as well. There may be others.
The list for aria-expanded does not include any of the menus, trees, or treegrid. There might be others that use it too.
The list for aria-haspopup does not include the menubuttons.
The list for aria-live should be double-checked. We use it in layout grid 2 (unless perhaps that is an alert). We have it in one of the carousels, I think.
The list for aria-modal does not include the modal dialog.
The list for aria-pressed does not include toggle button. I think there are others that may use it too.
The list for aria-valuemin does not include any spinbutton or slider examples.
I agree with @shirsha about the double reading of high contrast support. The text included in the link text is confusing because it makes it seem like "with high contrast support" is part of the example name. I don't think we should add that to the link text. Having "(HC)" outside the link text is appropriate for everyone. It is not part of the target page title, so it should not be part of the link text. And, including the high contrast support designation in the link text makes the link harder to understand. |
…example documentation
@mcking65 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, both the report results and the new approach!
@mcking65 |
@jongund I am supposed to review the PR. Sorry for the delay. I will finish the review tonight and add the approval if it looks good. In case we need further discussion, this was added to meeting agend for tomorrow, May 4. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feedback on APG Example Index page
- Role
meter
is missing in "Examples by Role" section. Here is meter example - Breadcrumb example has
aria-lable
andaria-current
which were not recorded in the "Example index" page.
Otherwise, it looks great. Thanks so much again for this user centered Example index page implementation, @jongund
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feedback on Coverage report page
- How about changing heading 1 to be "ARIA Example Coverage report" instead of "ARIA Roles, Properties and States Referenced in Guidance and Examples"?
- CSV files: Hard to read because multiple number of colunms are added after the "reference" column if there are more than one guidance or examples. (This is a minor issue)
- "Roles with no guidance or example list" is helpful to see what APG group needs to work on next. Thanks so much @jongund
- May "Roles with at Least One Guidance or Example (13)" be incoporated into "Roles with More than One Guidance or Example (34)" if the former table is just trying to indicate either guidance or example is missing?
- "Coding practice" table
- May we add a reference link to APG code-guide wiki under "Coding practice"?
- What does "ARIA Features that are different" mean?
- Can "Example Code ID" be "ID referece for ARIA Example container"?
- What would be the coding practice implication for "roles in documentation" and "aria attributes in documentation" unless they are in HTML and Javascript?
- Can "created using" be "created using Javascript"
I have updated the roles to include roles in ARIA 1.2, including |
@jongund @a11ydoer |
@a11ydoer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job and Thanks so much, @jongund!
breadcrumb file name issue 1886 was created and assigned to myself, @a11ydoer |
Out of curiosity and connecting dots question
I am wondering what aspect of QA we are referring to regarding the difference between attributes in HTML and JS but not ones in the documentation? |
@a11ydoer |
…scripts (pull w3c#1859) Updates the scripts that generate the example index and coverage reports: * Updated list of roles to include new roles in ARIA 1.2: * caption * code * deletion * emphasis * generic * input * insertion * meter * paragraph * Fixed bug in aria-label also getting references to aria-labelledby , by using a regex, and this was also applied to checking roles. * Updated code to use node-html-parser for identify the roles, properties and states associated with an example, making the code much easier to read and understand. * Added the identification of examples with specific high contrast support features. * Updated coverage report to provide more information on example quality assurance. * Add a coding practices section to the coverage report to monitor implementation of APG coding practices.
Editors and working group members would know about the URL and could use it for planning or reporting purposes. It does not need to be linked anywhere in the APG for the general public, but I don't think it needs to be "hidden" from the general public either. I don't think there is anything that is a secrete in the coverage report. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review complete
The changes in the this pull request:
aria-label
also getting references toaria-labelledby
, by using a regex, and this was also applied to checking roles.node-html-parser
for identify the roles, properties and states associated with an example, making the code much easier to read and understand.NOTE: The changes added
node-html-parser
to thepackage.json
file.Preview of example index
Preview of coverage report