-
Notifications
You must be signed in to change notification settings - Fork 2.3k
[enh] simple: basic ARIA fixes #894
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
Conversation
260296e to
afe7107
Compare
|
ping @mrpaulblack another PR is required to update the tabs in the preferences:
|
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.
I tested this PR (roughly / I'm not aware of all ARIA aspects) and I think it is huge step forward --> Lets merge this PR
FYI I rebased on current master.
|
Agreed. Sorry I put the review off again and again so after Markus tested it I am also for merging (Looking at the files the aria labels seems clear IMO 👍 and the reset works) |
|
@dalf After some more testing the new reset impl. does not clear the input filed when the user already searched for something. It will only reset to the search term that the user searched for... |
|
That's the expect behavior of a reset button From https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/reset :
I don't know how to clear the input without JS. Fix for the JS version: diff --git a/searx/static/themes/simple/src/js/main/search.js b/searx/static/themes/simple/src/js/main/search.js
index 6ef95f5b..1fb0a7cb 100644
--- a/searx/static/themes/simple/src/js/main/search.js
+++ b/searx/static/themes/simple/src/js/main/search.js
@@ -31,10 +31,11 @@
// update status, event listener
updateClearButton();
- cs.addEventListener('click', function () {
+ cs.addEventListener('click', function (ev) {
qinput.value = '';
qinput.focus();
updateClearButton();
+ ev.preventDefault();
});
qinput.addEventListener('keyup', updateClearButton, false);
} |
|
only happens with the non JS version tho; |
|
<input type="search" may include a clear button (on Chrom(e|ium) browser, but not on FF). |
[simple theme] fix small CSS issue from #894
|
@dalf so first of all I misunderstood an that issue happens in both JS and non JS version in current master. I tried around for a while in HTML, but looks like this is not really possible since it will always reset to value. If you have time could you please make a PR with the JS fix? I mean this way it at least works with JS and kinda works with no JS 👍 [EDIT] qwant has the same problem without JS: https://www.qwant.com/?q=time |
What does this PR do?
Fix some ARIA issues reported by FF devtools / Accessibility tab.
For reference: https://www.w3.org/TR/wai-aria-practices/examples/landmarks/complementary.html
Why is this change important?
How to test this PR locally?
Author's checklist
I didn't wanted to change the CSS in this PR, but there is a need to do in another PR to fix these issues:
accessibility / a11y is whole universe, and I'm not an expert: I've tried to keep the changes that seems to improve the UX with a screen reader. The idea was to add as few as possible rather too much and counterproductive. However I'm have failed here and there.
I'm not sure the usage of
fieldsetHTML element in the preferences:legendHTML element is not alabeldisplayproperty of thelegendinside afieldset: https://stackoverflow.com/questions/62692452/cant-position-html-legend-tag-with-css-gridRelated issues
Related to #818