[EuiScreenReaderOnly] Revert left positioning change #5215
Conversation
- due to issues with Kibana Selenium functional tests
- It's unlikely those have Selenium text assertions on them in Kibana, so this is probably(?) safe
cee-chen
force-pushed
the
eui-screen-reader-only-left
branch
from
3d07c74
to
64729cb
Compare
cee-chen
force-pushed
the
eui-screen-reader-only-left
branch
from
64729cb
to
e506d3b
Compare
| &[target='_blank'] { | ||
| // Make the screen reader only text positioned relatively against the link itself | ||
| position: relative; | ||
|
|
||
| .euiScreenReaderOnly { | ||
| left: 0; | ||
| } |
There was a problem hiding this comment.
I'm 50/50 on keeping this in vs. just removing it entirely 🤷 I would be surprised if any Kibana functional tests were asserting on any external link text, so I think this is safe to keep in from that perspective - but I do also think it would be cleaner to require the external icon always for target="_blank" links and simply bake the (opens in a new tab or window) text into the icon aria-label itself (see cee-chen#2 (comment))
There was a problem hiding this comment.
I'm on the fence about this change. If there's a clear benefit in the exception to the wider SR-only rule, let's keep it. Otherwise, I think it could be removed.
I've also been thinking on the benefits and risks of adding the parenthetical to the aria-label attribute. Don't have a good argument either way, so I'll trend toward leaving it as is for now.
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5215/ |
| &[target='_blank'] { | ||
| // Make the screen reader only text positioned relatively against the link itself | ||
| position: relative; | ||
|
|
||
| .euiScreenReaderOnly { | ||
| left: 0; | ||
| } |
There was a problem hiding this comment.
I'm on the fence about this change. If there's a clear benefit in the exception to the wider SR-only rule, let's keep it. Otherwise, I think it could be removed.
I've also been thinking on the benefits and risks of adding the parenthetical to the aria-label attribute. Don't have a good argument either way, so I'll trend toward leaving it as is for now.
| @@ -112,8 +112,8 @@ | |||
| position: absolute; | |||
| // Keep it vertically inline | |||
| top: auto; | |||
| // Chrome requires a left value | |||
| left: 0; | |||
| // Chrome requires a left value, and Selenium (used by Kibana's FTR) requires an off-screen position for its .getVisibleText() to not register SR-only text | |||
There was a problem hiding this comment.
The left: -10000px; rule will create a box that extends off the left of the screen in JAWS and VoiceOver, but does improve the situation with testing and is an existing behavior in EUI. I'd love to hear more about Chrome needing a left: 0; rule -- I hope I haven't been missing something important in the past.
There was a problem hiding this comment.
Sorry, to clarify, Chrome required a left property to be set for the scrolling issue that #5152 fixed, not for screen-reader-only behavior alone. There's a footnote in the PR description about it:
On the subject of browser-specific shenanigans: in FF, I was able to remove the
topandleftpositioning properties totally and the fix worked. However, on Chromium/webkit, justclipalone didn't fix the scrolling issue: I neededleft: -10000pxtoo 😖 The only other place I could find where someone else had this issue was this filed bug, but they seemed to come to the conclusion the issue was with FontAwesome, not Chromium (which I sorta disagree with, but c'est la vie). Maybe someday someone will google around and come across this issue, and the cycle of the internet will be complete :)
|
++, it's for sure not ideal, and I don't think anyone could have predicted Selenium would have such bizarre behavior here 💀 Going to go ahead and merge for upgrade expedience, maybe someone can take a look at this separately sometime in the future! |
+ remove `left` CSS on SR text, IMO it's not doing anything / isn't necessary - see elastic#5215 (review)
Summary
We changed
.euiScreenReaderOnlytoleft: 0in #5152, specifically here.Unfortunately, this change caused Kibana's FTR Selenium
.getVisibleText()to include screen reader text when it previously did not:These functional test failures can be viewed in our Kibana upgrade PR: elastic/kibana#112462
Why revert?
38.0.1release and update our Kibana upgrade to point at that release.left: 0is important. It only affects Safari on VO visually and it has no impact on actual screen reader functionality. I don't think that small visual nice-to-have is worth the developer time being spent right now on fixing Kibana tests/delaying the upgrade.QA
yarn build-packnode scripts/functional_tests --config x-pack/test/functional/config.js --grep='Elasticsearch overview mb'Checklist
- [ ] Check against all themes for compatibility in both light and dark modes- [ ] Checked in mobile- [ ] Checked in Chrome, Safari, Edge, and Firefox- [ ] Props have proper autodocs and playground toggles- [ ] Added documentation- [ ] Checked Code Sandbox works for any docs examples- [ ] Added or updated jest tests- [ ] Checked for breaking changes and labeled appropriately