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

Feed Example: Convert images to inline SVG #2729

Merged
merged 9 commits into from
Jul 24, 2023

Conversation

frozenzia
Copy link
Contributor

@frozenzia frozenzia commented Jun 21, 2023

Fixes #2628
Possibly also w3c/wai-aria-practices#190 ? Was not clear to me the relation between aria-practices and wai-aria-practices.

Once this is OK, need to remember to deal with issue w3c/wai-aria-practices#225. Again, not sure about the relation, sorry if it shouldn't be mentioned here.

Ah, couple of points.

  1. as suggested, I used radio-rating.html as "inspiration" for this fix. The CSS for that file included the following bits:
.restaurant-rating svg {
  forced-color-adjust: auto;
  touch-action: pan-y;
}

.restaurant-rating svg .star-none {
  stroke-width: 3px;
  stroke: #f8951d;
  fill-opacity: 0;
}

..., which I did not include / copy for the feed example, as they didn't seem necessary. But supposing that I've misunderstood something, I could be wrong about that.

  1. Also just realized I've left .restaurant-star-img selector in the CSS and it's completely unnecessary.

Preview feed example in compare branch

Preview of feed example display page

Review checklist


WAI Preview Link (Last built on Wed, 28 Jun 2023 07:48:36 GMT).

Just happened upon this one - looks like someone's copy-pasting had not
been completely finished, and comments still said "HOME", though they
clearly were meant to say "END".
Testing that focus moves up, not down.
Testing that focus moves to first, not last element in row.
CONTROL+A is used in this test to select all text in a textarea. But on
a mac that key combo does nothing in a textarea. Instead COMMAND+A
needs to be used.

Copied this solution from `alertdialog_alertdialog.js`.
Apparently this code as-is breaks automatically run tests, possibly
because in this test (vs. alertdialog test) we await the results,
and I suspect the `Key.COMMAND` doesn't return anything in a non-mac
environment. 🤷🏽‍♀️
@frozenzia frozenzia force-pushed the issue2628-broken-star-images branch from 0b367aa to 2084eab Compare June 22, 2023 18:14
@frozenzia frozenzia marked this pull request as ready for review June 22, 2023 18:21
@mcking65 mcking65 changed the title Issue2628 broken star images Feed Example: Convert images to inline SVG Jun 26, 2023
@daniel-montalvo daniel-montalvo self-requested a review June 27, 2023 18:16
@css-meeting-bot
Copy link
Member

The ARIA Authoring Practices (APG) Task Force just discussed PR2729: Feed Example: Convert images to inline SVG by frozenzia.

The full IRC log of that discussion <jugglinmike> Subtopic: PR2729: Feed Example: Convert images to inline SVG by frozenzia
<jugglinmike> github: https://github.com//pull/2729
<jugglinmike> Matt_King: We need visual review. I've partially tested with a screen reader using two browsers, but we should probably have additional verification from an accessibility standpoint (mostly the contrast). We also need someone to look at the code
<jugglinmike> dmontalvo: I can double-check this with a screen reader in Safari
<jugglinmike> Siri: if this can wait until the week of July 10, then I can review visually
<jugglinmike> Andrea_Cardona: I can review the code
<jugglinmike> Matt_King: Great. You're all assigned. Thank you!
<jugglinmike> jamesn: Heads up: I think the high-contrast is not right

@jnurthen
Copy link
Member

IMO the SVG should recolour based on high contrast mode. It is not currently doing this.

@frozenzia
Copy link
Contributor Author

@jnurthen, alright, I just tried to mimic the existing images. I'll look into "high contrast mode" and try to come up w/better colors.

@frozenzia
Copy link
Contributor Author

@jnurthen, understand your comments better now that I learned what "high contrast mode" even is. Latest commit should fix it if I've understood things right. I don't have a Windows machine to test on, and actually Mac's "Invert colours" does not seem to trigger the media-query (for "forced-colors") I added, but is that okay? It does work as intended if I use Chrome and "Rendering" settings to manually emulate "forced-colors". If I've understood properly, that should then also work for Windows High Contrast Mode.

Copy link
Contributor

@a11ydoer a11ydoer left a comment

Choose a reason for hiding this comment

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

The code change looks good to me.

@a11ydoer a11ydoer requested a review from jongund July 11, 2023 18:15
@css-meeting-bot
Copy link
Member

The ARIA Authoring Practices (APG) Task Force just discussed Feed Example: Convert images to inline SVG.

The full IRC log of that discussion <jugglinmike> Subtopic: Feed Example: Convert images to inline SVG
<jugglinmike> github: https://github.com//pull/2729
<jugglinmike> Matt_King: James provided some good feedback to the author, but James is now on vacation. We would like someone to look at this, in particular, high-contrast mode
<jugglinmike> Jem: I reviewed it just now, but I didn't review for high-contrast mode
<jugglinmike> jongund: I can look at high-contrast mode
<jugglinmike> Matt_King: We'd ask Shirisha and Alyssa to review, previously
<jugglinmike> Jem: I've assigned you as a review, jongund

Copy link
Contributor

@jongund jongund left a comment

Choose a reason for hiding this comment

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

High contrast mode on windows works great on the inline SVG images.

Copy link
Contributor

@andreancardona andreancardona left a comment

Choose a reason for hiding this comment

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

Code review looks good

@mcking65 mcking65 merged commit 1d85d95 into w3c:main Jul 24, 2023
@mcking65
Copy link
Contributor

Thank you @frozenzia!!

@mcking65 mcking65 added the enhancement Any addition or improvement that doesn't fix a code bug or prose inaccuracy label Jan 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Any addition or improvement that doesn't fix a code bug or prose inaccuracy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feed Display Example Broken Star Images
7 participants