REGRESSION: Arrow keys do not scroll source view in Resources pane or Scripts pane

[Mark Rowe (bdash)]

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.

[Timothy Hatcher]

Regression from the switch to canvas source view.

[Mark Rowe (bdash)]

<rdar://problem/7594367>

[Timothy Hatcher]

Fixed in http://trac.webkit.org/changeset/54133.
https://bugs.webkit.org/show_bug.cgi?id=34400

[Timothy Hatcher]

I take it back, this was not fixed.

[Adele Peterson]

What's the status of this?  Was this just broken around the same time as the canvas change?

[Timothy Hatcher]

It was broken by canvas. Then remained gone because we no longer use an iframe. The iframe gave us this behaviour for free.

[Brady Eidson]

I'll take this one.  Patch coming soon.

[Brady Eidson]

Created attachment 49608 [details]
Simple fix.

[Pavel Feldman]

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

[Darin Adler]

(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.

[Brady Eidson]

(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.

[Brady Eidson]

(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...

[Brady Eidson]

Created attachment 49626 [details]
Also patch TextViewer.js

[Brady Eidson]

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.

[Pavel Feldman]

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.

[Brady Eidson]

(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.

[Brady Eidson]

Created attachment 49639 [details]
Okay, herewego

[Brady Eidson]

*** Bug 35447 has been marked as a duplicate of this bug. ***

[Brady Eidson]

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.