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

Adds noTitle option to RelativeTime component #4635

Merged
merged 8 commits into from
Jun 5, 2024

Conversation

lindseywild
Copy link
Contributor

@lindseywild lindseywild commented May 30, 2024

Blocked until a new release of https://github.com/github/relative-time-element ships and is updated in primer/react.

Closes https://github.com/github/accessibility-audits/issues/5633

Changelog

New

Changed

Removed

Rollout strategy

  • Patch release
  • Minor release
  • Major release; if selected, include a written rollout or migration plan
  • None; if selected, include a brief description as to why

Testing & Reviewing

Merge checklist

Copy link

changeset-bot bot commented May 30, 2024

🦋 Changeset detected

Latest commit: 33f9c8d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@primer/react Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

github-actions bot commented May 30, 2024

size-limit report 📦

Path Size
packages/react/dist/browser.esm.js 89.45 KB (+0.02% 🔺)
packages/react/dist/browser.umd.js 89.74 KB (-0.02% 🔽)

@lindseywild lindseywild changed the title WIP Adds noTitle option to RelativeTime component Adds noTitle option to RelativeTime component Jun 4, 2024
@github-actions github-actions bot temporarily deployed to storybook-preview-4635 June 4, 2024 13:39 Inactive
@lindseywild
Copy link
Contributor Author

Integration failures are unrelated to this PR!

@lindseywild lindseywild marked this pull request as ready for review June 4, 2024 14:07
@lindseywild lindseywild requested a review from a team as a code owner June 4, 2024 14:07
Co-authored-by: Clay Miller <clay@smockle.com>
"name": "noTitle",
"type": "boolean",
"defaultValue": "",
"description": "Removes the `title` attribute provided on the element by default"
Copy link
Member

@siddharthkp siddharthkp Jun 5, 2024

Choose a reason for hiding this comment

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

Hi! 👋

For the docs, can you also add when is it recommended to skip title?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this description? Or are you talking about updating the in-progress docs PR?

Copy link
Member

Choose a reason for hiding this comment

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

Ideally both places, but either is fine for now :)

@siddharthkp
Copy link
Member

siddharthkp commented Jun 5, 2024

Hi!

Can i request additional context, what does passing no-title down do? When is it recommended to use + when is it not recommended to use?

Based on https://github.com/github/accessibility-audits/issues/5633, trying to understand if this should be the default behavior or only in specific cases? (which ones?)

@lindseywild
Copy link
Contributor Author

lindseywild commented Jun 5, 2024

@siddharthkp Hi! Good question. Passing no-title tells the relative-time-element not to render a title attribute. title attributes are inaccessible by default, but we can't fully remove it from relative-time-element without a bigger conversation since it's all over GitHub and people use it often. If it helps, I have a PR for updated guidance in the works that I will merge after this one is merged :)

Ideally it'd be default behavior, but we'd need a larger conversation with stakeholders and design to come up with an accessible solution (we've attempted this in Office Hours, but there is nothing straightforward that we can do other than this without extensive testing).

This is also to match the behavior introduced in PVC.

@joshblack
Copy link
Member

Would it be preferable to always have the title not be present, at least from the Primer React side? Internally we could set noTitle on the relative time element but we wouldn't expose it as a prop to consumers to toggle.

Just wanted to ask to see if this would be helpful or if not because of what you shared about usage at GitHub and how there isn't a good alternative yet when the title is not displayed 👀

@lindseywild
Copy link
Contributor Author

Would it be preferable to always have the title not be present, at least from the Primer React side? Internally we could set noTitle on the relative time element but we wouldn't expose it as a prop to consumers to toggle.

Yes, it would be preferable to never have a title attribute! I added a story that shows how we can instead use a Tooltip on interactive elements, but that doesn't solve the case of non-interactive elements :/ If you think it's ok I am totally fine removing the title attribute altogether, but I think that may ruffle some feathers and we'd would probably require user testing first 😅

@siddharthkp
Copy link
Member

siddharthkp commented Jun 5, 2024

If it helps, I have a PR for updated guidance in the works that I will merge after this one is merged :)

Yes, this was very helpful!

Please correct me if I understood this incorrectly:

  • For visual users, the date is presented as "3 days ago" and then they can hover for exact date + time
  • For screen reader users, the date is also presented as "3 days ago", but there is no way to know exact date + time because title is ignored.

