Summary: | REGRESSION: Arrow keys do not scroll source view in Resources pane or Scripts pane | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mark Rowe (bdash) <mrowe> | ||||||||
Component: | Web Inspector (Deprecated) | Assignee: | Brady Eidson <beidson> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | adele, beidson, bweinstein, darin, joepeck, keishi, mitz, mjs, pfeldman, pmuellr, rik, timothy | ||||||||
Priority: | P2 | Keywords: | InRadar, Regression | ||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
Mark Rowe (bdash)
2010-01-29 18:04:59 PST
Regression from the switch to canvas source view. I take it back, this was not fixed. What's the status of this? Was this just broken around the same time as the canvas change? It was broken by canvas. Then remained gone because we no longer use an iframe. The iframe gave us this behaviour for free. I'll take this one. Patch coming soon. Created attachment 49608 [details]
Simple fix.
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 (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. (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. (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... Created attachment 49626 [details]
Also patch TextViewer.js
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 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. (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. Created attachment 49639 [details]
Okay, herewego
*** Bug 35447 has been marked as a duplicate of this bug. *** 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. |