Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#17062 closed defect (fixed)

[regression] Cannot scroll list up and down using the keyboard

Reported by: Paul Christopher Owned by: Adrian Vasiliu
Priority: undecided Milestone: 1.9.1
Component: DojoX Mobile Version: 1.9.0b2
Keywords: Cc:
Blocked By: Blocking:

Description

Description

When navigating a huge list with the keyboard, scrolling down the list shows a jumpy behavior. Navigating the list upwards with the tab key does not scroll the nodes into view.

Steps to reproduce the issue

Open http://archive.dojotoolkit.org/nightly/dojotoolkit/dojox/mobile/tests/test_FilteredList-EdgeToEdgeStoreList-auto.html . Use the tab key to navigate the page. Notice the last semi-hidden list item is scrolled into view. That's nice. But if you hit tab again, the list jumps down by 5 or more items. By purpose?

Now try to navigate upwards using Shift-Tab. Notice focus gets lost in Nirwana since the focused list items are not scrolled into view.

Discussion

Maybe not for tablets but for other embedded JS devices, keyboard support is necessary (I have such a use case): Long list can be scrolled e.g. with the up and down arrows of a remote control. Therefore a keyboard support is necessary and should work.

All in all I was wondering whether using a tabindex of 0 is sufficient or whether the list as a whole should be focusable with the tab key and the list items themselves are navigated with the up and down keys only? This means: list items neeed to have a tabindex of -1 and not 0. Maybe one of the accessibility experts can tell us more on that...

Attachments (10)

ticket17062-nonAndroid.patch (14.2 KB) - added by Sebastien Brunot 6 years ago.
Sample solution that works on desktop browser and on iOS. Doesn't work on android stock browser, not yet tested on blackberry devices (IBM CCLA).
ticket17062.patch (15.8 KB) - added by Sebastien Brunot 6 years ago.
Support navigating among input field using TAB (desktop keyboard) or NEXT (mobile soft keyboard). (IBM CCLA).
patch17062-adrian.patch (1.6 KB) - added by Adrian Vasiliu 6 years ago.
Fix autoscroll when navigating among input field using TAB/SHIFT-TAB (desktop) or PREV/NEXT (mobile soft keyboard) - Adrian Vasiliu (IBM, CCLA)
test_pure-HTML.html (1.5 KB) - added by Adrian Vasiliu 6 years ago.
For comparison purposes - a pure HTML (no Dojo) test page containing many focusable elements (here, input fields).
test_BigNodes.html (3.7 KB) - added by Paul Christopher 6 years ago.
test_pure-HTML_BigNodes.html (1.5 KB) - added by Paul Christopher 6 years ago.
patch17062-adrian-v2.patch (1.5 KB) - added by Adrian Vasiliu 6 years ago.
Fix autoscroll when navigating among input field (and among any focused elements) using TAB/SHIFT-TAB (desktop) or PREV/NEXT (mobile soft keyboard) - Adrian Vasiliu (IBM, CCLA)
patch17062-adrian-v3.patch (1.7 KB) - added by Adrian Vasiliu 6 years ago.
Fix autoscroll when navigating among focusable elements using TAB/SHIFT-TAB (desktop) or PREV/NEXT (mobile soft keyboard) - Adrian Vasiliu (IBM, CCLA)
patch17062-adrian-v4.patch (2.2 KB) - added by Adrian Vasiliu 6 years ago.
Fix autoscroll when navigating among focusable elements using TAB/SHIFT-TAB (desktop) or PREV/NEXT (mobile soft keyboard) - Adrian Vasiliu (IBM, CCLA)
patch17062-adrian-v5.patch (2.6 KB) - added by Adrian Vasiliu 6 years ago.
Fix autoscroll when navigating among focusable elements using TAB/SHIFT-TAB (desktop) or PREV/NEXT (mobile soft keyboard) - Adrian Vasiliu (IBM, CCLA)

Download all attachments as: .zip

Change History (53)

comment:1 Changed 6 years ago by Paul Christopher

Maybe the problem is somewhere in scrollable.js event listener for the scroll event (line 207 et seq]:

  • When tabbing forward, the scrollTo position seems to be calculated wrong?
  • When tabbing backward, no scroll event is fired at all.

comment:2 Changed 6 years ago by Sebastien Brunot

This is a regression from the fix for http://bugs.dojotoolkit.org/ticket/16363.

I'm attaching a patch for a proposal to fix both #17060 and #16363:

1) do not reset the scrollTop and scrollLeft value in the scrollable node, but take it into account when performing internal calculation;

2) listen to focusin events and use the internal scrollable mechanism to scroll the currently selected element into view.

Tested on Chrome, Firefox ESP and IE10 on the dektop, and on iPhone iOS6.

comment:3 Changed 6 years ago by Adrian Vasiliu

