fixed bug with indexing aria-label property and added high contrast information #1709

jongund · 2020-12-29T19:38:08Z

The changes in the this pull request:

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, since it only adds a few lines of code to the example and coverage script files.
Examples with high contrast support have links with aria-label to indicate that the example has high contrast support (e.g. removed the use of the title attribute from previous proposals). The information will also then be available in screen reader "list of links" feature/

NOTE: The changes added node-html-parser to the package.json file.

Preview of example index
Preview of coverage report

…belledby

carmacleod · 2020-12-30T14:29:23Z

Do we want it to also match things like setAttribute('aria-label', label);?
If so, maybe use a regular expression instead of indexOf?
Something like the following should work, I think?
Note that \b means "end of word", and double-backslash is needed for a single backslash in javascript strings.

const regex = RegExp(ariaPropertiesAndStates[i] + '\\b');
if ( regex.test(code) ) ...

jongund · 2021-01-14T15:36:23Z

@carmacleod
We are only matching aria properties and states inside CODE elements in the Properties and States tables. I am not real great with regex statements, but I think your suggestion is a good one. I will update the pull request to use the regex.

Jon

mcking65

Jon, This was a great catch, thank you!!

We were definitely overlisting aria-label, and this is much better in that respect.

However, it does mistakenly leave out the checkbox example from aria-labelledby.

I see some examples where aria-label is used but not documented, but that is a separate issue. They should not be captured in the index (one of the layout grids and the rearrangeable listbox). I don't think it is worth capturing that in a separate issue because we already have issues calling for rework on both.

I think this may also introduce a new bug that effects indexing of aria-orientation. Strange that it is only that one property.

mcking65 · 2021-01-18T15:58:44Z

coverage/index.html

@@ -586,6 +586,7 @@ <h2 id="props_with_no_examples_label">Properties and States with No Guidance or
            <li><code>aria-invalid</code></li>
            <li><code>aria-keyshortcuts</code></li>
            <li><code>aria-multiline</code></li>
+            <li><code>aria-orientation</code></li>


There is a a new bug here because aria-orientation does have an example in slider

mcking65 · 2021-01-18T15:59:43Z

coverage/index.html

-</td>
-          </tr>
-          <tr>
-            <td><code>aria-orientation</code></td>


This is the other side of the aria-orientation bug; it should not have been removed from this list.

mcking65 · 2021-01-18T16:34:30Z

examples/index.html

              </ul>
            </td>
          </tr>
          <tr>
            <td><code>aria-labelledby</code></td>
            <td>
              <ul>
-                <li><a href="checkbox/checkbox-1/checkbox-1.html">Checkbox  (Two State)</a></li>


This appears to be a bug; aria-labelledby is used and documented in the checkbox-1 example.

mcking65 · 2021-01-18T16:40:11Z

examples/index.html

@@ -652,17 +628,12 @@ <h2 id="examples_by_props_label">Examples By Properties and States</h2>
            <td><code>aria-multiselectable</code></td>
            <td><a href="listbox/listbox-rearrangeable.html">Listboxes with Rearrangeable Options</a></td>
          </tr>
-          <tr>
-            <td><code>aria-orientation</code></td>
-            <td><a href="slider/slider-2.html">Slider  with aria-orientation and aria-valuetext</a></td>


Same aria-orientation bug mentioned above.

@mcking65
@carmacleod
I will update the indexing to use the regex suggestion and see if that fixed the issue with aria-orientation.

carmacleod · 2021-01-18T16:57:39Z

@jongund In case it's helpful, here's a cheat sheet I use for regex: http://www.rexegg.com/regex-quickstart.html#ref
I can never remember what characters to use. ;)

jongund · 2021-01-18T22:59:40Z

@mcking65
@carmacleod
Use the regex suggested by Carolyn and it seems it fixed the problems with aria-orientation bug.

mcking65

Jon, this is looking better still.

Wow, isn't this devilishly difficult to fix/review? I notice a couple of places where the previous version fixed errors that are no longer getting fixed.

Previous version fixed list of examples using aria-hidden; it removed Alert Dialog, which does not use aria-hidden.
Previous version fixed list of examples using aria-posinset and aria-setsize; it removed File Directory Treeview Using Computed Properties, which do not use those properties.

I also noticed some other places where attributes are being overlooked by the algorithm.

