Skip to main content

Search and Top Navigation

#10686 closed bug (notabug)

Opened November 05, 2014 04:28PM UTC

Closed October 17, 2015 09:02PM UTC

Last modified October 18, 2015 06:40PM UTC

Dialog: Focus restore on dialog close can cause unwanted scrolling

Reported by: usmonster Owned by:
Priority: minor Milestone: none
Component: ui.dialog Version: git (not yet released)
Keywords: Cc:
Blocked by: Blocking:
Description

The current implementation of #8730 can cause unwanted scrolling when closing a dialog.

The culprit is here:

https://github.com/jquery/jquery-ui/commit/14691ae#diff-bd5d7db0e6a3ff783a7ec2ccf261f6b4R251

The problem is that this implementation assumes that the dialog was opened via a user interaction with a :focusable element ("this.opener" in the code--a not entirely accurate name), which is not always the case. Even when it is, the element may not be in view. If the originally-focused element is not in view at the time of closing the dialog, this call to .focus() will result in it being unexpectedly scrolled into view. This produces a jarring effect for the user.

Example of this behavior: http://jsfiddle.net/jzxseunh/1/

A real use case:

I have set up a shortcut key that opens a dialog for taking a screenshot of the current viewport. Clicking "OK" in the dialog closes it and triggers the screenshot. There is an element somewhere at the top of the page that has the focus. I scroll down to a section of the page where the focused element is not in view and trigger the screenshot, but it will only capture the top of the page since the focus was restored and the page was scrolled before the screenshot was executed.

There are several other typical and valid use cases, and perhaps also some hacky workarounds, but the bottom line is that closing a dialog should probably not ever trigger scrolling.

Maybe, instead of calling .focus() inside the above-referenced if condition, the focusing step could be wrapped in a save/restore of the scroll position of this.opener.scrollParent()? (An optimization could do make this happen conditionally, based on whether or not the element was already visible inside its scrollparent and the viewport).

Does this sound reasonable? Thoughts?

Attachments (0)
Change History (12)

Changed November 05, 2014 05:50PM UTC by usmonster comment:1

