Closed Bug 1397366 Opened 2 years ago Closed 2 years ago

DevTools - Scratchpad - Main menu: Edit - "Find", "Find Again", "Jump to line" - does not work

Categories

(DevTools :: Scratchpad, defect, P2)

defect

Tracking

(firefox-esr52 wontfix, firefox55 wontfix, firefox56 wontfix, firefox57 wontfix, firefox58 wontfix, firefox59 fixed)

RESOLVED FIXED
Firefox 59
Tracking Status
firefox-esr52 --- wontfix
firefox55 --- wontfix
firefox56 --- wontfix
firefox57 --- wontfix
firefox58 --- wontfix
firefox59 --- fixed

People

(Reporter: janekptacijarabaci, Assigned: jdescottes)

References

Details

(Keywords: regression)

Attachments

(1 file)

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:45.9) Gecko/20100101 Goanna/3.2 Firefox/45.9 PaleMoon/27.4.2
Build ID: 20170821181241

Steps to reproduce:

DevTools - Scratchpad - Main menu: Edit - "Find", "Find Again", "Jump to line" - does not work
Keyboard shortcuts works (only).
See Also: → 1011504
Blocks: 1292592
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
Hi Patrick,
I was interested in working on this bug and was trying to repro the same.
I opened scratchpad. Entered some text. 
Pressed Ctrl+F(find), it worked fine. Next pressed Ctrl+G(find again) - didn't work - nothing happened.
Next pressed Ctrl+J(goto line), worked fine.

I wanted to know what is exactly broken here.
Flags: needinfo?(pbrosset)
The keyboard shortcuts do work, but not the corresponding menu items. 
I just tested on Firefox 56, and they still don't work for me (knowing that this was filed 5 years ago, this is a good sign of how little scratchpad is used!)
Flags: needinfo?(pbrosset)
> knowing that this was filed 5 years ago

5 days ago :-)
...I didn't find another similar bug.
(In reply to janekptacijarabaci from comment #5)
> > knowing that this was filed 5 years ago
> 
> 5 days ago :-)
Ahah, sorry, I need to wake up now.
Thanks for tracking the regressing bug. Beyond scratchpad, this also affects the style editor which sees some context menu items being disabled when they should not.
Priority: -- → P2
Not entirely sure about the best way to proceed here: I ended up restoring the controller code for the source editor commands that was removed in Bug 1292592. I just extracted it and made it optional (having it by default was conflicting with the debugger's usage of the source editor).

I initially tried to simply call source-editor APIs from scratchpad when receiving a command. This was working fine, but I couldn't drive the state of the find-again menu item correctly. On popupShowing, I was checking the state of the codemirror search and either enabling or disabling the item. But something else (an actual controller?) kept removing the "disabled" attribute that I was setting. Haven't investigated much more in that direction.
Attachment #8939626 - Flags: review?(poirot.alex)
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Comment on attachment 8939626 [details]
Bug 1397366 - restore source-editor commands controller for scratchpad & styleeditor menus;

https://reviewboard.mozilla.org/r/209928/#review217108

Works great!

I wasn't expecting to see a patch in 2018 introducing more XUL :)

::: devtools/client/sourceeditor/editor-commands-controller.js:94
(Diff revision 2)
> + * Create and insert a commands controller for the provided SourceEditor instance.
> + */
> +function insertCommandsController(sourceEditor) {
> +  let input = sourceEditor.codeMirror.getInputField();
> +
> +  if (input.controllers && typeof input.controllers.insertControllerAt == "function") {

Do these conditions happen to be false?
I imagine that's when the input isn't a xul element.
But given that you call it only from StyleEditor and Scratchpad, it should always be true, isn't it?
May be we want it to throw when used against HTML element so that we can remove it when refactoring to pure HTML?
Attachment #8939626 - Flags: review?(poirot.alex) → review+
(In reply to Alexandre Poirot [:ochameau] from comment #13)
> Comment on attachment 8939626 [details]
> Bug 1397366 - restore source-editor commands controller for scratchpad &
> styleeditor menus;
> 
> https://reviewboard.mozilla.org/r/209928/#review217108
> 
> Works great!
> 
> I wasn't expecting to see a patch in 2018 introducing more XUL :)
> 
> ::: devtools/client/sourceeditor/editor-commands-controller.js:94
> (Diff revision 2)
> > + * Create and insert a commands controller for the provided SourceEditor instance.
> > + */
> > +function insertCommandsController(sourceEditor) {
> > +  let input = sourceEditor.codeMirror.getInputField();
> > +
> > +  if (input.controllers && typeof input.controllers.insertControllerAt == "function") {
> 
> Do these conditions happen to be false?
> I imagine that's when the input isn't a xul element.
> But given that you call it only from StyleEditor and Scratchpad, it should
> always be true, isn't it?
> May be we want it to throw when used against HTML element so that we can
> remove it when refactoring to pure HTML?

Thanks for the review! 

Good point about the test. It's actually never false, for any input. Even for inputs in HTML documents (such as inspector inputs) this would return true. We had a similar check in another part of the code, but it was about running devtools in a webpage vs running devtools as a Firefox panel, which doesn't apply here.

Removed the check.
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b51eda5903d5
restore source-editor commands controller for scratchpad & styleeditor menus;r=ochameau
https://hg.mozilla.org/mozilla-central/rev/b51eda5903d5
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.