The aria-current index is missing Breadcrumb Example.
The aria-modal index is missing Modal Dialog Example.
The aria-selected index is missing:

Date Picker Combobox Example
Select-Only Combobox Example.
Collapsible Dropdown Listbox Example
Listbox Example with Grouped Options
Scrollable Listbox Example

jongund · 2021-01-19T02:46:26Z

@mcking65
The aria-current issues is because the breadcrumb example is in a file named index.html. The current script ignores files name index.html. I think the breadcrumb example should be renamed to breadcrumb.html or could make the script only ignore the index.html file in the examples directory..

carmacleod · 2021-01-19T03:39:12Z

Renaming the breadcrumb example to breadcrumb.html makes sense for another reason as well: the open in codepen button uses the filename to set a title for the codepen, and currently this is "index" for the breadcrumb example (which isn't a very good title).

mcking65 · 2021-01-19T06:12:19Z

@jongund
I'm fine with including the breadcrumb file rename in this PR.

jongund · 2021-01-19T20:56:03Z

@mcking65
I think I have fixed the problems with the reference index and the coverage report, aria-current is now referencing both treeview navigation and breadcrumb examples..
I also renamed index.html file in the breadcrumbs directory to breadcrumbs.html
Please review and let me know if there ae still problems.

mcking65 · 2021-04-06T17:45:23Z

Per discussion in March 30 meeting, Sarah will assist with a code review and help ensure it is robustly supporting our desired indexing behaviors.

I thought we had those behaviors documented in the wiki; I can't find it. I believe they should be part of the infrastructure documentation. We should have documentation of:

What we expect to get indexed, e.g., what authors need to do to ensure a section or page is listed in the index for a specific topic.
2.Where the index generation code is and when/how it is run.

smhigley

There's one bug that should be updated, but otherwise it looks OK. Looking at the logic though, I wonder if we should consider moving to a node html parser in the future, so we're not trying to process html strings ourselves.

I don't want to hold up this PR, but it might be a good backlog item to look into converting this all to use something like this library: https://www.npmjs.com/package/node-html-parser

smhigley · 2021-04-09T17:11:35Z

coverage/index.html

        <li><a href="../examples/checkbox/checkbox-1/checkbox-1.html">Checkbox  (Two State)</a></li>
+        <li><a href="../examples/coding-template/Depricated-MultipleImplementationExample-Template.html">EXAMPLE_NAME</a></li>


This file (and the coding-template directory) should be excluded

jongund · 2021-04-09T18:00:39Z

@smhigley
Thank you for finding the bug, I will look at the parser, it looks like the right way to go.

smhigley · 2021-04-09T18:17:07Z

@jongund awesome! I'm also 100% in favor of merging this PR now, and letting that be a future item, though 😄

jongund · 2021-04-09T20:27:09Z

@smhigley
Can you re-review I update the reference and coverage scripts to use node-html-parser. Makes the code alot cleaner and easier to read!

jongund · 2021-04-12T16:23:09Z

@mcking65 @smhigley @carmacleod
I updated the reference and coverage report scripts based on Sara's feedback on using node-html-parser, which made the code much cleaner. Adding the high contrast was only a few lines of code, so to speed up merging I just added it to this pull request. No longer using the title attribute on the link to provide high contrast information to screen readers, using aria-label instead. So if people can re-review I think it is ready to merge.

@nschonni
NOTE: Not sure why one of the regression tests is failing, since I did not change any examples. I tried re-runing the job, but it still fails. This seems to happen when pull requests languish for long periods of time.

jongund · 2021-04-14T15:29:22Z

Pull #1859 superceeds this pull request.

jongund added 2 commits December 29, 2020 13:36

fixed bug with indexing aria-label, it also got references to aria-la…

b0b702f

…belledby

fixed bug in coverage report too

d206684

jongund changed the title ~~fixed bug with indexing aria-label,~~ fixed bug with indexing aria-label property Dec 31, 2020

mcking65 requested changes Jan 18, 2021

View reviewed changes

jongund added 2 commits January 18, 2021 13:11

Merge branch 'master' into fix-bug-index-generator

56e5084

replaced indexOf with regex

5bf2200

mcking65 requested changes Jan 19, 2021

View reviewed changes

jongund added 2 commits January 19, 2021 14:50

updated algorithms for searching for roles, properties and states

4c17041

removed index.html for breadcrumbs

6cabaa3

updated console output for references and coverage

921dc02

Base automatically changed from master to main March 3, 2021 18:13

mcking65 mentioned this pull request Apr 6, 2021

updated indexes to include high contrast support information #1607

Closed

mcking65 requested a review from smhigley April 6, 2021 17:36

smhigley reviewed Apr 9, 2021

View reviewed changes

jongund added 2 commits April 9, 2021 12:49

ignoring depricated template files

5290cad

fixed spelling mistake in deprecated file name and in JS

b091bc6

jongund added 2 commits April 9, 2021 15:19

fixed bugs

739e696

updated package files to include node-html-parser

bb08117

jongund requested a review from smhigley April 9, 2021 20:27

added high contrast information

a4ae965

jongund changed the title ~~fixed bug with indexing aria-label property~~ fixed bug with indexing aria-label property and added high contrast information Apr 12, 2021

jongund added 5 commits April 12, 2021 10:41

fixed linting error

4cfd0e7

merged main to this branch

7741f44

updated aria-practices.html

9a0e9e0

resolving unwanted changes to aria-practices.html

88a7cdc

updated aria-practices.html to resolve conflicits

3b66145

updated npm

086862e

jongund closed this Apr 14, 2021

zcorpan deleted the fix-bug-index-generator branch May 31, 2021 12:27

zcorpan mentioned this pull request May 31, 2021

meta: Cleaning up old branches #1552

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixed bug with indexing aria-label property and added high contrast information #1709

fixed bug with indexing aria-label property and added high contrast information #1709

jongund commented Dec 29, 2020 •

edited by pr-preview bot

carmacleod commented Dec 30, 2020 •

edited

jongund commented Jan 14, 2021

mcking65 left a comment •

edited

mcking65 Jan 18, 2021

mcking65 Jan 18, 2021

mcking65 Jan 18, 2021

mcking65 Jan 18, 2021

jongund Jan 18, 2021

carmacleod commented Jan 18, 2021

jongund commented Jan 18, 2021

mcking65 left a comment

jongund commented Jan 19, 2021

carmacleod commented Jan 19, 2021

mcking65 commented Jan 19, 2021

jongund commented Jan 19, 2021

mcking65 commented Apr 6, 2021

smhigley left a comment

smhigley Apr 9, 2021

jongund commented Apr 9, 2021

smhigley commented Apr 9, 2021

jongund commented Apr 9, 2021

jongund commented Apr 12, 2021

jongund commented Apr 14, 2021

		<li><a href="../examples/checkbox/checkbox-1/checkbox-1.html">Checkbox (Two State)</a></li>
		<li><a href="../examples/coding-template/Depricated-MultipleImplementationExample-Template.html">EXAMPLE_NAME</a></li>

fixed bug with indexing aria-label property and added high contrast information #1709

fixed bug with indexing aria-label property and added high contrast information #1709

Conversation

jongund commented Dec 29, 2020 • edited by pr-preview bot

carmacleod commented Dec 30, 2020 • edited

jongund commented Jan 14, 2021

mcking65 left a comment • edited

Choose a reason for hiding this comment

mcking65 Jan 18, 2021

Choose a reason for hiding this comment

mcking65 Jan 18, 2021

Choose a reason for hiding this comment

mcking65 Jan 18, 2021

Choose a reason for hiding this comment

mcking65 Jan 18, 2021

Choose a reason for hiding this comment

jongund Jan 18, 2021

Choose a reason for hiding this comment

carmacleod commented Jan 18, 2021

jongund commented Jan 18, 2021

mcking65 left a comment

Choose a reason for hiding this comment

jongund commented Jan 19, 2021

carmacleod commented Jan 19, 2021

mcking65 commented Jan 19, 2021

jongund commented Jan 19, 2021

mcking65 commented Apr 6, 2021

smhigley left a comment

Choose a reason for hiding this comment

smhigley Apr 9, 2021

Choose a reason for hiding this comment

jongund commented Apr 9, 2021

smhigley commented Apr 9, 2021

jongund commented Apr 9, 2021

jongund commented Apr 12, 2021

jongund commented Apr 14, 2021

jongund commented Dec 29, 2020 •

edited by pr-preview bot

carmacleod commented Dec 30, 2020 •

edited

mcking65 left a comment •

edited