Opened 7 years ago
Last modified 22 months ago
#42586 new defect (bug)
Code Editor: Inconsistent results between finding in CodeMirror vs browser
Reported by: | wpress2010 | Owned by: | |
---|---|---|---|
Milestone: | Future Release | Priority: | normal |
Severity: | major | Version: | 4.9 |
Component: | Administration | Keywords: | needs-patch changes-requested |
Focuses: | accessibility | Cc: |
Description
When in the Appearance/Editor, f'rinstance, when editing a child theme's CSS file, the Ctrl-F (Search) no longer works. SOMETIMES it will find a string IF that string is on the VISIBLE portion of the screen. It will NOT find that string in the file if the string is NOT visible.
Sometimes it will not find the string at all - visible or not.
A serious impeiment to the Editor!
Attachments (4)
Change History (55)
#1
@
7 years ago
- Component changed from Editor to General
- Keywords needs-patch added
- Milestone changed from Awaiting Review to 4.9.1
- Priority changed from normal to high
- Severity changed from normal to major
#2
@
7 years ago
Actually, navigating between results can be done with cmd+g
but there's no visual indication of this, like there is with the browser's built-in search.
#3
@
7 years ago
Are you getting any console errors? I'm able to search using the code editor's built in search without issue (though there isn't a way to move to the next result or back, but I don't think it ever had that?)
#4
@
7 years ago
I am not sure what you are referring to here. The "built-in editor" apparently is only called when you click on the link in the stylesheet file when in the Editor - a new message above the editing area that says,
"Did you know?
There’s no need to change your CSS here — you can edit and live preview CSS changes in the built-in CSS editor."
Foolish message, as I AM editing the child theme's CSS file! This new "feature" is just some kind of "dumbing down" of the admin functions, IMHO.
Is there a way to invoke this "built-in CSS editor" in the actual CSS file? I personally think that making styling changes via the theme's "Additional CSS" Customize tab is BAD practice. That's what a child theme is for!
#5
@
7 years ago
Who's owning this for 4.9.1?
Looking at the editor, I worked out that Cmd+f
is search, Cmd+f, Enter
then re-opens the search and advances on to the next result
I'd have not guessed that Cmd+g
is the next search result (Cmd+shift+g
is the previous).
https://codemirror.net/doc/manual.html#addons has a bunch of search related things which might help, but I can't actually tell what if any of those addons are within the minified build we're using.
Sidenote: We're running version 5.29.1-alpha-ee20357
of codemirror, 5.32.0 was released 2 days ago, and there's been 3 releases since it was commited.
#6
@
7 years ago
there is no ui to jump to next or previous "search term" but it highlights all the "search term" and displays it on the scroll bar. PFA
#7
@
7 years ago
@thrijith Is the context of those screenshots a patch which was missed from being attached?
#8
follow-up:
↓ 9
@
7 years ago
@dd32 no it's highlighting the "search term" without any patch in 4.9 but there is no way to to jump from one result to another other than using Ctrl + g
for next and Ctrl + Shift + g
for previous as you mentioned in your comment.
#9
in reply to:
↑ 8
@
7 years ago
Replying to thrijith:
@dd32 no it's highlighting the "search term" without any patch in 4.9 but there is no way to to jump from one result to another other than using
Ctrl + g
for next andCtrl + Shift + g
for previous as you mentioned in your comment.
Ah right okay, I don't get the scrollbar highlighting, perhaps because I'm on Chrome for Mac.
#10
@
7 years ago
Oddly enough, the problem seems to have resolved itself, and now (at least in Firefox /Win 56.0.2) the search works as expected. Ctrl-F to open the Search box at the top left of the Editor screen, and then F3 to advance to next instance of search target.
When I initially reported the lack of expected Search behavior, Ctrl-F did NOT open the Search box.
#11
@
7 years ago
- Milestone changed from 4.9.1 to 4.9.2
The search behaviour actually varies depending on whether your cursor is focused in the code editor or not. If it is, it uses the code editor search functionality, if not the default browser functionality remains.
Punting due to lack of a patch.
#13
@
7 years ago
- Component changed from Customize to General
- Summary changed from Search function in Editor now broken with update to 4.9 to Code Editor: Inconsistent results between finding in CodeMirror vs browder
The only thing I can think of doing here is intercept any “Cmd-F” commands and route them into CodeMirror. This would mean, however, that a user would not be able to do a find to locate a file in the file list, which would be unfortunate. Maybe the solution here would be to show a notice when doing “Cmd-F” when not focused on CodeMirror to prompt the user to focus on CodeMirror to do a find of code.
CodeMirror only loads the current viewport into the DOM, so that is why the browser's find does not match results as it would in a textarea
. This is for performance.
Code editor tickets we're keeping in the General component.
#14
@
7 years ago
- Summary changed from Code Editor: Inconsistent results between finding in CodeMirror vs browder to Code Editor: Inconsistent results between finding in CodeMirror vs browser
#15
@
7 years ago
To have persistent Search, moving to the next position on enter key you need to use alt+f instead ctrl+f.
Here are some keybindings which are used for search/replace functionality.
Ctrl-F / Cmd-F
Start searching
Ctrl-G / Cmd-G
Find next
Shift-Ctrl-G / Shift-Cmd-G
Find previous
Shift-Ctrl-F / Cmd-Option-F
Replace
Shift-Ctrl-R / Shift-Cmd-Option-F
Replace all
Alt-F
Persistent search (dialog doesn't autoclose, enter to find next, Shift-Enter to find previous)
Alt-G
Jump to line
This ticket was mentioned in Slack in #core by jorbin. View the logs.
7 years ago
@
7 years ago
This simply adds Ctrl-F and Cmd-F (keeping . both Mac and PC) bound to findPersistent
as extra keys.
#17
@
7 years ago
This patch also prevents taking over the browsers Ctrl/Cmd-F as it only works if the editor is selected. Might be good to keep the Alt-F as well for users who already expect that.
Also, the behaviour of findPersistent
allows for pressing enter to jump to the next match as well as pressing Cmd/Ctrl-f cycles the matches as well.
This ticket was mentioned in Slack in #core by desrosj. View the logs.
7 years ago
This ticket was mentioned in Slack in #core by williampatton. View the logs.
7 years ago
#22
@
7 years ago
The patch looks fine, some considerations from testing it out though:
We should grab some styles for the active search result, currently all search results, and the active one, get the same yellow color, making it impossible to distinguish which one you are currently observing as the active result, this is especially bothersome as you can't "find backwards" if you go too far.
Another is that the current implementation only works if the CodeMirror window has focus, this is not the default state, so if I pick a file to edit and then instantly hit ctrl/cmd+f to find what i need, I'll still get the browser search. It might be worth considering either auto-focusing the editor, or hijacking the search command on the editor pages only to provide a better user experience.
#23
@
7 years ago
In my quick testing, having a persistent search on Ctrl/Cmd+F is indeed more intuitive than using Alt+F, which doesn't work for me on Windows.
Let's get 42586.patch in (but keep Alt+F just in case) and continue iterating in 4.9.4.
#26
@
7 years ago
- Keywords needs-patch added; has-patch needs-testing removed
- Milestone changed from 4.9.3 to 4.9.4
Let's address comment:22 in 4.9.4.
This ticket was mentioned in Slack in #core by danieltj. View the logs.
6 years ago
#29
follow-up:
↓ 37
@
6 years ago
Hello,
As far as I know, it still needs a patch to address comment:22
4.9.5 beta release is planned for next Tuesday so we'll need a patch in the next few days otherwise it will be punted to 4.9.6 (which is not that bad).
This ticket was mentioned in Slack in #core by audrasjb. View the logs.
6 years ago
This ticket was mentioned in Slack in #core by desrosj. View the logs.
6 years ago
This ticket was mentioned in Slack in #core by desrosj. View the logs.
6 years ago
This ticket was mentioned in Slack in #core by desrosj. View the logs.
6 years ago
#37
in reply to:
↑ 29
@
6 years ago
4.9.8 beta release is scheduled for next Tues (July 17).
Any chance comment:22 will be addressed by then or should be punt to 4.9.9?
#38
@
6 years ago
- Milestone changed from 4.9.8 to Future Release
This needs an owner for it to be milestoned.
#39
@
6 years ago
Looking at what's left to do, see comment:22
I see codemirror uses a cm-searching
CSS class to highlight all the matches but I couldn't find any CSS class used for the current "active" match so I'm not sure this can be done. Anyone have found a way to do it? Otherwise I'm afraid there's nothing we can do and it should be addressed upstream.
Another is that the current implementation only works if the CodeMirror window has focus, this is not the default state, so if I pick a file to edit and then instantly hit ctrl/cmd+f to find what i need, I'll still get the browser search. It might be worth considering either auto-focusing the editor, or hijacking the search command on the editor pages only to provide a better user experience.
I kindly disagree :)
- auto-focusing is an option when there's only one, very specific, task to complete and there are no other possible workflows (e.g. landing in a page or a modal with nothing else then a form to fill in). This is not the case in this page, where there are many possible workflows and auto-focusing the editor would be far from ideal for keyboard users
- native browser functionalities shouldn't be "hijacked" unless there's a really, really, good reason to do that. Making the
Alt+F / Ctrl/Cmd+F
search limited to the editor area would not allow to search in the rest of the page: as a used, I would like to search, for example, in the list of files in the sidebar (which can be pretty long) or in any other part of the page
@SergeyBiryukov @Clorith given the above, not sure this ticket should stay open :) thoughts?
This ticket was mentioned in Slack in #accessibility by rianrietveld. View the logs.
6 years ago
#41
@
6 years ago
It seems, from a quick glance, that CodeMirror expects you to style selected text for this, as it appears to selected the "active" text.
I understand the disagreement with the find feature though, but I'd like to make a counter-argument here that the editing of files is the primary focus, and that the files list on the side isn't very search friendly if files are in directories (as they will be in many cases). It just felt "unnatural" that I can't just search to find what i need in the code editor right away without extra interactions, but I will admit that I have limited experience with these use cases, and will defer to your experience here.
This ticket was mentioned in Slack in #accessibility by joedolson. View the logs.
5 years ago
This ticket was mentioned in Slack in #accessibility by audrasjb. View the logs.
5 years ago
This ticket was mentioned in Slack in #accessibility by afercia. View the logs.
5 years ago
This ticket was mentioned in Slack in #accessibility by audrasjb. View the logs.
5 years ago
#48
@
5 years ago
- Keywords close added
- Milestone changed from 5.3 to Future Release
Given this ticket still needs some discussions, has no owner and still needs patch or decision, I'm moving it to Future release.
Also adding close
keyword as it appears the ticket should probably be fixed upstream in Code mirror. Leaving it open for the moment so discussion could continue in the ticket.
Cheers,
Jb
This ticket was mentioned in Slack in #accessibility by audrasjb. View the logs.
5 years ago
#50
@
22 months ago
Test Report (Regression)
The purpose of this test is to provide current context of the issue.
Environment
- Hardware: MacBook Pro Apple M1 Pro
- OS: macOS 12.6
- Browser: Safari 16.0, Google Chrome 107.0.5304.110
- Server: nginx/1.23.2
- PHP: 7.4.32
- WordPress: 6.2-alpha-54642-src
- Theme: twentytwentyone v1.7
Actual Results
- ✅ When using
Cmd-F
to search, pressingReturn
keeps the search box visible. - ❌ When using
Cmd-F
to search, the current "focused" search result is not highlighted differently from other results. PressingReturn
advances the code to the next result, but no special highlighting is applied. (See Figure 1.)
Additional Notes
- After a
Cmd-F
search, pressingESC
to exit the search box removes highlighting except for the "focused" result. - Using
Cmd-G
(andShift-Cmd-G
) instead ofCmd-F
clearly shows the "focused" result in a different color.
Supplemental Artifacts
Figure 1: Patch 42586.patch applied.
#51
@
22 months ago
- Keywords changes-requested added; close removed
- Priority changed from high to normal
Following up on this report. Thank you to everyone who has contributed so far!
To summarize for historical context:
- The original report was that
Ctrl-F
did not work as expected, but was later confirmed as working again. - Subsequently, r42625 added support to press
Return/Enter
after aCtrl-F
search to advance to the next result, while leaving the search box visible. - However, the
Ctrl/Cmd-F
key modifiers introduced in r42625 appear to cause the loss of "focused" search result highlighting described in comment:22. (See comment:50.) - Discussion also followed whether the code editor should hijack the browser's default
Ctrl/Cmd-F
functionality. - It's worth clarifying that since its adoption to core, CodeMirror's default mappings define "Find" as
Ctrl/Cmd-F
, "Find Next" asCtrl/Cmd-G
, and "Find Previous" asShift-Ctrl/Cmd-G
.
Next Steps
Because r42625 introduced the "focused" highlight regression in comment:22, and this update has been around several years (I don't think we can revert at this point), this ticket should remain open until the regression has been addressed. So I'm removing the close
keyword, and marking for changes-requested
.
Also, because this ticket has been reviewed repeatedly in #accessibility, and the high
priority is questioned each time, I've marked as normal
.
Optional Follow-Ups
- If anyone feels strongly that the browser's default Find key mappings should be overridden by the code editor, this should be explored in a new ticket.
- The CodeMirror version used in core is v5.29.1-alpha-ee20357, and hasn't been updated in some time. CodeMirror 5 is currently v5.65.9 (and completely overhauled in version 6).
I just noticed this too. The search (which overrides the built-in browser search) is unusable. It doesn't allow for moving between instances of search results.