Opened 6 years ago

Closed 5 years ago

Last modified 5 years ago

#10686 closed bug (notabug)

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?

Change History (12)

comment:1 Changed 6 years ago by usmonster

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

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

...with something like this:

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.

Last edited 6 years ago by usmonster (previous) (diff)

comment:2 Changed 6 years ago by tj.vantoll

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.

comment:3 Changed 6 years ago by usmonster

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.

comment:4 Changed 6 years ago by tj.vantoll

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.

comment:5 Changed 6 years ago by usmonster

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.

comment:6 Changed 6 years ago by tj.vantoll

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 :)

comment:7 Changed 6 years ago by usmonster

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. :]

Last edited 6 years ago by usmonster (previous) (diff)

comment:8 Changed 5 years ago by hmethvin

Resolution: notabug
Status: newclosed

Closing based on prior discussion.

comment:9 Changed 5 years ago by usmonster

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!

Last edited 5 years ago by usmonster (previous) (diff)

comment:10 Changed 5 years ago by Scott González

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.

comment:11 Changed 5 years ago by usmonster

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.

comment:12 Changed 5 years ago by Scott González

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.

Note: See TracTickets for help on using tickets.