A few comments to ticket17062.patch:

  • scrollable.cleanup() still removes the this._onScroll listener that the patch no longer installs.
  • Instead of adding the focus listener using domNode.addEventListener, I think it should use dojo/on or even better widget.on. Unless there's a reason not to do so.
  • Minor formatting: we usually add blanks around the ternary operators "? :".
  • From an usability point of view, there are a few cosmetic details which seem a bit annoying (although not fatal, given that we do need a solution for the bug...):
    • it's a bit unusual that the focused field moves to the very top
    • this is particularly unpleasant and disturbing because of the lack of animation during this move.
    • and the behavior is not consistent (systematic), for instance load test_TextBox-in-ScrollableView.html on iPhone 5 iOS 6.1.3, touch "field7" => it goes to top; touch "field11" => it goes to top; close the keyboard, scroll to the bottom of the list, touch, "field38" => it does NOT go to top...
  • More annoying: load test_TextBox-in-ScrollableView.html on iPhone 5 iOS 6.1.3, touch "field7" then scroll the view (while the keyboard is open) a bit towards the top, then start typing => the input field is not visible, the cursor can overlap the header, and the user types "into the Nirvana".
  • While the patch now calls scrollable.scrollIntoView, there is also dijit/form/_FormWidgetMixin which calls dojo/windows.scrollIntoView. On iOS6, they are both called, first our's, second dijit's. Would be nice to clarify how the two articulate...
  • Differently than dijit's _FormWidgetMixin, the patch does not check the flag widget.scrollOnFocus (defined by dijit/form/_FormWidgetMixin, which is extended by mobile text widgets). Shouldn't it?
Last edited 6 years ago by Adrian Vasiliu (previous) (diff)

comment:4 Changed 6 years ago by Sebastien Brunot

Hi, thanks for the review of the patch !

I've attached another version that fixes the first and the third problems. For the other ones, here's the current status:

  • do you think we can add dojo/on to the list of requirements for scrollable.js ? If yes, I can replace the call to addEventListener to a call to dojo/on, no problem with that. I just wanted to use the dependencies that were already defined (and connect is deprecated).
  • I've relied on the current implementation of scrollable.scrollIntoView, and it offers only to options: move the focused field to the top or to the bottom. I've performed tests with both solution, and my point of view is that moving the focused field to the top is less disturbing that moving it to the bottom. Maybe a better algorithm would be (?):
    • Do not move the field if it is already in view;
    • If the field is not in the view and the view need to be scrolled up to display it, display the field to the top;
    • If the field is not in the view and the view need to be scrolled down to display it, display the field at the bottom.
  • I've initialy tried to move the field whatever its position in the list, and it appears that moving a field that is at the bottom of the list to the top is disturbing because it scrolls the view to reveal the void after the list. That explain why there is no scroll when you select a field near the bottom of the list.
  • The problem with the "user typing into the Nirvana" is unfortunately already there without my patch, and not fixed with it. Maybe we could create another defect in TRAC for this one ?
  • When windows.scrollIntoView is called, I guess that the native browser scroll is called. That's why my patch update scrollable to that is take into accound the scrollTop and scrollLeft values on domNode to perform its calculations.
  • I confirm that widget.scrollOnFocus is not taken into account, but it doesn't seem either with the current version. So maybe a new ticket should be created for this one ?

comment:5 Changed 6 years ago by Adrian Vasiliu

Welcome, glad to see that you take my comments as constructive criticism. Okay, let me first summarize where we are in my eyes:

  1. Initially (before my changes for #16363), our scrolling machinery was confused (broken) by the automatic browser scroll when a field gets focused.
  2. My changes avoided that, while allowing to navigate with Next/Tab but curiously (and asymmetrically) with Prev/Shift-Tab it doesn't uncover the hidden fields. (There is no scroll event in such cases - would be nice to understand why).
  3. Your proposed patch nicely repairs the regression that I introduced, while introducing other misbehaviors: ergonomical regression on iOS (as described in my previous comment - that I would consider non-fatal, but annoying to see a regression for something that used to work very well) and quite severe regression on some Android, at least Android 4.0.4/stock browser, details below). (Testing would anyway need to cover more platforms.)

At a lower level of detail, the initial trouble was that our scrolling machinery wasn't able to cope with the presence of browser scroll. My patch avoided that by removing the browser scroll as it arrives, and compensating (to get a similar visual result) by applying a similar amount of our own scroll. Now, in your proposal, the scrolling machinery is no longer confused by the browser scroll; we were hoping that once this is done, it is enough, but unfortunately it appears to not be the case. Once it supports the browser scroll, our scrolling machinery confuses the automatic scroll when fields are focused. That is, instead of having scrollable.js confused by the browser scroll, the browser scroll is confused by scrollable.js... Hence the code you added to do the auto-scrolling "manually" (using sv.scrollIntoView), as a replacement of the auto-magic scroll (which in fact is still attempted - with no visible result - by both the browser and dijit's _FormWidgeMixin). All in one, it would seem we're not yet in a safe, sane state of the scrolling machinery.

