Bug 34356 - REGRESSION: Arrow keys do not scroll source view in Resources pane or Scripts pane
Summary: REGRESSION: Arrow keys do not scroll source view in Resources pane or Scripts...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords: InRadar, Regression
: 35447 (view as bug list)
Depends on:
Blocks:
 
Reported: 2010-01-29 18:04 PST by Mark Rowe (bdash)
Modified: 2010-02-26 14:27 PST (History)
12 users (show)

See Also:


Attachments
Simple fix. (2.59 KB, patch)
2010-02-26 12:02 PST, Brady Eidson
pfeldman: review-
beidson: commit-queue-
Details | Formatted Diff | Diff
Also patch TextViewer.js (4.35 KB, patch)
2010-02-26 13:03 PST, Brady Eidson
pfeldman: review-
beidson: commit-queue-
Details | Formatted Diff | Diff
Okay, herewego (2.91 KB, patch)
2010-02-26 14:15 PST, Brady Eidson
pfeldman: review+
beidson: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Rowe (bdash) 2010-01-29 18:04:59 PST
For some reason the up and down arrow keys no longer scroll the source view in the Resources pane in r54084.  It works correctly in a build of WebKit from earlier in the week.
Comment 1 Timothy Hatcher 2010-01-29 18:06:57 PST
Regression from the switch to canvas source view.
Comment 2 Mark Rowe (bdash) 2010-01-29 18:43:34 PST
<rdar://problem/7594367>
Comment 4 Timothy Hatcher 2010-02-01 11:49:38 PST
I take it back, this was not fixed.
Comment 5 Adele Peterson 2010-02-22 17:47:23 PST
What's the status of this?  Was this just broken around the same time as the canvas change?
Comment 6 Timothy Hatcher 2010-02-22 19:46:56 PST
It was broken by canvas. Then remained gone because we no longer use an iframe. The iframe gave us this behaviour for free.
Comment 7 Brady Eidson 2010-02-26 10:26:44 PST
I'll take this one.  Patch coming soon.
Comment 8 Brady Eidson 2010-02-26 12:02:03 PST
Created attachment 49608 [details]
Simple fix.
Comment 9 Pavel Feldman 2010-02-26 12:12:30 PST
Comment on attachment 49608 [details]
Simple fix.