(this is where I'm probably understanding incorrectly)

To make both experiences similar, we need to replace title with an accessible element, like a tooltip. By removing title without replacing it with tooltip, we do make the experience the same, but don't we lose the detailed information about data and time?


Ideally it'd be default behavior, but we'd need a larger conversation with stakeholders and design to come up with an accessible solution (we've attempted this in Office Hours, but there is nothing straightforward that we can do other than this without extensive testing).

I see where you are coming from! I feel a little worried about noTitle because I don't know how it or where will be used in the product. Can you share a little more on that.

How do you feel about primer components for RelativeTime (both PRC and PVC) using a tooltip internally instead of expecting it to be added in the product? It would lead to some inconsistency in the short term (between primer vs non-primer relative time), but hopefully will set the direction towards accessible + consistent

@lindseywild
Copy link
Contributor Author

lindseywild commented Jun 5, 2024

@siddharthkp You are mostly correct with how you're understanding it! The other user we're excluding is keyboard users -- even if a user can navigate to the element because it's focusable, like a link, the title attribute does not display for them.

but don't we lose the detailed information about data and time?

No, if we pass in the full date and time as the tooltip (check out the story Link with Tooltip) that shows the full date in the tooltip, but the shortened version as the main text.

I feel a little worried about noTitle because I don't know how it or where will be used in the product. Can you share a little more on that.

Sure! We can think of this as a "baby step" to a fully accessible solution. Since we don't have one right now, this is better than what currently exists, but still not fully ideal. One example of where this can be used in the product is on PRs, where the timestamp acts as a link to that comment. We can use noTitle to not render a title attribute, and instead replace it with a Tooltip with the full time as it is in the example story.

I am also thinking that if audit issues start coming in, similar to the one linked in the PR, it gives teams a way to fix the audit issue with minimal impact or needing to reassign the issue to Primer -- they have a few options of how to address it. They can do one of the following:

  • Pass in noTitle so they don't render a title (and we can let the teams decide if they want to replace it or go without it)
  • For interactive elements, pass in noTitle and pass in the full date/time in a Tooltip
  • For non-interactive elements, pass in noTitle and write as much of the full date/time out as possible

How do you feel about primer components for RelativeTime (both PRC and PVC) using a tooltip internally instead of expecting it to be added in the product?

We considered this! The tricky part is that not all RelativeTime elements are interactive. Some are static text, which we can't render a Tooltip on in an accessible way. We considered having two components, RelativeTime (as-is) and RelativeTimeWithTooltip, but right now consumers wrap the RelativeTime component in a <Link> tag, so it'd be hard to determine in the component what it's parent is and whether or not it should render a Tooltip. Also, consumers may not know the difference between the two. But if you think otherwise, we can explore it.

@siddharthkp
Copy link
Member

siddharthkp commented Jun 5, 2024

Ah, I seeee! Thanks you going into detail, now it's very clear to me.

The tricky part is that not all RelativeTime elements are interactive.

I completely missed this detail!


Summarising for myself first:

Current scenario:

  • RelativeTime renders an inline text element (usually short text like "3 days ago")
  • We add a title attribute to the element which has the full datetime, but is only available to visual users, not to screen reader users

Ideal scenario:

  • Case 1: If RelativeTime (inline text element) is wrapped in an interactive element like Link, the parent element can receive focus and we can add a Tooltip on the parent element with full datetime. To make sure there isn't also a redundant browser tooltip, we need to remove title attribute
  • Case 2: If RelativeTime is standing on it's own without an interactive parent, there is no way to show full datetime consistently between visual users and screen reader users. We need to modify the visual text to show more details. We could keep the title attribute, but it's better to nudge developers to keep the experience consistent

@lindseywild
Copy link
Contributor Author

lindseywild commented Jun 5, 2024

@siddharthkp Everything you said looks good, one slight modification:

"We add a title attribute to the element which has the full datetime, but is only available to visual users, not to screen reader users or keyboard or mobile users"

And regarding:

We could keep the title attribute, but it's better to nudge developers to keep the experience consistent

Correct! Eventually we hope to have a better solution, but this is just one small step to getting there :)

@siddharthkp
Copy link
Member

siddharthkp commented Jun 5, 2024

I am also thinking that if audit issues start coming in, similar to the one linked in the PR, it gives teams a way to fix the audit issue with minimal impact

Very fair, happy to merge this as a temporary "unblocker" for teams.

We can think of this as a "baby step" to a fully accessible solution. Since we don't have one right now, this is better than what currently exists, but still not fully ideal.

Just wanted to note that this kind of change is scary because even though I called it a temporary unblocker, once it's in dotcom, it's not temporary anymore. To remove it from primer, we would need to create a plan to remove it in dotcom first. I'd nudge you to follow up with a discussion post where we can try to find a long term fix.

Copy link
Member

@siddharthkp siddharthkp left a comment

Choose a reason for hiding this comment

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

Approved. See also: #4635 (comment)

Ready to merge whenever you'd like

@lindseywild lindseywild added this pull request to the merge queue Jun 5, 2024
@lindseywild
Copy link
Contributor Author

@siddharthkp Thank you! That totally makes sense. That plan would also need to include a deprecation plan for relative-time-element since it's reliant on that... I can follow up with a Discussion post with the current findings and research we've done :)

Merged via the queue into main with commit bd861cc Jun 5, 2024
30 checks passed
@lindseywild lindseywild deleted the lw/remove-title-from-relative-time branch June 5, 2024 15:44
@primer primer bot mentioned this pull request Jun 4, 2024
@lindseywild
Copy link
Contributor Author

@siddharthkp posted! #4645

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