Hence, I think we should keep digging into it to find a more globally satisfactory solution - not necessarily for all the issues related with input fields inside ScrollableView, but at least to reach a state where we are undoubtedly in a better shape than at the initial step. If we don't find such a better solution, then we'll have to balance the pros and cons and we may prefer the new misbehaviors to the old ones, but let's not jump into it without trying to do better. (The time pressure is not higher than the 1.9.1 deadline.) That's basically my point.

The misbehavior on HTC One X Android 4.0.4: on stock browser it's nearly unusable, because the scroll to top now done on focus events messes up the location of the "duplicated fields" that the browser adds on top of the original field. On Chrome, it's much better, however sometimes after the scroll to top the view scrolls again which finally puts the focused field somewhere in the middle instead of the top, thus making the behavior even less consistent (in addition to the fact that fields at the bottom of the list do not trigger scroll to top).

Another minor remark: at line 238 the listener cleanup code in the patch contains:

if(this._onFocusIn && this.domNode.addEventListener){

I think it would more logically be:

if(this._onFocusIn && this.domNode.removeEventListener){

Finally, to answer directly your points:

  • Adding a dep. on dojo/on wouldn't be a pb, we already require it via dijit/_WidgetBase... But, as I said, widget.on() would be even better.
  • "moving the focused field to the top is less disturbing that moving it to the bottom" - I agree, but still disturbing in comparison with the usual auto-magic scroll.
  • Concerning a possible better algorithm, I don't think it should move the field selectively to either top or bottom, but to the center - more exactly, do the same as the behavior with pure HTML on a given platform. And, again, even better than that would be... if we wouldn't break the auto-magic scrolling such the we don't need to interfere.
  • Yes, the problem with the "user typing into the Nirvana" was already there. Now, the point is whether the changes in the scrolling machinery are safe - more troubles are still there, less we can be confident in the modified scrolling machinery. Per se, creating a new Trac for it wouldn't solve the issue... but if we don't manage to fix it now yes, it needs to be done.
  • "When windows.scrollIntoView is called, I guess that the native browser scroll is called." - if you refer to dojo/windows.scrollIntoView called by dijit, looking at the code shows it's more complex than that, and not easy to understand... But testing with this call removed does not fix our troubles, while the auto-magic scroll still happens (as I noted in the comments of my patch for #16363).
  • "I confirm that widget.scrollOnFocus is not taken into account, but it doesn't seem either with the current version. So maybe a new ticket should be created for this one ?" - I don't think so. dijit's _FormWidgetMixin calls dojo/window.scrollIntoView conditionally depending on widget's 'scrollOnFocus' flag, that is it obeys the flag before attempting to move (visually) the field. This is a situation comparable with the one in your patch, which does not obey the flag. Differently, the current code does not move (visually) the field, it only transfers the scroll amount from the browser scroll to our scrolling machinery.
Last edited 6 years ago by Adrian Vasiliu (previous) (diff)

comment:6 Changed 6 years ago by Adrian Vasiliu

I have tried to investigate whether the regression due to #16363 was there at the origin of this change and to understand what produces it. In practice this may not modify the picture radically, but the fact is that part of the regression for the prev-next navigation with the fix of #16363 wasn't there at the moment this fix was first attached, that is on 21 Nov. 2012 (the patch was firstly mentioned in #16363#comment:1 and was refreshed later), and it is a side-effect of changes elsewhere (in dojox/mobile/common, see below).

I've compared the behavior of the test case originally attached to #16363 (#16363/test_TextBox-in-ScrollableView?.html) in 1.9 vs with a checkout of Dojo at rev. 29961 (15 nov. 2012), that is around the time point when the original patch was attached, on top of which I've put the change in scrollable.js which was committed 4 months later. The behavior differs depending on browsers, but on iPhone 5 iOS 6 the navigation with "prev" works at rev. 29961 and does not work with 1.9.

I have searched for the change which made the difference. There are many "candidates", including the numerous changes in dijit's focus management for form widgets and the changes in dojo/window.scrollIntoView (called by dijit on focus events). But so far I've identified the change which matters as the one in dojox/mobile/common. More exactly: at rev. 29961 just as in 1.9, _ScrollableMixin calls sv.scrollIntoView on resizeAll topics, and the changes in common are such that there is no longer any resizeAll topic published when navigating with prev/next on the virtual keyboard.

Does this mean dojox/mobile/common is faulty? No, I rather think it there was no good reason for common to do resizeAll in such cases. It's just that we were somehow relying on these calls to help for the auto-scroll of input fields in more cases than normally (a normal case is the change of device orientation).

Overall, it remains that for auto-scroll there are many pieces of code in dijit and mobile which are trying to do it (by calling either dojo/window.scrollIntoView or scrollable's scrollIntoView), and it's unclear whether it's a sane thing knowing that the browser seems to do it itself anyway. Things for which it would be nice to get a clear understanding for the longer run.

Last edited 6 years ago by Adrian Vasiliu (previous) (diff)

comment:7 Changed 6 years ago by Adrian Vasiliu

If we'd try to "mimic" what was previously done by _ScrollableMixin, it would give a variant of your patch doing simply:

this._onFocusIn = function(e){
	var elem = win.doc.activeElement;
	if(_this.isFormElement(elem) && dom.isDescendant(elem, _this.containerNode)){
		_this.scrollIntoView(elem);
	}
};

This seems to work globally better (tested so far on FF and Chrome on desktop, and iPhone 5), but of course more testing and a careful comparison of pros and cons is needed.

comment:8 Changed 6 years ago by Sebastien Brunot

Using the last snipet of code is unfortunately not working in google Chrome (desktop) with dojox/mobile/tests/test_ScrollableView-v-vh-vf-inp.html (problems with the view footer bar when navigating the fields, I did not investigate more).

I'm attaching a new version of the patch that fix the problems with android.

comment:9 Changed 6 years ago by Adrian Vasiliu

The trouble in Chrome with the footer bar in test_ScrollableView-v-vh-vf-inp.html when using my snippet over your patch (plus the dep. on dojo/dom which needs to be added) is indeed worrying, but more globally, not only wrt. my snippet. The snipped calls scrollable's scrollIntoView, which is legal... If the footer bar layout is broken, this raises a doubt more globally about the new version of scrollable (now including the management of browser's scroll). So I'm not sure we can spare the effort to understand why the footer is broken by this change.

comment:10 Changed 6 years ago by Sebastien Brunot

This is still a work in progress, but I have a draft of a solution that seems to perform well on desktop browser (IE10, Chrome and Firefox) and on iOS (mobile Safari). I've attached it as patch ticket17062-nonAndroid.patch.

The reference tests I ran to test this draft of a solution are:

1) dojox/mobile/tests/test_ScrollableView-v-vh-vf-inp.html

2) dojox/mobile/tests/test_FilteredList-EdgeToEdgeStoreList-auto.html (keyboard navigation on desktop browsers only)