>  /*
> - * Copyright (C) 2007, 2008 Apple Inc.  All rights reserved.
> + * Copyright (C) 2007, 2008, 2009, 2010 Apple Inc.  All rights reserved.

We would put just 2010 here, not sure about you folks thought.

>          this._containerContentElement.id = "resources-container-content";
> +        this._containerContentElement.tabIndex = 0;
> +        this._containerContentElement.addEventListener("keydown", this._handleKeyDownForScroll.bind(this), false);

You should put this code into the TextViewer.js. Otherwise Scripts panel is not affected with your changes.

>  
> +    _handleKeyDownForScroll: function(event)
> +    {

I'd call it _handleKeyDown since it is registered for all keyboard events, not necessarily related to the scrolling. Also should be in TextViewer.js
Comment 10 Darin Adler 2010-02-26 12:20:18 PST
(In reply to comment #9)
> > - * Copyright (C) 2007, 2008 Apple Inc.  All rights reserved.
> > + * Copyright (C) 2007, 2008, 2009, 2010 Apple Inc.  All rights reserved.
> 
> We would put just 2010 here, not sure about you folks thought.

If there were changes published in 2009 and someone just forgot to add the year at that time, it's fine to add the year 2009 now. If there we indeed no significant changes in 2009, then yes, we should add only 2010.
Comment 11 Brady Eidson 2010-02-26 12:48:00 PST
(In reply to comment #10)
> (In reply to comment #9)
> > > - * Copyright (C) 2007, 2008 Apple Inc.  All rights reserved.
> > > + * Copyright (C) 2007, 2008, 2009, 2010 Apple Inc.  All rights reserved.
> > 
> > We would put just 2010 here, not sure about you folks thought.
> 
> If there were changes published in 2009 and someone just forgot to add the year
> at that time, it's fine to add the year 2009 now. If there we indeed no
> significant changes in 2009, then yes, we should add only 2010.

Yes, I'd already checked - Apple contributors made changes in 2009 and we'd just forgotten to add the date.
Comment 12 Brady Eidson 2010-02-26 12:50:27 PST
(In reply to comment #9)

> >          this._containerContentElement.id = "resources-container-content";
> > +        this._containerContentElement.tabIndex = 0;
> > +        this._containerContentElement.addEventListener("keydown", this._handleKeyDownForScroll.bind(this), false);
> 
> You should put this code into the TextViewer.js. Otherwise Scripts panel is not
> affected with your changes.

This bugzilla was only about the Resources panel.  If you knew about a regression separate from this bug, you should've filed it or at least commented here!

> >  
> > +    _handleKeyDownForScroll: function(event)
> > +    {
> 
> I'd call it _handleKeyDown since it is registered for all keyboard events, not
> necessarily related to the scrolling. 

Done

New patch coming...
Comment 13 Brady Eidson 2010-02-26 13:03:21 PST
Created attachment 49626 [details]
Also patch TextViewer.js
Comment 14 Brady Eidson 2010-02-26 13:06:36 PST
Filed https://bugs.webkit.org/show_bug.cgi?id=35447 to track the lack of horizontal keyboard scrolling in the Text Viewer.

We should either:
A - Bring back the previous iframe behavior of "no horizontal scrollbars"
or
B - Make horizontal keyboard scrolling work.

But that's a separate task.
Comment 15 Pavel Feldman 2010-02-26 13:10:26 PST
Comment on attachment 49626 [details]
Also patch TextViewer.js

> ===================================================================
> --- WebCore/inspector/front-end/AbstractTimelinePanel.js	(revision 55293)
> +++ WebCore/inspector/front-end/AbstractTimelinePanel.js	(working copy)

This file should not change. Changes to the TextViewer.js should be sufficient to cover all viewers' scrolling.

> +        var scrollLines = 0;
> +        if (event.keyCode === WebInspector.KeyboardShortcut.KeyCodes.Up)
> +            scrollLines = -1;
> +        else if (event.keyCode == WebInspector.KeyboardShortcut.KeyCodes.Down)
> +            scrollLines = 1;
> +        
> +        if (scrollLines) {
> +            event.preventDefault();
> +            event.stopPropagation();
> +            this.containerElement.scrollByLines(scrollLines);

> +        var scrollLines = 0;
> +        if (event.keyCode === WebInspector.KeyboardShortcut.KeyCodes.Up)
> +            scrollLines = -1;
> +        else if (event.keyCode == WebInspector.KeyboardShortcut.KeyCodes.Down)
> +            scrollLines = 1;

This way Cmd + Up is going to be treated as one-line step. You should only consume Arrow events with no modifiers.

Btw, what's going on to the Left / Right? Now that we don't wrap lines we might need to handle them as well.
Comment 16 Brady Eidson 2010-02-26 13:17:08 PST
(In reply to comment #15)
> (From update of attachment 49626 [details])
> > ===================================================================
> > --- WebCore/inspector/front-end/AbstractTimelinePanel.js	(revision 55293)
> > +++ WebCore/inspector/front-end/AbstractTimelinePanel.js	(working copy)
> 
> This file should not change. Changes to the TextViewer.js should be sufficient
> to cover all viewers' scrolling.

I'm pretty sure you're wrong about this...  hitting you up on IRC to resolve this quicker.

> Btw, what's going on to the Left / Right? Now that we don't wrap lines we might
> need to handle them as well.

I filed https://bugs.webkit.org/show_bug.cgi?id=35447 as a followup - it is independent from this task.
Comment 17 Brady Eidson 2010-02-26 14:15:41 PST
Created attachment 49639 [details]
Okay, herewego
Comment 18 Brady Eidson 2010-02-26 14:16:48 PST
*** Bug 35447 has been marked as a duplicate of this bug. ***
Comment 19 Brady Eidson 2010-02-26 14:27:57 PST
Landed in http://trac.webkit.org/changeset/55313

File https://bugs.webkit.org/show_bug.cgi?id=35455 to track the Timeline panel, which I thought was a regression and part of this bug.