[terra-clinical-result] Remove <time> tags from ResultTimeHeaderCell #876
Conversation
| @@ -63,7 +62,6 @@ const ResultTimeHeaderCell = (props) => { | |||
| className={timeHeaderCellClassnames} | |||
| > | |||
| <time className={dateClassnames}>{date}</time> | |||
| <VisuallyHiddenText text={'\u00A0'} /> | |||
There was a problem hiding this comment.
Does this not work with the space? I think space was added to have a gap between the date and the time.
There was a problem hiding this comment.
It's not needed anymore I'm pretty sure. If you look at the screen recordings seems like it's reading out fine to me (I could be wrong)
There was a problem hiding this comment.
Probably we can confirm with UX on this if the gap is fine.
There was a problem hiding this comment.
So I noticed after trying to add it back in that it reads out 'space' when using voice over in the semantic table. So when it's there it does fix the JAWs side but it creates an issue on the VO side. We're trying to figure out a solution that will work for both but not sure if one issue is worse than the other from an accessibility standpoint.
There was a problem hiding this comment.
I am not sure if it is a good practice but if we use a 'space' (VisuallyHiddenText text=" ") directly, VO doesn't read it out. Does using space affect any other way?
There was a problem hiding this comment.
To be fair I don't think using visually hidden text like this is best practice anyways. If that works I'm fine with it - did you happen to try that with JAWs as well? I'm having issues spinning up my windows desktop this morning. Maybe @zxeleven can take a look and try on JAWs
There was a problem hiding this comment.
@vinaybhargavar I gave that a try yesterday and while it does fix the issue with VO, it reintroduces the JAWS issue of combining the date and time and reading it incorrectly. Going to play around with not using the VisuallyHiddenText component to see if I can find a solution that works for both.
There was a problem hiding this comment.
Talked with @brooksienoodlesoup, this looks to be a bigger problem. He said:
I think the JAWS version sounds worse than the VO version.
But, I think the problem with this implementation is something other than a needed space. I think I may have inadvertently given a pass to the HTML validation on how you were using the time element in the examples you coded, when in fact it fails validation. That's my bad. We need to use a valid string for date/time in order to validate the HTML code. To test whether a date time string is valid HTML, use the Nu HTML Validator, then pick the "text input" method, and type in some HTML code that looks like the date/time string you want to validate, like this:
<title>Test</title>Dec 20, 2010 21:00
This date/time string fails validation when you check it in the Nu validator. If we are using invalid code, then it is not hard to imagine how a browser/screen reader combo will not read out the contents correctly.
The HTML5 specification lists valid formats for date and time, which will work with the time element. You'll want to make sure that the manner in which the date and time is presented in the HTML matches up exactly to one of the formats listed in the HTML specification for the time element.
If we can't provide date/time info in a way that is approved by the HTML spec, then I don't think we can expect the screen reader output to work correctly.
Looking at the doc for the time element it looks like our dates have to be passed in as yyyy-mm-dd. So in our case we would have to extract the year, month, and day from the date that is passed in and then put it into this format and pass it to the time element instead of the raw date. Our time should be fine as something like hh:mm is standard.
There was a problem hiding this comment.
The above solution of pulling out the year, month, and day is going to be involved and idk if we have the time for that. My other thought was to keep the time tag around the "time" but revert the "date" to use divs again. It seemed to read out the date fine without the time tag, it just won't be semantic. At least accessibility is improved for time (which I believe was the one that sounded worse to start) and we could eventually come back to the date portion if needed. Again, it reads out the date pretty reliably even when using a div - I'd have to go re-test through all the browsers in VO and JAWs to ensure that.
CC: @krycunn
There was a problem hiding this comment.
After a long conversation with Brooks and Aaron right now we have decided to remove the <time> tags entirely and revert back to how this was done before those changes were made.
This is for several reasons
- The way our dates are passed in will not work with
<time>unless we make several changes to how we format those - The
<time>tag is not fully supported by screen readers at this time and may cause differences in how its read between readers and browsers - Because JAWs doesn't support time tags it's still reading out the date (and time? I'd have to double check with Aaron on that) in the same way it was before the addition of the time tag. I think the hope was that eventually JAWs would support it, but as of right now the update didn't change much.
- Voice over didn't particularly benefit from this change, the way it reads out the date and time before and after the time tag change seem to be the same. The difference is there was additional context by adding the time tag but from how it reads out it was already working without the time tag.
With all of the above it seems the cons to using it are larger than the pros, Brooks advises that we should remove the time tags - @scottwilmarth said he trusts what Brooks and we have talked through.
- I tested this branch locally and have added recordings for both the semantic and non-semantic examples. I also tested in edge with VO and it sounds the same. Will have Aaron do some recordings on the JAWs side.
time-tag-removed-non-semantic-example-VO.mov
time-tag-removed-semantic-example-VO.mov
- Also, I tested what it currently sound like with the time tag in main - can confirm that it sounds the same as when it's not using the time tag (minus how it incorrectly reads out the space with the time tag obviously).
time-tag-added-non-semantic-example-VO.mov
time-tag-added-semantic-example-VO.mov
|
@sdadn @sycombs Because we merged Aaron's changes last week after mine it looks like we missed a piece of validation. His changes didn't get checked against the new terra site test for semantic table usage I created for ResultTimeHeaderCell. Nothing threw a WDIO error because it looks the same in the existing non-semantic examples. And because this sprint I'm creating the main site examples for those semantic tables - we didn't have existing wdio tests to catch this issue. Anyways, the problem pretty basic, the date and time were squished together when the component is used in a semantic table. This PR has a simple fix for the styling. @brooksienoodlesoup there isn't much difference to review here, I'm not sure if you want to/need to take a look. |
|
Apologies, in JAWs we're noticing the behavior that the visually hidden text was originally added to address. Will probably need to add it back, looking at it now. |
sdadn
changed the title
|
@zxeleven @vinaybhargavar @sdadn @sycombs this pr has changed quite a bit but is good to be looked at again. The basic run down (if you don't want to read my giant comment thread) is that the Talked with Brooks about this and he confirmed we can remove the time tags entirely and revert to how we were handling date and time previously. |
brooksienoodlesoup
added
⭐ Accessibility Reviewed
⭐ UX Reviewed
and removed
Accessibility Review Required
UX Review Required
labels
|
Tested with JAWS both with and without the time-tag-removed-non-semantic-example-JAWS.movtime-tag-removed-semantic-example-JAWS.movtime-tag-added-non-semantic-example-JAWS.movtime-tag-added-semantic-example-JAWS.mov |
aalexanderj
changed the title
Summary
When merging changes for the most recent release two of our PRs didn't play together properly and it was missed. We are seeing some issues with spacing in the ResultTimeHeaderCell component. When in a semantic table the time and date are squished together instead of being on separate newlines. This only effects the semantic examples.
Issue with semantic table:
Looks fine in non-semantic example:
Update: This lead us to seeing several different issues with how the time tag is working. After a long discussion we have decided to remove it entirely.
What was changed:
Change the time tags back to divs, also remove the hidden text between the two.
Testing
This change was tested using:
Reviews
In addition to engineering reviews, this PR needs:
Additional Details
This PR resolves:
UXPLATFORM-XXXX
Thank you for contributing to Terra.
@cerner/terra
Validation:
Non-semantic
time-tag-removed-semantic-example-VO.mov
Semantic table
time-tag-removed-non-semantic-example-VO.mov