_comment0: To be clear, my proposal is to replace this line: \ \ {{{ \ #!js \ if ( !this.opener.filter( ":focusable" ).focus().length ) { \ // ... \ }}} \ \ ...with something like this: \ \ {{{ \ #!js \ var isOpenerFocusable = this.opener.is( ":focusable" ); \ if ( isOpenerFocusable ) { \ var openerScrollParent = this.opener.scrollParent(), \ scrollTop = openerScrollParent.scrollTop(), \ scrollLeft = openerScrollParent.scrollLeft(); \ this.opener.focus(); \ openerScrollParent.scrollTop( scrollTop ).scrollLeft( scrollLeft ); \ } else { \ // ... \ }}} \ \ Plus any details, coding style, or optimizations I may have missed.1415210517139899

To be clear, my proposal is to replace this line:

#!js
if ( !this.opener.filter( ":focusable" ).focus().length ) {
    // ...

...with something like this:

#!js
if ( this.opener.is( ":focusable" ) ) {
    var openerScrollParent = this.opener.scrollParent(),
        scrollTop = openerScrollParent.scrollTop(),
        scrollLeft = openerScrollParent.scrollLeft();
    this.opener.focus();
    openerScrollParent.scrollTop( scrollTop ).scrollLeft( scrollLeft );
} else {
    // ...

Plus any details, coding style, or optimizations I may have missed.

Changed November 06, 2014 09:29PM UTC by tj.vantoll comment:2

This is an interesting one. I understand your use case (and think it's valid), but I'm not sure it's common enough to be accounted for in the widget itself. Especially because I can think of cases where you do want to scroll on close.

For your purposes you can change your code sample into a quick dialog extension:

$.widget( "ui.dialog", $.ui.dialog, {
    close: function( event ) {
        var openerScrollParent = this.opener.scrollParent(),
            scrollTop = openerScrollParent.scrollTop(),
            scrollLeft = openerScrollParent.scrollLeft();

        this._super( event );
        
        openerScrollParent.scrollTop( scrollTop ).scrollLeft( scrollLeft );        
    }
});

Here's that extension in action: http://jsfiddle.net/tj_vantoll/scfjdzp4/. My opinion is that this use case is best handled as an extension, but I'll leave this ticket open for others to chime in. Suggestions on how the widget could be more extensible are also welcome.

Changed November 06, 2014 11:10PM UTC by usmonster comment:3

Thanks for the response. The use case I gave is just one of many other valid ones I can think of. For example, if you've scrolled a page and then try to close the tab or window (accidentally or on purpose), a dialog could open to ask if you really want to leave the page. Clicking "no" in this case would scroll the page and lose your place.

There are many such use cases, the general theme being "the user scrolls the active element out of the viewport, a dialog is triggered without the user's interaction with the active element or any other :focusable element, said dialog is closed, the document scrolls 'by itself', the user is confused." Though I actually can't think of a case where you'd want closing a dialog to change the scroll position of the page, especially as default behavior.

Restoring the ''focus'' is fine and correct, but the scrolling is, in my opinion, an occasional side effect of this browser a11y logic whose possibility was overlooked in the initial feature implementation.

In any case, my opinion is that the widget should be the one that ''does'' allow the scroll, and that the default behavior should restore both the focus AND the scroll position, if needed. Other opinions are definitely welcome! Thanks again.

Changed November 07, 2014 03:37AM UTC by tj.vantoll comment:4

I think it would be disorienting to a user to have focus placed in a field that is not visible. For instance the user may try to use the arrow keys to move around the page and be confused why their key presses are not working. Browsers scroll to focused fields for a reason and I think we should respect that behavior, at least by default.

Changed November 07, 2014 08:56AM UTC by usmonster comment:5

Browsers scroll to newly-focused elements (not just form fields) for a11y reasons, but I'm not convinced these reasons apply to the case we have here, where we're just trying to restore pre-dialog state. Official a11y folks can weigh in here.

In your example, if the user scrolls a focused form field out of view (only done via mouse/touch or Page Up/Down) and begins to type, the browser will scroll the field back into view. This is default browser behavior. My suggestion won't change this, but it will actually fix the cases where the user wants to continue using the mouse/page keys to scroll from where they left off.

Changed November 07, 2014 01:35PM UTC by tj.vantoll comment:6

Here's the specific situation I'm talking about: if you open this example in Chrome (http://jsfiddle.net/tj_vantoll/scfjdzp4/), close the dialog, and press the down arrow key (in an attempt to scroll down) it won't do anything. In general I just don't like focusing elements that are off the potentially off the screen by default.

I think we need some other opinions :)

Changed November 07, 2014 03:54PM UTC by usmonster comment:7

_comment0: I think I understood the example situation and am sympathetic to your motivation, though I try not to think of it as an active focusing of an element, but instead as a passive restoration of page state (i.e. pre-dialog focus & scroll position). :] My proposal's intention is just to make the jQUI dialog behavior more consistent with what the browser already does natively with alert/confirm/prompt: http://jsfiddle.net/scfjdzp4/1/ . \ \ Here's another real use case to consider where ''not'' restoring the focus to an out-of-view element would be undesirable: I have a list widget that listens to the arrow keys for list navigation while it's focused. Navigating to an item gives the element the focus. I scroll the page so the active item is out of view. A dialog is opened via some non-focus-changing event. When it closes, I don't want the list to lose focus. (I also don't want the page to jump back to the active list item.) \ \ Ok, I'll be quiet now, until other opinions are made known. :]1416397058095070

I think I understood the example situation and am sympathetic to your motivation, though I try not to think of it as an active focusing of an element, but instead as a passive restoration of page state (i.e. pre-dialog focus & scroll position). :] My proposal can be seen as making the jQUI dialog behavior more consistent with what the browser already does natively with alert/confirm/prompt: http://jsfiddle.net/scfjdzp4/1/ .

Here's another real use case to consider where ''not'' restoring the focus to an out-of-view element would be undesirable: I have a list widget that listens to the arrow keys for list navigation while it's focused. Navigating to an item gives the element the focus. I scroll the page so the active item is out of view. A dialog is opened via some non-focus-changing event (e.g. pressing the ? key to open a help menu dialog). When I close the dialog, I expect the focus to "remain on" (i.e. be restored to) the active list item, but I don't want the page to scroll it back into view. Either not restoring focus or restoring focus without also restoring the scroll position would leave or put the page in an unnatural state, forcing the user to intervene.

Ok, I'll be quiet now, until other opinions are made known. :]

Changed October 17, 2015 09:02PM UTC by hmethvin comment:8

resolution: → notabug
status: newclosed

Closing based on prior discussion.

Changed October 17, 2015 11:01PM UTC by usmonster comment:9

_comment0: Hello! I just wanted to advocate for this ticket again. \ \ I'm not sure the prior discussion ever reached a proper conclusion, so would it be possible to reopen the ticket? \ \ The open, essential question for me is whether or not we want the focus-restoring behavior of closing a jQuery UI Dialog to be, by default, closer in line with the standard behavior of closing a JS alert/confirm/prompt dialog (i.e. pre-dialog focus is restored, without scrolling). My opinion is that yes, it should. \ \ I'm not yet convinced that the use case given by tj.vantoll is a valid counter-argument to the ones I provided. In the specific case TJ cites, any user interaction with the page after the dialog is closed is identical to what would have happened if no dialog had opened in the first place. To me (and I hope not just to me), this is correct behavior. \ \ It would be great if anyone else on the jQuery UI team or even from the community had a chance to provide feedback on this issue, so it's not just two voices re-stating our opinions. :] Thanks!1445123074789053

Hello! I just wanted to advocate for this ticket again.

hmethvin: I'm not sure the prior discussion ever reached a proper conclusion, so would it be possible to reopen the ticket?

The open, essential question for me is whether or not we want the focus-restoring behavior of closing a jQuery UI Dialog to be, by default, closer in line with the standard behavior of closing a JS alert/confirm/prompt dialog (i.e. pre-dialog focus is restored, without scrolling). My opinion is that yes, it should.

I'm not yet convinced that the use case given by tj.vantoll is a valid counter-argument to the ones I provided. In the specific case TJ cites, any user interaction with the page after the dialog is closed is identical to what would have happened if no dialog had opened in the first place. To me (and I hope not just to me), this is correct behavior.

It would be great if anyone else on the jQuery UI team or even from the community had a chance to provide feedback on this issue, so it's not just two voices re-stating our opinions. :] Thanks!

Changed October 18, 2015 09:30AM UTC by scottgonzalez comment:10

The resolution by hmethvin was the result of multiple team members and community members weighing in. In the full year that this ticket was open, we only heard one person request this change.

Changed October 18, 2015 05:47PM UTC by usmonster comment:11

Thanks for the elaboration, scott.gonzalez. I wasn't aware that any other team member had expressed an opinion on this (there is only feedback from one on this ticket), so perhaps I should've used another forum to make the issue more visible and to get more explicit feedback on it.

Since the focus-restoring behavior this ticket seeks to correct is not limited to jQuery UI's Dialog implementation, I will seek feedback elsewhere and see if other a11y experts or folks from the greater web standards/development community can chime in. It seems like it may be a hole in a standard or some guidelines somewhere (maybe here or here?)...

If I learn something interesting or useful, I'll update here for posterity if that's OK.

Changed October 18, 2015 06:40PM UTC by scottgonzalez comment:12

If I learn something interesting or useful, I'll update here for posterity if that's OK.

Absolutely. We're always willing to revisit decisions based on new/expanded information.

Note that the current implementation just follows the recommendation from the last link you provided (the ARIA authoring practices):

When the dialog is closed or cancelled focus should return to the element in the application which had focus before the dialog is invoked. This is usually the control which opened the dialog.