Unfortunately, this solution is NOT WORKING ON ANDROID stock browser (tested in emulator with Android 4.0.3 and Android 4.2.2).

This draft of a solution need to be:

  • updated to work on android stock browser
  • tested on Blackberry devices

What is implemented in this draft is:

1) scrollable uses the current scrollTop and scrollLeft values to perform its calculations.

2) whenever there is a browser scroll, _ScrollableMixin update the position of any local fixed header and footer (e.g. view header and view footer) to take the current scrollTop value into account.

3) when an element in the scrollable receives the focus, the scrollable scrollTo method is used to display it near the middle of the viewport (while avoiding creating a "void" area at the top of the scrollable).

4) whenever there is a /dojox/mobile/afterResizeAll event (happens on orientation change, and when the keyboard is shown or hidden on Android, for example) and an element is currently focused, _ScrollableMixin uses the scrollable scrollTo method to display the focused element near the middle of the viewport (while avoiding creating a "void" area at the top of the scrollable).

Changed 6 years ago by Sebastien Brunot

Sample solution that works on desktop browser and on iOS. Doesn't work on android stock browser, not yet tested on blackberry devices (IBM CCLA).

comment:11 Changed 6 years ago by Sebastien Brunot

Last attached version of ticket17062-nonAndroid.patch works fine on android emulator, it still need to be tested on real devices.

comment:12 Changed 6 years ago by Adrian Vasiliu

Milestone: tbd1.9.1

comment:13 Changed 6 years ago by Adrian Vasiliu

Owner: changed from Eric Durocher to Adrian Vasiliu
Status: newassigned

comment:14 Changed 6 years ago by Sebastien Brunot

ticket17062.patch has been updated, and tested on:

  • iOS 6 (iPhone 4) => works fine
  • iOS 5 (iPad 1) => doesn't worked, so we keep the default browser behaviour
  • android 2.2 (Samsung Galaxy Tab) => works fine
  • android 3.2 (Samsung Galaxy Tab) => works fine
  • android 4.0.4 (Samsung Galaxy SIII) => works fine, BUT an existing problem is enhanced: if a field is selected near the bottom of the screen, and then the keyboard is hidden and the page is scrolled to the top, the address bar is continualy flashing (appearing / disappearing / appearing / ...).
  • android 4.2.2 (emulator) => works fine
  • blackberry 6 (9800) => works fine
  • blackberry 7 (9860) => works fine
  • Chrome 26 => works fine
  • Firefox ESR 17.0.5 => works fine
  • Internet Explorer 10 => works fine

The patch should also be tested on physical devices for android 4.1 and 4.2, and on a windows phone.

Last edited 6 years ago by Sebastien Brunot (previous) (diff)

Changed 6 years ago by Sebastien Brunot

Attachment: ticket17062.patch added

Support navigating among input field using TAB (desktop keyboard) or NEXT (mobile soft keyboard). (IBM CCLA).

comment:15 Changed 6 years ago by Adrian Vasiliu

