Closed Bug 1159001 Opened 5 years ago Closed 4 years ago

The find input inside the source editor should select the previous search when being reopened

Categories

(DevTools :: Source Editor, defect, P2)

defect

Tracking

(firefox41 fixed)

RESOLVED FIXED
Firefox 41
Tracking Status
firefox41 --- fixed

People

(Reporter: bgrins, Assigned: zer0)

References

(Blocks 1 open bug)

Details

(Whiteboard: [polish-backlog][difficulty=easy])

Attachments

(1 file, 2 obsolete files)

STR:

Open up the source editor (for example, the style editor on https://www.mozilla.org/en-US/)
Press ctrl+f to find
Type "Open".  See how results are matched
Press Enter to close the find mode
Press ctrl+f again

Expected:

The "Open" text is highlighted to make it easy to type over it and replace that text with something else (as customary for 'find' behavior throughout the browser and other applications)

Actual

The cursor is put at the end of the string
Summary: The find input inside the source editor should select all text when being reopened → The find input inside the source editor should select the previous search when being reopened
Whiteboard: [devedition-40][difficulty=easy]
Priority: -- → P2
Assignee: nobody → zer0
Status: NEW → ASSIGNED
Attached patch select-search.patch (obsolete) — Splinter Review
I also took occasion to make the find input in a type="search", with the placeholder instead of the label, that is more consistent with the rest of Firefox UI's search box; making also the input get the whole space available. However, if you prefer the previous setting, I'll revert those changes.
Attachment #8605202 - Flags: review?(bgrinstead)
Comment on attachment 8605202 [details] [diff] [review]
select-search.patch

Review of attachment 8605202 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. Can you please update the README in the directory to show the updated patched changes needed when importing a new copy of the plugin?  Also, I'd like if we had a super basic test in place for this (if nothing else so that it can be expanded upon with things like Bug 1153474).  a single browser_editor_search test could suit both of these bugs, I think
Attachment #8605202 - Flags: review?(bgrinstead) → feedback+
(In reply to Brian Grinstead [:bgrins] from comment #2)

> Looks good. Can you please update the README in the directory to show the
> updated patched changes needed when importing a new copy of the plugin?

I'm not familiar with that; I have just to copy & paste the new diff in the "Localization patches" section I guess, but the diff compared to what, exactly? The same in the patches, or to some previous commit for search.js?
 
> Also, I'd like if we had a super basic test in place for this (if nothing
> else so that it can be expanded upon with things like Bug 1153474).  a
> single browser_editor_search test could suit both of these bugs, I think

I'll make one for both then; thanks
Flags: needinfo?(bgrinstead)
Blocks: 1153474
(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #3)
> (In reply to Brian Grinstead [:bgrins] from comment #2)
> 
> > Looks good. Can you please update the README in the directory to show the
> > updated patched changes needed when importing a new copy of the plugin?
> 
> I'm not familiar with that; I have just to copy & paste the new diff in the
> "Localization patches" section I guess, but the diff compared to what,
> exactly? The same in the patches, or to some previous commit for search.js?

I believe we will need a diff of the current version with the patch applied and this file: https://github.com/codemirror/CodeMirror/blob/4.2.0/addon/search/search.js.  And yes, just needs to get copied/pasted into the patches section.
Flags: needinfo?(bgrinstead)
Attached patch search v2 (obsolete) — Splinter Review
As mentioned, the test for this behavior will be covered by bug 1153474.
Attachment #8617401 - Flags: review?(bgrinstead)
Attachment #8605202 - Attachment is obsolete: true
Comment on attachment 8617401 [details] [diff] [review]
search v2

Review of attachment 8617401 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.  Please update the commit message to include a little more context.  Maybe 'Bug 1159001 - Select text in the Style Editor search input when it is reopened;r=bgrins'
Attachment #8617401 - Flags: review?(bgrinstead) → review+
Attached patch search v3Splinter Review
Added a better commit message as mentioned.
Attachment #8617401 - Attachment is obsolete: true
Attachment #8623112 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/23d4c6e36439
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Whiteboard: [devedition-40][difficulty=easy] → [polish-backlog][difficulty=easy]
Duplicate of this bug: 1041954
" Press Enter to close the find mode " There is some mistake in this line. To close 'find' bar is necessary to press " ESC "
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.