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

Reuse pronunciation fakespot-aria-hint element #15905

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

janbrasna
Copy link
Contributor

@janbrasna janbrasna commented Jan 21, 2025

One-line summary

Only keep the ID fakespot-aria-hint once, in a nav (both old and new), to be referenced from anywhere else on the page.

🐞 Some tweaks had to be added for Gecko in addition to that (WebKit and Blink were happy), see comments.

Significant changes and points to review

  • keeps the "canonical" ID span in the nav heading for the product, that ends up included in most pages
  • adds the same for legacy nav, that may get loaded for some locales instead
  • removes the ID span from includes for home, and from products landing, to not appear twice on the same page with the same ID…
  • these other marked up labelledby occurrences rely on the presence of the element through the nav include and reference it.

Caveat: only works on pages with a nav included. That currently covers all the necessary occurrences; in case of a page without a nav included this element might need to be carried over localy, if referenced.

Also fixes:

  • skipped Fakespot description in launchpad
  • missing punctuation in launchpad

Issue / Bugzilla link

#15530
#15847 (review)

Testing

This obviously doesn't get picked up by TTS so just "speaking the page" won't apply this. You'd have to test with e.g. VoiceOver to get the proper pronunciation.

(turn on sound: …)

Screen.Recording.2025-01-21.at.19.38.41.mov
Screen.Recording.2025-01-23.at.16.02.48.mov

(compare to #15847 (comment) …)

Copy link

codecov bot commented Jan 23, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.29%. Comparing base (f13a56d) to head (317f18a).
Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #15905   +/-   ##
=======================================
  Coverage   79.28%   79.29%           
=======================================
  Files         159      159           
  Lines        8343     8347    +4     
=======================================
+ Hits         6615     6619    +4     
  Misses       1728     1728           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@janbrasna janbrasna marked this pull request as draft January 23, 2025 15:28
Copy link
Contributor Author

@janbrasna janbrasna left a comment

Choose a reason for hiding this comment

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

Testing in FXDE 135 and VoiceOver (macOS 15) uncovered some woes where basically visually-hidden or otherwise repositioned or cropped elements got spoken out of order, so a more hacky approach had to be employed for the texts to make sense.

It worked as expected with other UAs, but for Gecko on OSX+VoiceOver I had to make some rather hacky adjustments 317f18a to get more helpful results, avoiding outputs like this:

Screenshot 2025-01-23 at 17 29 24

If you have better readers or different OS implementations/APIs, you can try checking out the PR at d239d2a if that still positions the punctuation in launchpad in front of the content, and speaks the products landing label only after all of the actual link contents — if the bug is in the OS/API, or the UA side exposing it weirdly…

There are still some things a bit less than optimal, but better than nothing:

(fine on one page, repetitive on another in navigation rotor:)

Screen.Recording.2025-01-23.at.21.11.26.mov

(at the same time only picking the correct label in addition to the literal text content when interacting with the landmark(s) nested:)

Screenshot 2025-01-23 at 17 30 58


Notes for reviewer:

(This looks ugly and hacky as a result, trying to sacrifice the markup for best compatibility and usability.)

Comment on lines +126 to +128
b {
color: transparent;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ugly element yesteryear, semantically redefined for basically no harm these days; picked for brevity of the extra markup.

This also helps unstyled content to make more sense (and fallback UAs as IE), so chose what blends in best, unstyled.

Maybe a note for posterity would help?

Suggested change
b {
color: transparent;
}
b { // punctuation for screen readers
color: transparent;
}

Comment on lines -28 to +29
<a class="m24-c-launchpad-link m24-t-product-fakespot" href="https://www.fakespot.com/{{ utm_params }}" data-cta-text="Fakespot" data-cta-position="product-list" aria-labelledby="fakespot-aria-hint">
<strong class="m24-c-launchpad-title">{{ ftl('m24-home-fakespot') }}</strong>
<a class="m24-c-launchpad-link m24-t-product-fakespot" href="https://www.fakespot.com/{{ utm_params }}" data-cta-text="Fakespot" data-cta-position="product-list">
<strong class="m24-c-launchpad-title" aria-label="fake spot.">{{ ftl('m24-home-fakespot') }}<b>:</b></strong>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why?

  1. Gecko didn't like the label on strong instead of anchor when labelled by another element and wasn't picking it up correctly, either not at all, or only added to the end of the whole link, instead of in place of the name etc.
  2. Gecko didn't like the punctuation outside of the other elements (wrong order, spelled out before the link), so it has to be here — in which case it also needs to include the punctuation of the content it's overriding to make sense when read out loud, so it differs from the ID ref content here.

<h2 class="mzp-c-picto-heading" aria-labelledby="fakespot-aria-hint"><a href="https://www.fakespot.com/{{ referrals }}" data-cta-text="Fakespot" data-cta-position="heading" aria-labelledby="fakespot-aria-hint">{{ ftl('firefox-products-fakespot') }}<span id="fakespot-aria-hint" aria-label="fake spot"></span></a></h2>
<h2 class="mzp-c-picto-heading" aria-labelledby="fakespot-aria-hint"><a href="https://www.fakespot.com/{{ referrals }}" data-cta-text="Fakespot" data-cta-position="heading" aria-label="fake spot">{{ ftl('firefox-products-fakespot') }}</a></h2>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why?

  • depending on the context, whether you're selecting the heading, or selecting a link, or listing those roles, you may get different results; so this has to override both elements (the label for anchor doesn't get used when its content its spoken for heading description…)
  • however Gecko for some reason did wild things when both the recursive elements reference the same description, so using a literal string for one made that better for most cases.

@janbrasna janbrasna marked this pull request as ready for review January 24, 2025 14:39
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.

1 participant