-
Notifications
You must be signed in to change notification settings - Fork 8.4k
[Discover][A11y] Add underline to search result highlights #228086
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
base: main
Are you sure you want to change the base?
[Discover][A11y] Add underline to search result highlights #228086
Conversation
Pinging @elastic/kibana-accessibility (Project:Accessibility) |
Pinging @elastic/kibana-data-discovery (Team:DataDiscovery) |
src/platform/packages/shared/kbn-unified-data-table/src/components/data_table.scss
Outdated
Show resolved
Hide resolved
…kowalska622/kibana into discover-a11y-search-result-highlights
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good overall!
Unfortunately no good deed goes unpunished in Kibana, and this causes a weird issue in the summary columns for o11y profiles where the badge values disappear 🫠
highlight.mp4
Luckily I had an idea of why this was happening since I knew they were doing some fancy stuff with highlights to support those badges... Looks like we probably just need to update some regex here to account for attributes on mark
tags. I'd recommend trying to make it generic, i.e. ignore any attributes, to protect against any similar changes in the future:
Lines 10 to 18 in c726ced
function extractTextAndMarkTags(html: string) { | |
const markTags: string[] = []; | |
const cleanText = html.replace(/<\/?mark>/g, (match) => { | |
markTags.push(match); | |
return ''; | |
}); | |
return { cleanText, markTags }; | |
} |
src/core/packages/chrome/layout/core-chrome-layout/layouts/common/global_app_styles.tsx
Outdated
Show resolved
Hide resolved
src/platform/plugins/shared/field_formats/common/utils/highlight/html_tags.ts
Outdated
Show resolved
Hide resolved
@davismcphee wow, thank you! This shows how important is a review from someone more experienced with the product, I wouldn't even know it can break like this |
…kowalska622/kibana into discover-a11y-search-result-highlights
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New regex matches any additional props added to mark
tag, before closing >
.
It fixes a bug which was introduced with a new styling class added to mark
- current regex was incorrectly recognizing tags and clean text
💛 Build succeeded, but was flaky
Failed CI StepsMetrics [docs]Async chunks
Page load bundle
History
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates! Works great with the latest changes, and I confirmed the issue with the summary badges is now resolved 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shared UX LGTM
Summary
Closes #214592
This PR adds
text-decoration: dotted underline
to search result highlights due to accessibility reasons.Checklist
Check the PR satisfies following conditions.
Reviewers should verify this PR satisfies this list as well.
release_note:breaking
label should be applied in these situations.release_note:*
label is applied per the guidelinesbackport:*
labels.Identify risks
Does this PR introduce any risks? For example, consider risks like hard to test bugs, performance regression, potential of data loss.
Describe the risk, its severity, and mitigation for each identified risk. Invite stakeholders and evaluate how to proceed before merging.