I know the testing is not yet finished, and there are further adaptations to be done for some platforms. I will redo a global testing campaign when it will ready. However, at this stage I would have these general comments:

  • The patch modifies drastically the default scrollType for Android (before patch: topLeft mode for Android < 3, after patch: topLeft for Android < 4.1 for now, and might further change). Per se, this might be a good change, but I don't think it should be done inside such a big patch for a specific issue (the one in this ticket). Instead, it would be better to do it in a dedicated ticket, say "Improve scrolling behavior on Android 3+" - of course after testing that the change not only accommodates the current fix, but is good in general (in particular we should look at possible regressions in the context of #14633, where the scroll type has been set to topLeft on Android < 3, and this was said to fix input field "jumping" issues. (For sure many things changed since then, including the changes in the present patch; still, testing in various contexts with and without input fields is mandatory, and a fortiori if this is to be put in 1.9.1.)
  • I'm still concerned by the price this patch pays for fixing the real bug in terms of ergonomical regressions. Since the original patch the behavior changed a lot, thanks for working so hard to overcome the initial troubles. However, I must admit I'm not convinced the new behavior is much more satisfactory than the previous one. In particular, the fact that clicking an item at the end of the list produces a jump which leaves half of the page empty while the footer stays on the bottom is such an ergonomical regression in my eyes.
  • Concerning the changes done in the scrolling machinery to take into account the browser scroll, the fact that this breaks browser's (webkit's) auto-magical scroll for input fields is still worrying me. I mean, this is kind of bad sign... and this holds even if we would successfully work it around as the patch attempted. That said, if we don't find a better solution, and if the behavior is satisfactory, and all tests go well, we might go for it.
Last edited 6 years ago by Adrian Vasiliu (previous) (diff)

Changed 6 years ago by Adrian Vasiliu

Attachment: patch17062-adrian.patch added

Fix autoscroll when navigating among input field using TAB/SHIFT-TAB (desktop) or PREV/NEXT (mobile soft keyboard) - Adrian Vasiliu (IBM, CCLA)

comment:16 Changed 6 years ago by Adrian Vasiliu

So I've attached a much simpler attempt to fix the issue (patch17062-adrian.patch), which just follows the path I've sketched in my #comment:7.

It is not a final patch, just to serve as a reference for a comparison. I've give it just a quick test in a few browsers. With this patch over the current trunk I'm not seeing significant troubles with the footer in test_ScrollableView-v-vh-vf-inp.html, I'm only seeing a footer flashing (flickering) when navigating from a field to another. Do you see more than that? I'm not saying the flashing is not annoying, just that it seems less annoying than other troubles, while being much simpler. Concerning the cause of the flashing, it might be due to the recent changes in #16681 then #17006?

Again, this needs to be further tested before taking any conclusion.

Version 3, edited 6 years ago by Adrian Vasiliu (previous) (next) (diff)

comment:17 Changed 6 years ago by Paul Christopher

I have just tested both suggested patches with http://archive.dojotoolkit.org/nightly/dojotoolkit/dojox/mobile/tests/test_FilteredList-EdgeToEdgeStoreList-auto.html, i.e. I have examined my personal use case which is running dojox/mobile not on a tablet PC but an embeded device (which does not have a virtual keyboard but arrows keys on a remote control only). These are the results:

  • Sadly Adrian's latest patch seems not to show many improvements concerning the original bug description: You cannot scroll the list back. Scrolling forward shows the same behaviour as described in the ticket description above: At the bottom, the list scrolls down by one item. On the next tab key, it jumps by five items or more.
  • Sébastien's patch seems to works - regarding the above use case - nicely instead: You can scroll forward and backward, however - from a usability point of view - it is quite unusual in my opinion, that the focused item is always kept in the middle even at the end of the list (which causes a black background to be shown).

I cannot really judge this: But maybe we just need an extra flag in Sébastien's patch which indicates where to show the focused item, a flag which can be set by the programmer to determine the scrolling behaviour? For devices with a virtual keyboard, always in the middle, for desktop computers and embeded devices with a real keyboard "naturally", i.e. the scrolling behaviour should be the same as for normal non-dojox/mobile scrollable containers?

comment:18 Changed 6 years ago by Adrian Vasiliu

Thanks for the testing, Paul. Can you please tell the OS and browser (including version number) of the embedded device that you have used? Also, did you test by applying the patches against the current trunk?

Last edited 6 years ago by Adrian Vasiliu (previous) (diff)

comment:19 Changed 6 years ago by Adrian Vasiliu

Okay, indeed, in my candidate patch I forgot to take care of focused elements which are not input fields. I've uploaded patch17062-adrian-v2.patch​ to fix this issue, could you please give it a try too? Thanks.

comment:20 Changed 6 years ago by Sebastien Brunot

Hi,

about the ergonomical regression, here is the algorithm I've proposed in comment 4:

  • Do not move the field if it is already in view;
  • If the field is not in the view and the view need to be scrolled up to display it, display the field to the top;
  • If the field is not in the view and the view need to be scrolled down to display it, display the field at the bottom.

