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

Update vnujar filtering to ignore no alt on role=img, again #1468

Merged
merged 7 commits into from
Oct 22, 2020
Merged

Conversation

spectranaut
Copy link
Contributor

@spectranaut spectranaut commented Jul 23, 2020

There are more errors no on "multithumb-slider" related to the alt text, I update the regex to filter out all of the alt+img related errors. I wonder if we should open a bug on vnu-jar? Or are our examples actually not valid html?

Also this was fixed recently here: #1354

@spectranaut
Copy link
Contributor Author

@nschonni can you review :) ?

.vnurc Outdated Show resolved Hide resolved
@@ -4,7 +4,7 @@
# Proposed, tracking in gh-429
Bad value “” for attribute “aria-activedescendant” on element “ul”:.*
Copy link
Contributor

Choose a reason for hiding this comment

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

issue 429 is closed, and the related validator issue is closed too. Can we remove this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like there are other hits now, so that issue is re-opened

@mcking65
Copy link
Contributor

mcking65 commented Sep 2, 2020

Do I have this right ... when updating vnu.jar from 20.3.16 to 20.6.30, the new version of vnu.jar finds errors related to the alt attribute on elements with role img that the prior version did not find?

Exactly what are the errors?

Do we have an open issue related to these errors to either fix them in the APG example or to track an open issue against the validator?

@nschonni
Copy link
Contributor

nschonni commented Sep 2, 2020

@mcking65 there is an issue from @spectranaut here #1467 that has the new failure messages

@spectranaut
Copy link
Contributor Author

I can't actually replicated this bug anymore after re-running npm install

@spectranaut spectranaut closed this Sep 3, 2020
@nschonni
Copy link
Contributor

nschonni commented Sep 3, 2020

I was seeing it with the package bump here nschonni#8

@spectranaut
Copy link
Contributor Author

Wow I'm really not seeing clearly today. I thought I was at the latest version of vnu-jar locally but I wasn't

@spectranaut spectranaut reopened this Sep 4, 2020
.vnurc Outdated Show resolved Hide resolved
@nschonni
Copy link
Contributor

nschonni commented Sep 4, 2020

Wow I'm really not seeing clearly today. I thought I was at the latest version of vnu-jar locally but I wasn't

No worries, I know @sideshowbarker sometimes turns around patches pretty quick, so I checked to see that wasn't the case too :)

.vnurc Outdated Show resolved Hide resolved
@sideshowbarker
Copy link

No worries, I know @sideshowbarker sometimes turns around patches pretty quick, so I checked to see that wasn't the case too :)

Is there some pending patch I haven’t merged? And that’s causing problems?

@mcking65
Copy link
Contributor

mcking65 commented Sep 4, 2020

In the multi-thumb slider, we have img elements with aria-label. If that is not valid, we need to change those to use alt. I didn't know that it is invalid to use aria-label instead of alt on img elements.

@sideshowbarker is that a recent change? In ARIA, aria-label is global. And, in ARIA, aria-label is supported on elements with the img role. So, whouldn't it be supported on elements with an implied img role?

@sideshowbarker
Copy link

@sideshowbarker is that a recent change?

I don’t know — I’d need to see the exact error or warning messages the HTML checker is emitting. I don’t see any actual error messages cited in this issue.

In ARIA, aria-label is global. And, in ARIA, aria-label is supported on elements with the img role. So, whouldn't it be supported on elements with an implied img role?

I don’t know. I’m happy to test with any actual markup for which the HTML checker doesn’t seem to be doing the right thing.

I do know that a while back (last year, I guess), I added a discretionary warning for aria-label on elements that the ARIA working group has been considering to disallow it for. Maybe that warning is what’s being run into in this case?

@mcking65
Copy link
Contributor

mcking65 commented Sep 5, 2020

@sideshowbarker

The errors are listed in #1467. We get the following errors on the multithumb slider example.

  • An “img” element with no “alt” attribute must not have a “role” attribute.
  • An “img” element with no “alt” attribute must not have any “aria-*” attributes other than “aria-hidden”.
  • An “img” element must have an “alt” attribute, except under certain conditions. For details, consult guidance on providing text alternatives for images.

spectranaut and others added 2 commits September 8, 2020 16:39
Co-authored-by: Nick Schonning <[email protected]>
Co-authored-by: Nick Schonning <[email protected]>
@spectranaut
Copy link
Contributor Author

fixed those new comments, @nschonni ! :)

@nschonni
Copy link
Contributor

@mcking65 this is the PR that will fix the vnu-jar version, and includes the config fixes

@nschonni
Copy link
Contributor

@mcking65 this can land before my other PR to switch to Actions

@mcking65
Copy link
Contributor

mcking65 commented Oct 13, 2020

Unfortunately, as a screen reader user, It's really difficult to use the github actions output on the checks tab when trying to find what failed. In the navigation where it lists all the checks, it's really hard to find the one that failed. I think I figured out that the button for expanding has a badge with the number of actions with failures. It's extremely difficult to hear that badge; the label doesn't say it is the numbering of failing actions. Even harder is reading the log. It seems to be in a scrolling div of some kind and the scrolling doesn't work with a screen reader. Since the output is TAP, I used the search box to search for "not OK". That helped get the right content on the screen, but it was difficult to read.

All I figured out from the log is there is some kind of webdriver error.

@nschonni
Copy link
Contributor

@mcking65 yeah, it looks like an unrelated regression test timeout issue. I think you might have an option to "Re-run check" for any failing tests, but I think this is OK to land either way.

There is a separate issue for seeing if we can add those inline failures like the JS/CSS/Spelling checks over here #1521

@nschonni
Copy link
Contributor

Horray! All green again

@spectranaut
Copy link
Contributor Author

@mcking65 it was an webdriver error... I think the answer really is to rerun the checks when that happens, but it's frustrating that it is so hard for you to get through to the content of the error from the github checks. I just wrote back to the internal Github Support tracker linking to your comment, for what it's worth!

@mcking65 mcking65 merged commit a9bb774 into master Oct 22, 2020
@zcorpan zcorpan deleted the issue1467 branch October 29, 2021 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants