Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
The search icon does not show in High Contrast mode in the latest version of Microsoft Edge. It works fine in IE11, and Firefox.
This icon has been a pain. We should probably convert it from using SVG to using CSS shapes.
Testing instructions
We'll need screenshots of the magnifying glass icon on the following browsers
- Chrome
- Firefox
- Edge
- Ie
- Safari
We'll need screenshots in high contrast mode of the following browsers
- IE
- Edge
- Firefox
Comment | File | Size | Author |
---|---|---|---|
#27 | search-button-hc-olivero.mp4 | 5.55 MB | mherchel |
#27 | 3223281-27.patch | 1.23 KB | mherchel |
#23 | Screenshot_8_17_21__3_22_PM.png | 176.11 KB | mherchel |
#22 | Screenshot_8_17_21__3_24_PM-3.png | 221.49 KB | mherchel |
#22 | Screenshot_8_17_21__3_24_PM-2.png | 228.66 KB | mherchel |
Comments
Comment #2
mherchelPatch attached. Tested on all major browsers including IE, Edge, and FF in high contrast mode.
Comment #3
mherchelAlso opened up this followup issue: #3223332: Olivero primary search button should be initialized with aria-expanded="false" by JS.
Comment #4
mherchelAdding testing instructions
Comment #5
mherchelRemoved a couple of unneeded CSS declarations.
Comment #6
kostyashupenkoThat element name
__🔎
looks... interesting) But i'm worrying about BEM naming at this point. BEM docs says "Names are written in lowercase Latin letters", so i'm not sure if we can let it go. For sure element name should be meaningfulComment #7
mherchelyeah, that's the patch's representation of the 🔎 emoji.
We already have one emoji in Olivero committed :D
https://git.drupalcode.org/project/drupal/-/blob/9.3.x/core/themes/olive...
Comment #8
andy-blumScreenshots attached:
Mac OS:
Windows 10:
Also included is a high contrast wide/narrow version of each Windows 10 browser.
Comment #9
andy-blumComment #10
andy-blumComment #11
mherchelper @lauriii, even though we have one emoji in a CSS selector, we don't want it to become a pattern. Once the selector is changed, we can leave RTBC.
This patch changes the selector.
Comment #12
mherchelI forgot to change the selector in the CSS for the X close icon.
Comment #13
mherchelLooks like the patch had a conflict with the changes in #3226865: Olivero: Button can't contain div element in it.
Updated patch attached.
Comment #14
mherchelAttached patch twice. Hiding one.
Comment #15
mherchelLink to Tugboat preview with patch applied: https://3223281-search-icon-hc-u4w8iagwkub9vitvsywaqwhtf2bfbepu.tugboat.qa/
Comment #16
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer commentedThis comment is gibberish. Is there a unicode character in there? It isn't going to be readable in every developer's text editor or terminal pager. It wasn't readable when I looked at the patch in a Firefox tab.
Suggest we avoid relying on high unicode in comments.
Edit: Oh, I see that the unicode emoji was used in the class name earlier, and it was cleaned up in #11. It's just a case of cleaning it up in the comments too.
Comment #17
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer commentedUpdating title. Forced colours is the preferred term these days. It's what the feature has called in the CSS standardization process, because Windows "High-Contrast" mode is a misnomer; users can also create intentionally low-contrast colour schemes.
Comment #19
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer commentedOn Slack today there was a wide-ranging conversation about icons in forced-colours mode, between @mherchel, @mikemai2awesome, and myself.
TL, DR:
Please hold off from committing this in a hurry. I have some ideas to tinker with the original SVG, so assigning to myself for that. If I haven't reported back in a week or two, feel free to un-assign me and proceed with the approach in the current patch.
Slack conversation log, 2021-08-13
Actual Slack link: https://drupal.slack.com/archives/C2ANFUGGG/p1628879288010000
Comment #20
mherchelFigured this out! Thank you for the additional prodding @andrewmacpherson
No interdiff here, because the approach is new.
Tugboat link with the new patch: https://3223281-search-icon-hc-2-kerxukjgvg5899u6uflgynbtgdvzcn8w.tugboa...
Comment #21
mherchelDid a more testing and the
HighlightText
is not the right keyword. It doesn't work properly in light themes. Changing toButtonText
fixes that issue. I also did some testing and removed the unneeded@supports
and!important
.I updated the code on the Tugboat instance (https://3223281-search-icon-hc-2-kerxukjgvg5899u6uflgynbtgdvzcn8w.tugboa...)
Comment #22
mherchelScreenshots.
MS Edge dark theme:
IE dark theme:
Firefox light theme:
MS Edge light theme:
IE Light theme:
Comment #23
mherchelForgot Firefox dark screenshot.
Comment #24
mherchelDuplicate comment. please ignore
Comment #25
mherchelWe discussed this in Drupal a11y office hours (agenda link)
Relevant part from the notes:
@andrewmacpherson will post a patch showing the techniques that he'd like to use. @mherchel will work on it as needed.
Comment #26
webchickLooks like it's been about a month since that conversation happened, and this issue is the final remaining Olivero stable blocker (apart from general testing), according to #3177296: [META] Make Olivero stable.
Anything the rest of us can do to help move things along? Would it help for someone else to take a stab at making the patch according to the notes in #25, and @andrewmacpherson to review, instead of being on the hook to write it?
Comment #27
mherchelI've reached out to Andrew a couple times, but I'm assuming he's pretty busy (which is totally okay since we're all volunteers here).
New patch attached with video showing that it works. No interdiff since the patch is so small.
Tugboat preview for this patch at https://3223281-search-icon-hc-3-kjl5rx8sqzivbmg2qo0dh8qqplfjtxkb.tugboa...
Comment #28
catchIndentation looks off here?
Comment #29
mherchelYep! You're looking at the compiled CSS there :D
Comment #30
mgiffordThanks for walking me through this @mherchel. Amazing how much work it is to address a buggy browser. Edge is usually pretty good, but..
There are times when the issue needs to be fixed upstream, but it looks like you've got a solution that works. It looks like you should be able to submit a bug here, but apparently not:
https://www.digitala11y.com/how-where-to-report-accessibility-bugs/
Not even sure where it would be on GitHub
https://github.com/orgs/MicrosoftEdge/repositories
Although perhaps because of their A11y Automation Suite, maybe Edge is just the only standards compliant browser (so perhaps everyone else is doing it wrong).
https://github.com/MicrosoftEdge/A11y
Anyways, this looks like a good use of forced-colors. Thanks again for the link to this MS Blog on Edge's high contrast mode.
A lot of work has gone into this, but this appears to be a simple fix to an already really good implementation. Looking forward to seeing it in Core.
Comment #33
lauriiiCommitted c788d3f and pushed to 9.3.x. Also cherry-picked to 9.2.x because Olivero is experimental. Thanks!
Comment #34
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer commentedAha, grand job. The approach in #27 is indeed what I suggested in the accessibility office hours call in #25.