这是indexloc提供的服务,不要输入任何密码
Skip to content

Conversation

@dalf
Copy link
Contributor

@dalf dalf commented Feb 12, 2022

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:

  • some focusable element are not hightlighted for example.
  • the "Skip to content" link is missing because it requires to change the CSS.

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 fieldset HTML element in the preferences:

Related issues

Related to #818

@return42 return42 requested a review from mrpaulblack February 17, 2022 16:11
@dalf dalf force-pushed the simple-aria-1 branch 2 times, most recently from 260296e to afe7107 Compare February 26, 2022 12:49
@dalf dalf mentioned this pull request Feb 27, 2022
Closed
@dalf
Copy link
Contributor Author

dalf commented Mar 13, 2022

ping @mrpaulblack

another PR is required to update the tabs in the preferences:

Copy link
Member

@return42 return42 left a 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.

@mrpaulblack
Copy link
Member

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 dalf merged commit b692035 into searxng:master Mar 19, 2022
@dalf dalf deleted the simple-aria-1 branch March 19, 2022 11:00
@mrpaulblack
Copy link
Member

@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...

@dalf
Copy link
Contributor Author

dalf commented Mar 19, 2022

That's the expect behavior of a reset button

From https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/reset :

elements of type reset are rendered as buttons, with a default click event handler that resets all of the inputs in the form to their initial values

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);
   }

@mrpaulblack
Copy link
Member

only happens with the non JS version tho;
I am reading: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/Input maybe there is some attribute for input to get around this?

@dalf
Copy link
Contributor Author

dalf commented Mar 19, 2022

<input type="search" may include a clear button (on Chrom(e|ium) browser, but not on FF).

mrpaulblack added a commit that referenced this pull request Mar 19, 2022
[simple theme] fix small CSS issue from #894
@mrpaulblack
Copy link
Member

mrpaulblack commented Mar 19, 2022

@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

@dalf dalf mentioned this pull request Mar 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants