Closed Bug 962531 Opened 10 years ago Closed 10 years ago

Page Up and Page Down should always scroll the output area in Console

Categories

(DevTools :: Console, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 29

People

(Reporter: rcampbell, Assigned: rcampbell)

References

Details

Attachments

(1 file, 2 obsolete files)

Should always be able to scroll the output area with Page Up and Page Down keys, even with the input line focused.
Priority: -- → P3
Assignee: nobody → rcampbell
OS: Mac OS X → All
Hardware: x86 → All
What happens when I have multi line input ?
Rob, the click-to-focus patch also broke double and triple clicks to select text in the output, unfortunately. The click event handler should also check ev.detail == 1 (one click).
Status: NEW → ASSIGNED
Depends on: 960695
Attached patch scrollOutput (obsolete) — Splinter Review
looking at testing this...
Attachment #8366858 - Flags: review?(mihai.sucan)
Depends on: 964268
Attached patch scrollOutput (obsolete) — Splinter Review
added a couple of tests to cover page scrolling in autocomplete popup and output area. Not comprehensive but should give us some minimal coverage.

Also removed XXX comments from webconsole and autocomplete js files.
Attachment #8366858 - Attachment is obsolete: true
Attachment #8366858 - Flags: review?(mihai.sucan)
Attachment #8367656 - Flags: review?(mihai.sucan)
Comment on attachment 8367656 [details] [diff] [review]
scrollOutput

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

Patch looks good. Tests pass here, however please update the tests as suggested.

::: browser/devtools/shared/autocomplete-popup.js
@@ +460,5 @@
> +   *
> +   * @type number
> +   */
> +  get _itemHeight() {
> +    return this._list._currentItem.clientHeight;

Why do you use _list._currentItem instead of _list.selectedItem?

::: browser/devtools/webconsole/test/browser_console_keyboard_accessibility.js
@@ +22,5 @@
>      hud = aHud;
>      ok(hud, "Web Console opened");
>  
> +    info("dump some spew into the console for scrolling");
> +    for (let i =0; i < 100; i++)

nit: let i = 0; ...

@@ +42,5 @@
> +    isnot(hud.outputNode.parentNode.scrollTop, currentPosition, "scroll position changed after page up");
> +
> +    EventUtils.synthesizeKey("VK_PAGE_DOWN", {});
> +    EventUtils.synthesizeKey("VK_PAGE_DOWN", {}); // twice to make sure we get back to the bottom
> +    is(hud.outputNode.parentNode.scrollTop , currentPosition, "scroll position now at bottom");

This is prone to intermittent failures. We cannot be sure that currentPosition will really be 'bottom', and that two page downs will get us there.

I suggest that you update currentPosition before sending the Page Down key, and just check that a page down changes the scroll location. You might want to check the direction of the scroll changes ( greater / lower than ...).

::: browser/devtools/webconsole/test/browser_webconsole_bug_585991_autocomplete_keys.js
@@ +92,5 @@
>          "completeNode.value holds watch");
>  
> +    EventUtils.synthesizeKey("VK_PAGE_DOWN", {});
> +
> +    is(popup.selectedIndex, 17,

This is also prone to intermittent failures. The selected index depends on the element height, popup height and other factors. Please simply check that the selectedIndex changed in the right direction. Dont bother to check the labels.

@@ +100,5 @@
> +      "completeNode.value holds __defineGetter__");
> +
> +    EventUtils.synthesizeKey("VK_PAGE_UP", {});
> +
> +    is(popup.selectedIndex, 0, "Index 0 is selected after Page UP");

Ditto.

::: browser/devtools/webconsole/webconsole.js
@@ +3899,5 @@
> +              this.hud.outputNode.parentNode.scrollTop -
> +              this.hud.outputNode.parentNode.clientHeight
> +            );
> +        }
> +        aEvent.preventDefault();

How about multiline input? Should we allow page up/down in multiline input?
Attachment #8367656 - Flags: review?(mihai.sucan) → review+
Attached patch scrollOutputSplinter Review
updated. Thanks!
Attachment #8367656 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/5becc1453393
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 29
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.