Do you think we should give it a try ?

Adrian, in one of your last comment you said: "Concerning the changes done in the scrolling machinery to take into account the browser scroll, the fact that this breaks browser's (webkit's) auto-magical scroll for input fields is still worrying me." => What do you mean exactly by breaking the browser's auto scroll for input fields ? Do you mean that you would prefer that we rely on the browser native auto scrolling when it is there, and only use the auto scrollable scroll when this is not provided by the browser ?

comment:21 Changed 6 years ago by Sebastien Brunot

Another last remark: I've been testing my last patch on an Android 4.1 device and it's simply not working. So there is still some work to do to supports all platforms (hopefully, android 4.1 and 4.2 are the two last ones...).

comment:22 Changed 6 years ago by Adrian Vasiliu

Sébastien: "What do you mean exactly by breaking the browser's auto scroll for input fields ?"

As you know, the browser takes normally care of triggering a page scroll as needed during the navigation from a focused element to another focusable element. Now, with our scrolling machinery, the browser apparently doesn't correctly compute the visibility of the elements, and this still holds with your proposed changes, hence in some cases doesn't trigger a page scroll (thus we do not get the scroll event, and we need to rely on focus events). Ideally, our scrolling machinery shouldn't interfere with this mechanism. There was hope to reach this state once the scrolling machinery is modified to take into account the browser scroll (as we have attempted long ago, and as implemented in your patch), and this doesn't appear to be the case. That's what I meant.

comment:23 Changed 6 years ago by Adrian Vasiliu

About the algorithm you proposed in comment 4: I'm afraid it would still be disturbing. Ideally, the behavior should be as close as possible as the "native" one, that is without our scrolling machinery. And I would think that on most browsers this "native" behavior doesn't move the focused element to either the very top, nor the very bottom.

I have the impression that relying on the (unmodified) scrollable.scrollIntoView() (as in my patch variant) provides a result which is closer to the "native" behavior.

Last edited 6 years ago by Adrian Vasiliu (previous) (diff)

comment:24 Changed 6 years ago by Paul Christopher

Adrian, I have not tested it on a mobile device. I have just used Chrome on Win7.

One thing I ran into with Sébastien's patch: I have a huge list in a ScrollablePane. The <ul> has tabindex=0, all <li> elements tabindex=-1. When focusing the list as a whole, i.e. the <ul> element (which is of course much bigger than the ScrollablePane), the ScrollablePane scroll to the very middle of the list so that,in a list of 200 items, item 100 et seq. are shown. This wasn't the case with the current 1.8 version of scrollable. It shows items 1-10.

comment:25 Changed 6 years ago by Adrian Vasiliu

Paul: "I have not tested it on a mobile device. I have just used Chrome on Win7."

Okay, it wasn't clear to me that the embedded device you mentioned runs Win7. BTW, if you have a minute to test patch17062-adrian-v2.patch​ it would be helpful.

comment:26 Changed 6 years ago by Paul Christopher

No, the embedded device runs on ubuntu and uses chromium. Regarding your patch: This one works nicely now! However it suprised me a little bit, that you now keep the focused item right in the middle of the container as Sébastien does. Just check ​http://archive.dojotoolkit.org/nightly/dojotoolkit/dojox/mobile/tests/test_FilteredList-EdgeToEdgeStoreList-auto.html on some desktop browser. I does not feel really natural there: Normally scrolling in a container only happens, if you are at the visible top or bottom of a container and if you try to focus an item, that is not yet visible. Depending on the scrolling direction, the focused item is always kept right at the top or bottom of the container. Why do you center the focused item? Because of the virtual keyboard?

Changed 6 years ago by Adrian Vasiliu

Attachment: test_pure-HTML.html added

For comparison purposes - a pure HTML (no Dojo) test page containing many focusable elements (here, input fields).

comment:27 Changed 6 years ago by Adrian Vasiliu

"the embedded device runs on ubuntu and uses chromium." - ok. You wrote previously "I have just used Chrome on Win7.", I guess for a distinct test. Anyway.

"Normally scrolling in a container only happens, if you are at the visible top or bottom of a container and if you try to focus an item, that is not yet visible. Depending on the scrolling direction, the focused item is always kept right at the top or bottom of the container." - this is not the behavior I see with Chrome 26 (other browsers behave differently) on Win7 using a pure HTML test case (no Dojo). I've attached test_pure-HTML.html to help for this comparison. After reducing the window height such that the entire list is no longer visible, and after pressing TAB repeatedly till the bottom of the visible part of the list, the next TAB produces a scroll such that the next item jumps to the middle of the window. The same happens in both directions (when navigating towards the bottom or top of the list). Now, given the difference of behavior between browsers, and if we want to stay simple, it's a matter of chosing the best compromise.

comment:28 Changed 6 years ago by Paul Christopher

