Closed Bug 967044 Opened 6 years ago Closed 5 years ago

Make Home and End keys scroll console output area and autocomplete popup

Categories

(DevTools :: Console, defect, P3)

defect

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 34

People

(Reporter: rcampbell, Assigned: veeti.paananen, Mentored)

Details

(Whiteboard: [lang=js][mentor=robcee])

Attachments

(1 file, 2 obsolete files)

1. when a user has focused the input line but has not entered anything, Home and End keys should scroll the output area to the top and bottom respectively.

2. if a user has typed something and an autocomplete popup is onscreen, the Home and End keys should scroll the autocomplete popup to the top and bottom (respectively).

3. If a user has typed something in the input area and there is no autocomplete popup, the input cursor should move to the start of the line with home key and end of line with end key.
Assignee: nobody → rcampbell
No longer blocks: devtools-shortcuts
Status: NEW → ASSIGNED
no time, happy to help someone else fix this.
Assignee: rcampbell → nobody
Whiteboard: [mentor=robcee][lang=js]
Status: ASSIGNED → NEW
OS: Mac OS X → All
Priority: -- → P3
Hardware: x86 → All
Mentor: rcampbell
Whiteboard: [mentor=robcee][lang=js] → [lang=js]
Attached patch 967044.patch (obsolete) — Splinter Review
Attachment #8446327 - Flags: feedback?(rcampbell)
Comment on attachment 8446327 [details] [diff] [review]
967044.patch

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

I think this looks right. Nice patch.

Can we have a test too? :)
Attachment #8446327 - Flags: feedback?(rcampbell) → feedback+
Whiteboard: [lang=js] → [lang=js][mentor=robcee]
Thanks. What sort of tests? Mochitest? Any pointers?
**** Bugzilla. Am I just doing something wrong or is the "Need more information from..." checkbox not working?
Flags: needinfo?(rcampbell)
it's working. Sorry for the delay.

Yes, browser mochitests.

Look in the browser/devtools/webconsole/test directory for examples.

docs are here: https://developer.mozilla.org/en-US/docs/Mochitest
Flags: needinfo?(rcampbell)
I tried multiple times to set needinfo using the checkbox below the comment field to no effect. It only appeared once I used "set flags" from the top box thing.

Anyway, I'm attaching another patch with some tests I managed to cobble together. It tests the autocompletion and input field fine, but I'm not sure how to test scrolling the output panel. It also complains about some leaks for reasons beyond me. Some advice would be great.
Attached patch 967044-2.patch (obsolete) — Splinter Review
Attachment #8446327 - Attachment is obsolete: true
Attachment #8456161 - Flags: feedback?(rcampbell)
Comment on attachment 8456161 [details] [diff] [review]
967044-2.patch

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

::: browser/devtools/webconsole/test/browser_webconsole_home_end.js
@@ +15,5 @@
> +  }, true);
> +}
> +
> +function consoleOpened(aHud) {
> +  HUD = aHud;

there's your leak. If you need to capture this, declare it at the top of the file with a |let hud | as you did with popup.

see http://mxr.mozilla.org/mozilla-central/source/browser/devtools/webconsole/test/browser_webconsole_bug_585991_autocomplete_keys.js?force=1

for an example of good setup at the beginning of a test.

(you could also just add your home and end key tests to that file since it's testing this kind of behavior)

@@ +17,5 @@
> +
> +function consoleOpened(aHud) {
> +  HUD = aHud;
> +  info("web console opened");
> +  jsterm = HUD.jsterm;

same here. Don't declare globals.

@@ +19,5 @@
> +  HUD = aHud;
> +  info("web console opened");
> +  jsterm = HUD.jsterm;
> +  popup = jsterm.autocompletePopup;
> +  inputNode = jsterm.inputNode;

and here.
Attachment #8456161 - Flags: feedback?(rcampbell)
OK, thanks for the help. I added new assertions to two existing test cases. I also realized that the input field already handles home/end by itself, so I removed the special handling for that.

Attaching a new patch.
Attachment #8456161 - Attachment is obsolete: true
Attachment #8457411 - Flags: review?(rcampbell)
Comment on attachment 8457411 [details] [diff] [review]
967044-home-end.patch

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

this looks good! Thanks!

R+ with a successful run through try.
Attachment #8457411 - Flags: review?(rcampbell) → review+
Veeti, do you have Try server access?
Assignee: nobody → veeti.paananen
Status: NEW → ASSIGNED
Flags: needinfo?(veeti.paananen)
Nope.
Flags: needinfo?(veeti.paananen)
lgtm! ready to land.
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/f0e6f82eefd0
Keywords: checkin-needed
Whiteboard: [lang=js][mentor=robcee] → [lang=js][mentor=robcee][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/f0e6f82eefd0
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [lang=js][mentor=robcee][fixed-in-fx-team] → [lang=js][mentor=robcee]
Target Milestone: --- → Firefox 34
Verified fixed:
repro: 2014-07-27-03-02-04-mozilla-central-firefox-34.0a1.en-US.linux-x86_64
fixed: 2014-08-05-03-03-00-mozilla-central-firefox-34.0a1.ru.linux-x86_64
QA Whiteboard: [bugday-20140706]
Status: RESOLVED → VERIFIED
QA Whiteboard: [bugday-20140706] → [bugday-20140806]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.