Closed
Bug 542480
Opened 14 years ago
Closed 5 years ago
Scrolling with arrows keys is not consistent with panning scroll
Categories
(Firefox for Android Graveyard :: Panning/Zooming, defect)
Firefox for Android Graveyard
Panning/Zooming
Tracking
(Not tracked)
RESOLVED
WONTFIX
People
(Reporter: fabrice.desre, Unassigned)
References
Details
(Whiteboard: [fx4rc-testday])
Attachments
(1 file, 2 obsolete files)
17.44 KB,
patch
|
vingtetun
:
feedback+
|
Details | Diff | Splinter Review |
STR : - load eg. a search results page from google - pan down with your finger - use the up/down arrows to scroll further Results : When using the keyboard, the scrolling position starts from the top of the page, with a jump. Expected results : No jump. Cause : this is because we don't call scrollTo() on the browser after a pan. The patch in bug 500700 fixes this.
Comment 1•14 years ago
|
||
(In reply to comment #0) > Cause : this is because we don't call scrollTo() on the browser after a pan. > The patch in bug 500700 fixes this. Yeah this is the root cause but i tend to think that we want a more complex behavior that the current one with key up/key down because of the difference of ratio between the browser and the actual view and also because scrolling with the keyboard also means handling a "scroll" event and refreshing the viewable area which is resource consuming. For these i think it will be better to make a hook in InputHandler.js and not relying on the scrollTo() sync (even if I hardly want it!)
Updated•14 years ago
|
Whiteboard: DUPEME? bug 520185
Updated•14 years ago
|
Component: General → Panning/Zooming
Updated•14 years ago
|
Whiteboard: DUPEME? bug 520185
Comment 3•14 years ago
|
||
Would it be desirable to have a smooth, "kinetic" style keyboard scrolling, a bit like what there is in N900 native browser? That would probably mean implementing the scrolling with some key event-related module in input handler using methods similar to the kinetic panning, and removing the default key scroll bindings.
Comment 4•14 years ago
|
||
To add on: there also appears to be a problem with scrolling when using the arrow keys or spacebar if the view has been expanded. Test case: 1) visit http://news.google.com.sg/ 2) double tap an article in the "Top Stories" section to expand the view 3) try to scroll all the way to the bottom using the down arrow key or spacebar 4) scrolling will stop before hitting the bottom: the search bar at the bottom of the web page cannot be seen 5) however, scrolling down with tap-drag still works
Comment 5•14 years ago
|
||
This patch is based on Jaakko's earlier patch. It implements smooth scrolling with keys. And no jumping occurs.
Attachment #460503 -
Flags: review?(mark.finkle)
Comment 6•14 years ago
|
||
(In reply to comment #5) > Created attachment 460503 [details] [diff] [review] > Fix proposal > > This patch is based on Jaakko's earlier patch. It implements smooth scrolling > with keys. And no jumping occurs. Could you use another patch format? This one is very uncommon here and hard to review or apply. https://developer.mozilla.org/en/Creating_a_patch
Comment 7•14 years ago
|
||
Changed patch format.
Attachment #460503 -
Attachment is obsolete: true
Attachment #460503 -
Flags: review?(mark.finkle)
Comment 9•14 years ago
|
||
This patch implements smooth scrolling using keyboard. The has an issue that key events are also processed in XUL-runner and this causes incorrect panning on the page. When panning key events should be processed by the code in this patch. So in panning key event handling should be disabled from XUL-runner for this patch to work correct.
Attachment #461485 -
Attachment is obsolete: true
Attachment #469845 -
Flags: feedback?(21)
Comment 10•14 years ago
|
||
Comment on attachment 469845 [details] [diff] [review] Fix proposal First my global impression by having tried the patch : Navigating with keyboard is way much better with this patch and I would appreciate to replace our old behavior code for this one. Then come a basic feedback review. >diff --git a/app/mobile.js b/app/mobile.js> /* ignore all events that belong to other windows or documents (e.g. content events) */ >- if (aEvent.view != window) >+ if (aEvent.view != window && !aEvent.keyCode) { >+ Util.dumpLn("InputHandler.handleEvent", aEvent.type, aEvent.view, "!=", window, "-> IGNORE!"); > return; >+ } Remove the debug code > /** > * Utility method for passing an EventInfo to the handlers of all modules beginning > * with the module at index skipToIndex and increasing (==> decreasing in priority). > */ > _passToModules: function _passToModules(aEvent, aSkipToIndex) { >+ try{ > if (this._grabber) { > this._grabber.handleEvent(aEvent); > } else { > let mods = this._modules; > let i = aSkipToIndex || 0; > > for (let len = mods.length; i < len; ++i) { > mods[i].handleEvent(aEvent); // event focus could get grabbed in this invocation > if (this._grabber) // so don't pass the event to the rest of modules > break; > } > } >+ } catch(e) { Util.dumpLn('IH', e.toString()); } > } > }; Please remove try/catch, this is often a way to debug but this can hide real problem for a release build. > // walk up the DOM tree in search of nearest scrollable ancestor. nulls are > // returned if none found. > let [targetScrollbox, targetScrollInterface] >- = this.getScrollboxFromElement(aEvent.target); >+ = ElementUtils.getScrollboxFromElement(aEvent.target); I wonder if ElementUtils is the name we want since all of this code is related to scrolling/panning. Mark Finkle has always, err, often good name for such things. But since all this code does not have a hard relationship with the rest of the code I don't see any reason for not moving it as you do. >- // ----------------------------------------------------------- >- // -- Utility functions >+/** >+ * Utility functions for element scrolling and clicking >+ */ >+var ElementUtils = { As I've said before this sounds too general. > > KeyModule.prototype = { > getClickerFromElement: function getClickerFromElement(elem) { > for (; elem; elem = elem.parentNode) > if (elem.customKeySender) > break; > return (elem) ? elem : null; > }, Now this sounds confusing to me. We have a getClickerFromElement into ElementUtils but also a getClickerFromElement into the KeyModule. I see two options: * rename the one in KeyModule for getClickerFromElement * Do a more elaborate version of the one into ElementUtils to look for an instance of a specific class Since the second require much more code change and doesn't bring much, let's do the first! (Sorry thinking out loud) > handleEvent: function handleEvent(aEvent) { >+ try { Remove the try/catch please > if (aEvent.type == "keydown" || aEvent.type == "keyup" || aEvent.type == "keypress") { >- let keyer = this._browserViewContainer.customKeySender; >- if (keyer) { >- keyer.dispatchKeyEvent(aEvent); >- aEvent.stopPropagation(); >- aEvent.preventDefault(); >+ this._keyer = this._browserViewContainer.customKeySender; >+ if (this._keyer) { Remove one space at the beginning of "this._keyer" >+ if (this._keyer.dispatchKeyEvent(aEvent)) { >+ aEvent.preventDefault(); >+ aEvent.stopPropagation(); >+ } >+ else { >+ // content did not consume the key event, allow scrolling with it >+ this._keyScroller.handleEvent(aEvent); >+ } > } >- } I'm thinking of merging KeyModule and KeyScroller since I don't see any other use for it. Any objections? >+KeyScroller.prototype = { >+ _dragger: null, >+ _scroller: null, >+ _direction: new Point(0, 0), >+ _momentum: new Point(0, 0), >+ _focus: new Point(0, 0), >+ _keyDown: {}, >+ >+ handleEvent: function handleEvent(aEvent) { >+ let ev = aEvent; Remove "ev" and use "aEvent" in the code. >+ let consume = false; >+ >+ switch (ev.type) { >+ case "mousedown": >+ this._focus = new Point(ev.clientX, ev.clientY); >+ break; What it is used for? >+ >+ case "keypress": >+ consume = this._handleKeyEvent(ev); >+ break; >+ >+ case "keyup": >+ // only handle this if we have reacted to down/press >+ if (this._keyDown[ev.keyCode]) >+ consume = this._handleKeyEvent(ev); >+ break; >+ } >+ >+ _updatePrefs: function _updatePrefs() { >+ // cache limit values from preferences >+ let branch = "browser.ui.keyScroll."; >+ this._minStep = Services.prefs.getIntPref(branch + "minStep"); >+ this._maxStep = Services.prefs.getIntPref(branch + "maxStep"); >+ this._speedGain = Services.prefs.getIntPref(branch + "speedGain"); >+ this._speedDecay = (100 + Services.prefs.getIntPref(branch + "speedDecay")) / 100.0; >+ this._fps = Services.prefs.getIntPref(branch + "fps"); >+ }, You should probably use the getBranch call to avoid adding it each time. https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIPrefService#getBranch%28%29 >+ >+ _canScroll: function _canScroll(ev) { Nit: Please use aEvent instead of ev. >+ if (ev.view != window) { >+ let t = ev.target; >+ >+ if (t.isContentEditable) { >+ return false; >+ } >+ >+ // check for editable xul elements >+ else if (t instanceof XULElement) { >+ if(t.tagName == 'textbox') >+ return false; >+ } >+ else if (t instanceof HTMLElement) { >+ // check for keyboard interactive elements >+ if (t instanceof HTMLInputElement || >+ t instanceof HTMLSelectElement || >+ t instanceof HTMLTextAreaElement) { >+ return false; >+ } >+ } >+ } You could probably make this code much robust by using mozIsTextField on the currently focused element. http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/html/nsIDOMHTMLInputElement.idl#118 >+ >+ _handleKeyEvent: function _handleKeyEvent(ev) { Nit: Please use aEvent instead of ev >+ let dx = 0, dy = 0; >+ let down = (ev.type != "keyup"); s/down/keyDown >+ let k = down ? 1 : -1; s/k/direction >+ switch (ev.keyCode) { >+ case KeyEvent.DOM_VK_LEFT: k = -k; // change direction and fallthrough >+ case KeyEvent.DOM_VK_RIGHT: dx = k; break; >+ case KeyEvent.DOM_VK_UP: k = -k; // change direction and fallthrough >+ case KeyEvent.DOM_VK_DOWN: dy = k; break; >+ } Nit: I like when the switch/case is fully expanded. >+ if ((!dx && !dy) || (this._keyDown[ev.keyCode] ? down : !down)) { >+ return false; // not a scroll key or no state change >+ } Nit: Remove brackets for one line expression >+ // update new scrolling direction >+ this._keyDown[ev.keyCode] = down; >+ this._direction.add(dx, dy); I don't really like the way you use to exchange information about the key state. >+ >+ // start scrolling if it has not yet been started >+ return (down && !this._timer) ? this._startScrolling(ev) : true; >+ }, >+ >+ _startScrolling: function _startScrolling(ev) { >+ Nit: We don't need this blank line >+ // we can not scroll this element >+ if (!this._canScroll(ev)) { >+ return false; >+ } Nit: Not brackets for one line >+ >+ // cache latest preferences >+ this._updatePrefs(); Nit: Add a blank line :) >+ // resolve the target element from the current key event focus >+ let targetElement = ev.target; >+ if (targetElement == window) { >+ // focus is on main-window, use it's focused element >+ targetElement = gFocusManager.getFocusedElementForWindow(window, false, {}); >+ // window does not have focus element, improvise one from the middle of the window >+ if (!targetElement) { >+ targetElement = Browser.elementFromPoint(window.innerWidth / 2, >+ window.innerHeight / 2); >+ } Is this case really common? In my opinion if the window doesn't have any focus element we don't want to dispatch event to them. >+ else if (targetElement instanceof HTMLElement) { >+ // use content-scrollbox for all HTML elements >+ targetElement = document.getElementById("content-scrollbox"); >+ } Does it works? I don't think e10s let you receive event from the web. (Btw, the html:input nested into a xul textbox is also an html element (ev.target probably ignored it) and extension developers can use them if they want (but this is less my concern)) I think this code is the root cause of a bug I've seen when the focus is in a textbox into an HTML document but the keys are still scrolling (which is a major issue for me) I'm considering adding a message from content to the chrome UI about focus changes (this could help for some FormHelperUI bugs too) >+ // can not start scrolling without a working dragger >+ if (!this._dragger || !this._dragger.isDraggable(targetElement, this._scroller)) { >+ Util.dumpLn("KeyScrollModule:", this._dragger ? "target not draggable" : "no scroll interface"); Remove the dump >+ return false; >+ } >+ >+ // grab input handler during scrolling >+ this._keyModule._owner.grab(this._keyModule); >+ >+ // start dragging >+ this._dragger.dragStart(this._focus.x, this._focus.y, targetElement, this._scroller); >+ this._timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer); >+ this._timer.initWithCallback(this, 1000 / this._fps, this._timer.TYPE_REPEATING_SLACK); >+ return true; >+ }, Now I see what use you have for _focus. What if a user is trying to scroll when he load a long page or he just switch to another tab without clicking somewhere? The code seems pretty vulnerable to that I think. >+ >+ notify: function notify(timer) { >+ try { Remove try/catch >+ // try to drag by momentum >+ let wasStopped = this._stopped(); >+ let didPan = this._dragger.dragMove(this._momentum.x, this._momentum.y, this._scroller); >+ if (!didPan) { >+ // did not manage to pan -> we've lost all momentum >+ this._momentum.x = this._momentum.y = 0; >+ } Remove brackets for one liner and add a blank line after. >+ if (!wasStopped || didPan) { >+ // drag was done or we have not yet stopped -> update momentum >+ this._momentum.x = this._updateMomentum(this._momentum.x, this._direction.x); >+ this._momentum.y = this._updateMomentum(this._momentum.y, this._direction.y); Since you are using _updateMomentum only here, you could potentially consider having multiple return value instead of calling it twice. https://developer.mozilla.org/en/new_in_javascript_1.7#Multiple-value_returns >+ >+ _updateMomentum: function _updateMomentum(m, d) { >+ let ret = m; Please use better arguments and variable name. I like explicit names. >+ _finish: function _finish() { >+ if (this._timer) { >+ this._timer.cancel(); >+ this._timer = null; >+ } Nit: add a blank line >+ if (this._dragger) { >+ this._dragger.dragStop(this._focus.x, this._focus.y, this._scroller); >+ this._dragger = null; >+ } Nit: add a blank line >+ this._scroller = null; >+ this._keyModule._owner.ungrab(this._keyModule); >+ this._momentum.x = this._momentum.y = 0; >+ // this._direction must be set only based on key events to reflect actual key state! What is this comment for? Sorry for the long feedback, it sounds more like a review... I like the provided behavior and the code is in a good shape in my opinion but still needs some bug fixes before being good to pass review.
Attachment #469845 -
Flags: feedback?(21) → feedback+
Comment 11•14 years ago
|
||
This patch still has problem that panning is not in sync with keyboard scrolling. Or the actual problem is that xulrunner keyboard scrolling is still used and content sometimes jumps during the key panning. Does anybody have a solution how to solve this annoying problem? Maybe create an API for setting xulrunner keyboard scrolling on/off from JS side or some other way.
Comment 13•13 years ago
|
||
4.0rc1 candidate build 3 for linux-i686 BuildID=20110318122108 SourceRepository=http://hg.mozilla.org/releases/mobile-2.0 SourceStamp=606c14460ded On a zoomed-in page, when scrolling using the arrow keys, Fennec appears to measure whether it reached the end of the page as if the page was not zoomed in, so you cannot reach the end (similar to last years bug 547307).
Updated•13 years ago
|
Whiteboard: [fx4rc-testday]
Comment 14•5 years ago
|
||
Closing all opened bug in a graveyard component
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•