Adrian and Sébastien, thank you very much for working so hard on this quite though topic! If I post any comments here: They are not meant as a mean criticism, but as a constructive input. I do that all mostly in my free time! If you think, it is not constructive, please tell me.

@Adrian: Yes, you are perfectly right about the behaviour of Chrome! I have not noticed that, but - I have to say - I don't think they have done a good job with this. IE and Firefox behave the same (and the way I have described in comment 26), and I personally like this more.

But apart from the "question of personal liking and disliking", I still have those two issues with your patch:

  • You are right about the behaviour of Chrome, but if you focus the inputs 30, 31, 32 in test_pure-HTML.html, Chrome does not extend the page with virtual spaces that are not present in the original HTML, i.e. input 30, 31, 32 are not kept in the middle anymore but left where they are, i.e. the focus box moves naturally down.
  • See test_pure-HTML_BigNodes.html: If a node is bigger than the visible area, Chrome does not center this node but leaves it where it is. Compare this to test_BigNodes.html (a test case for dojox/mobile and your patch). Focus the list (wrapping div) as a whole with the tab key, and the list items 9 et seq. are shown (whereas items 1 et seq. should be displayed).

All in all, please test test_BigNodes.html. It is not really usable for me at the moment:

  • When focusing the list as a whole, the list is scrolled to the middle.
  • If you go on with tab based navigation, suddenly the list jumps up again to list item 1.
  • And at the end of the list, virtual space is created and makes the list shrink when navigating (since the focused item is always kept in the middle which should not be the case in my opinion).

Please not: test_BigNodes.html is just a simulation of my real use case: There, the list's <ul> has tabindex=0, each list item <li> tabindex=-1. The list is navigated with the arrow keys. To achieve this, I have written a keynav mixin for lists. If you are interested, I can contribute this mixin (it's like longListMixin) to dojox/mobile.

Changed 6 years ago by Paul Christopher

Attachment: test_BigNodes.html added

Changed 6 years ago by Paul Christopher

comment:29 in reply to:  28 Changed 6 years ago by Adrian Vasiliu

For now, just to clarify this point:

Replying to Paul Christopher:

If you think, it is not constructive, please tell me.

On the contrary, I fell your input (on this ticket as on others) as very constructive. I should have said "thanks" more often :-)

To go back to the technical issues: we will perform a new testing campaign including your new test cases. Stay tuned...

Changed 6 years ago by Adrian Vasiliu

Attachment: patch17062-adrian-v2.patch added

Fix autoscroll when navigating among input field (and among any focused elements) using TAB/SHIFT-TAB (desktop) or PREV/NEXT (mobile soft keyboard) - Adrian Vasiliu (IBM, CCLA)

Changed 6 years ago by Adrian Vasiliu

Attachment: patch17062-adrian-v3.patch added

Fix autoscroll when navigating among focusable elements using TAB/SHIFT-TAB (desktop) or PREV/NEXT (mobile soft keyboard) - Adrian Vasiliu (IBM, CCLA)

