Skip to content
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

Fix case-sensitive path matching for the search box #3249

Merged
merged 4 commits into from
May 2, 2023

Conversation

LeonardYam
Copy link
Contributor

Description

Currently, navigating to a file-path with uppercase characters like /encoding/encodeInto.any.html does not work.
Somewhat related issue: #3009

Review Information

Enter any file-path which contains upper-case characters into the search-box.

Local build vs production:

2023-04-06.19-48-58.mp4

Changes

matches = matches
          .filter(p => p.toLowerCase())
          .filter(p => p.includes(query))

Auto-complete options are generated from matches. I believe the intention here was to map all paths to a lower-case string and perform query matching. However, converting all paths to lowercase would cause further issues later (since the file-path is case-sensitive). Thus, the change was to perform case-insensitive query matching while keeping case-sensitive paths.

if (autocompleteSelection.value === path) {
  this.dispatchEvent(new CustomEvent('autocomplete', {
    detail: {path: path},
  }));

path refers to queryInput which is the text within the search-box. Currently, queryInput is being converted to lowercase and hence the equality-check fails for any path which contains uppercase characters. The path passed into detail should also be changed to use the case-sensitive value in autocompleteSelection.

Notes

I am unable to test whether these changes affects complex queries due to #2941. This would also mean that fixes on #2980 and other bugs would be unable to proceed.

An alternative solution to this would be to ensure queryInput does not get lowercased, however due to the above issue, keeping it lowercase for now would be the safer option.

Copy link
Contributor

@DanielRyanSmith DanielRyanSmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was having trouble replicating this issue, but then I noticed you're using Firefox as your browser to test the site. This seems to be a Firefox-specific problem, as it doesn't trigger the handleChange() method after selecting from the search box options (which I assume you trigger instead in the video by clicking out of the search box). This problem is caused because the query is changed to lowercase in the search box before handleChange() is called.

This PR in its current form breaks the implementation on Chrome and Safari, but after playing around, the suggested changes below should work on all browsers.

I've submitted some changes that break other browsers' view on this site before, so I have some experience. This is a good reminder to test site changes on each of the major browsers, and a reason why the Interop effort is so important. 🙂

Thanks for identifying a problem and bringing forward a solution @LeonardYam!

webapp/components/test-search.js Show resolved Hide resolved
webapp/components/test-search.js Outdated Show resolved Hide resolved
webapp/components/test-search.js Outdated Show resolved Hide resolved
LeonardYam and others added 2 commits April 7, 2023 13:28
Add more details to comments

Co-authored-by: Daniel Smith <[email protected]>
@LeonardYam
Copy link
Contributor Author

LeonardYam commented Apr 7, 2023

Ah, thanks for pointing it out, I'll remember to test on Chrome and Safari in the future!

I have committed the relevant changes and this should be ready for merging.

Note:
Will #2941 be resolved anytime soon? I would be willing to pick it up since working on bugs related to search queries requires this to be fixed first.

Copy link
Contributor

@DanielRyanSmith DanielRyanSmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again!

@DanielRyanSmith
Copy link
Contributor

About #2941, I do not think that this will be resolved any time soon, but there are ways to run the search cache locally, although it is intensive in terms of processing power. The instructions for that are in a readme file in a searchcache subfolder

@DanielRyanSmith DanielRyanSmith merged commit 459bee0 into web-platform-tests:main May 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants