Search keyboard shortcuts hijacked?

Bug
1

Not sure if this is a bug or a feature (it looks new to me anyhow, move accordingly if wrong):

I usually search for content inside posts (e.g. today I wanted to search something about development on macOS inside the first long post). Now it seems that Ctrl+F/CMD+F are mapped to opening the search input set to search current thread. There are some issues/possible improvements:

  • this didn’t work in all threads
  • pressing enter 2 times on same query just hides the results, to get them back, query needs to be changed
  • possible fixes for long posts:
  • search results should be highlighted
  • add ability to fallback to browser search
  • cycle through found words

I hope search didn’t fail me and no thread about this exists yet :slight_smile:

2

This is intentional. You can press CTRL+F twice to get back to the browser built-in search on long topics.

And plenty more topics on the matter
https://meta.discourse.org/search?q=ctrl%2Bf

1 Like
3

Oh, I didn’t realise this before. :worried:

Still, there is a bug with 2xEnter I guess :slight_smile:

4

That is indeed interesting, might be a feature request, what should it do when you press Enter a second time?

This seems to have regressed

5

I guess it should just refresh the results and not hide the search input. This would be a bug fix.

An improvement would be to cycle through the results (a new feature).

6

@tgxworld can you check and make sure search results are highlighted, that this has not regressed?

7

Both bugs are valid and I’ve fixed them in

https://github.com/discourse/discourse/commit/ee58c4cd94d58047ec4096440fa3d6d8c4aadbc9
https://github.com/discourse/discourse/commit/de6ca4f736b91d57b0b0e60f86669a9eb7bece66

5 Likes
10

@eviltrout I was looking into adding an acceptance test but I can’t quite figure out why the search menu closes in test but not in development :thinking:

https://gist.github.com/tgxworld/0a32269987440f7bdeae2df7ee88e98e

2 Likes
11

@tgxworld, this seems to be impacting Notifications. I can’t click on my notifications and get to the post anymore.
https://github.com/discourse/discourse/commit/ee58c4cd94d58047ec4096440fa3d6d8c4aadbc9

Repro Steps
Click on User Profile Avatar, click any notification, fails to navigate to post, throws error

Uncaught TypeError: Cannot read property 'searchContextEnabled' of undefined
    at t.linkClickedEvent (_application-ab1012b….js:44622)
    at _application-ab1012b….js:49145
    at t.value (_application-ab1012b….js:49127)
    at t.value (_application-ab1012b….js:49142)
    at t.click (_application-ab1012b….js:45204)
    at _application-ab1012b….js:44913
    at _application-ab1012b….js:44849
    at t.value (_application-ab1012b….js:49127)
    at s (_application-ab1012b….js:44848)
    at HTMLDocument.<anonymous> (_application-ab1012b….js:44912)
2 Likes
12

Can confirm, affecting me on both Windows (Chrome) and Android 7.1 (Chrome).

13

Fixed in

https://github.com/discourse/discourse/commit/fc52624aac658eef63f5b61bb1846463216d49e6

Our build was broken for awhile so I couldn’t deploy stuff

3 Likes
14

I am not sure off the top of my head, would require a bit more in depth tracing through the app.

Having said that, I am not sure I love defining pretender stuff in the test like that. It discourages reuse across tests in the suite, and you can’t even embed it here because it takes up so much space that I can’t see anything else :stuck_out_tongue:

It would be one thing if it was just a short stub of a response, but if using a fixture I really think it works better in its own file!

2 Likes
15

I guess using fixtures is the easiest way to get our JS acceptance tests going but one of the downsides that I find with fixtures is that they are usually too specific to a single use case and makes it hard to be reuse. In addition, fixtures are so huge that a new developer wouldn’t be able to easily understand why a fixture was added unless given the right context. That is why I prefer to add fixtures at the file level or embedded them within each individual test if you need the same route to return a different fixture. Just some thoughts I have after fighting with acceptance tests quite abit :stuck_out_tongue:

3 Likes