comment:30 Changed 6 years ago by Adrian Vasiliu

  • To take into account the case of "big" nodes raised by Paul, I've uploaded a v3 of my patch variant. Paul, in my testing it seems to do the trick for your test_BigNodes.html, hope it also does the trick in your real use case (that you said it's a bit different). If you can test, just tell how it goes. (I don't think this v3 is perfect... but hopefully a simple but good enough compromise.)
  • Also uploaded a corrected version of the "v2". Previously, it contained a different code than the one I wanted to put into it... (sorry). That said, v2 is now superseded by v3.
  • Sebastien, do you still plan to work on your patch? I refer here to the pending issues on Android 4.1/4.2 etc., and maybe other improvements. Just to know if it's a good moment for a comparative testing campaign.

comment:31 Changed 6 years ago by Paul Christopher

Wow, v3 is exactly the thing I was looking for! It works great for big nodes. You can scroll the list forward and backward nicely. That's neat. The only minor thing I noticed: If moving forward in the list with the Tab key and then changing the direction with Shift + Tab to go backwards again, the focused item is always kept at the botom of the scrollable view. Instead, I think, the focused item should move upwards until the top of the scrollable view is reached? Do you know what I mean? Just take test_BigNodes.html and start to "tab" into the list. After item 8 change the direction with Shift + Tab. The focused item is always kept at the bottom now, i.e. the focused item is always the last visible entry of the list. And that's not how FF or IE behave. Both browser'S move the focus upwards until the top is reached.

comment:32 in reply to:  31 Changed 6 years ago by Adrian Vasiliu

Replying to Paul Christopher:

Do you know what I mean?

Yes, I do know. As previously said, the picture is complex, because different browsers behave differently. Besides Chrome, Safari does not behave the way you described as your "ideal", on either iOS or Windows (and there are even differences betw. Safari iOS and Safari/Windows?, which are understandable as an adaptation to the presence of virtual keyboard). If we would enforce the behavior that you describe on all browsers, it would be disturbing for users of browsers that have a different "native" behavior.

All in one, as already said, it's all a matter of compromise, knowing that minimizing the amount of code is also a quality criteria. At a minimum, we should make it usable without major ergonomy troubles on all supported browsers. We will see if the current compromise in this v3 can be further improved - or whether Sebastien's way (or a future evolution of it) would be preferable. Thanks again for your testing and feedback.

comment:33 Changed 6 years ago by Sebastien Brunot

I think it would be usefull to test the last v3 version of the patch on each platform, to report the results and then build from there. I don't see any point in pursuing work on a "competing" patch...

Changed 6 years ago by Adrian Vasiliu

Attachment: patch17062-adrian-v4.patch added

Fix autoscroll when navigating among focusable elements using TAB/SHIFT-TAB (desktop) or PREV/NEXT (mobile soft keyboard) - Adrian Vasiliu (IBM, CCLA)

comment:34 in reply to:  33 Changed 6 years ago by Adrian Vasiliu

Replying to sbrunot:

I think it would be usefull to test the last v3 version of the patch on each platform, to report the results and then build from there.

I've attached a v4, which aims to improve over v3 by making the behavior more systematic and by letting the browser handle the auto-scroll itself in more cases than in v3. In terms of behavior it is kind of mix of Paul's "ideal" and Sebastien's point about triggering scroll only for elements outside the visible bounds (while not actively attempting to scroll to center - Safari/iOS still does it by itself). A more exhaustive testing needs to be done (I plan to do it, any other testing is welcome).

I don't see any point in pursuing work on a "competing" patch...

As we don't have any "perfect" solution, any attempt to get a better solution is welcome. I don't see it in terms of competition but as a collective effort to find the best solution. I'm personally not sure mine is better (except from the simplicity criteria), hence the need of testing. Besides that, your patch, Sebastien, covers different points which deserve anyway to be treated separately (improved positioning of header/footer and maybe revisiting the choice of the default scrollType value on each platform).

comment:35 Changed 6 years ago by Paul Christopher

Version 4 is simply perfect in my eyes! Very well done Adrian! Small code size, but yet enough to reach the desired aim.

Concering browser behaviour: You are right and I haven't thought about that: Different browsers have differnt behaviour. And this is hard, since scrollable view tries to mimic the native behaviour. And if you don't want to enforce a wrong scrolling behaviour which does not match the platform defaults, there is no way round to code this behaviour in and use has()/browser sniffing to choose the correct scrolling algorithm depending on the browser currently used. But that's maybe a little bit overpowered? I am happy with V4! :-))

comment:36 Changed 6 years ago by Adrian Vasiliu

This v5 just adds to v4 the respect of the "disableScrollTouch" flag (and minor improvements in comments).

comment:37 Changed 6 years ago by Damien Mandrioli

Patch v5 behavior looks correct. Works on Android (when keyboard arrows are available) and iOS. However, lots of devices do not provide keyboard arrows: Windows Phone, BB10 and most of Android devices.

comment:38 in reply to:  37 Changed 6 years ago by bill

Summary: [dojox/mobile] Cannot scroll list up and down using the keyboard[regression] Cannot scroll list up and down using the keyboard

Are you sure it's OK to just overwrite _onFocus?

comment:39 Changed 6 years ago by Adrian Vasiliu

Good point Bill. As you have probably guessed, the intention wasn't at all to overwrite _onFocus(), but just to store it such that we can remove it in cleanup(). And in practice it is typically not an overwrite, because nor dojox/mobile/scrollable, nor the modules on which it is typically mixed contain _onFocus(). But we can't exclude that, for instance, dijit/_FocusMixin (which adds _onFocus() to WidgetBase?) comes into the picture. So you are right, to be safe we shouldn't use the same name. I'll rename it right away into something more specific. Thanks Bill.

Changed 6 years ago by Adrian Vasiliu

Attachment: patch17062-adrian-v5.patch added

Fix autoscroll when navigating among focusable elements using TAB/SHIFT-TAB (desktop) or PREV/NEXT (mobile soft keyboard) - Adrian Vasiliu (IBM, CCLA)

comment:40 Changed 6 years ago by Adrian Vasiliu

Following Bill's point, reuploaded v5 with _onFocus renamed into _onFocusScroll (to avoid risk of name clash).

comment:41 in reply to:  37 Changed 6 years ago by Adrian Vasiliu

Replying to dmandrioli:

Patch v5 behavior looks correct. Works on Android (when keyboard arrows are available) and iOS.

Thanks Damien for the testing.

comment:42 Changed 6 years ago by Adrian Vasiliu <vasiliu@…>

Resolution: fixed
Status: assignedclosed

In fc5d66be761e7e2767e134af5475b3178e8a3973/dojox:

Error: Processor CommitTicketReference failed
Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 

comment:43 Changed 6 years ago by Adrian Vasiliu <vasiliu@…>

In 06ac34974206f3606f61f7dc35d6ea362edf53a0/dojox:

Error: Processor CommitTicketReference failed
Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 
Note: See TracTickets for help on using tickets.