Arbitrary scroll on dialog open

I ran into this issue with layout_paragraphs' field widget modals too. I don't believe the bug is specific to the media library or any other specific dialog. The scroll is simply due to an element being given focus - initially because that happens before the dialog contents have been fully rendered, but also thinking conceptually - what if a dialog's contents start with a long set of text before any focusable elements? Is it really right that those are scrolled past, in order for focus to be given to the first element? That feels like trading one accessibility concern for another, if I'm honest.

The decision to put focus into the dialog contents comes from way back on https://bugs.jqueryui.com/ticket/3327/ (17 years ago!) which does seem reasonable, just without anticipating this. Would some kind of focus trap at the top of dialog contents be appropriate?

I wonder if this kind of bug will just have to wait for 📌 Add a native dialog element to deprecate the jQuery UI dialog Needs work to replace the jQuery UI implementation, or if it can be proven to be a bug specifically to Drupal's use of it?

Nice find @james.williams, I think your analysis is correct.

Seems like it happens when the height of the dialog exceeds the height of the screen. On a fresh install my first media upload didn't trigger it, but opening it a second time with a media item in the library causes the scrolling.

Here's an old post where someone claims a dummy link worked: https://forum.jquery.com/portal/en/community/topic/default-scroll-positi...

I've been trying some related things but they're not quite working.

I also tried https://www.drupal.org/project/dialog_native →

Doesn't seem to have this problem, but the default styles need adjustment.

Thanks. I dug a bit further too and found that yes, it would probably only happen for dialogs with content that is higher than the available space in the viewport. And perhaps more interestingly, only when the height option is passed as a percentage. (eg. The media library, which specifies it as the string 75%). I checked the jquery UI API documentation for the Dialog widget, which says ‘auto’ is apparently the only string it would support! So I think this is all due to Drupal abusing an unsupported possibility for the height option!

So a workaround is to alter the specific cases where a percentage height is specified, and override that with ‘auto’ or a specific numeric height.

Drupal's copy of jQuery UI Dialog is modified.

This line says it converts percentage to pixels: https://git.drupalcode.org/project/drupal/-/blob/11.x/core/misc/dialog/d...

Sure - so Drupal has added some support for percentage heights, but this is an example of a bug which that addition has not accounted for.

For what it's worth, this is the 'fix' I've added locally, because I'm confident that at least on my project, we'd rather always have the user scrolled to the top, rather than scrolled to the element that has focus:

(($) => {

  /**
   * Ensure new dialogs have their scroll position set to their top.
   *
   * The first element in dialogs is given focus, but that can be before the
   * dialog has its final height, so the scroll position can be incorrect. It is
   * technically possible for the focussed element to end up below the viewed
   * area, but we assume our implementation will not suffer that problem.
   */
  window.addEventListener('dialog:aftercreate', (event, dialog, $dialog) => {
    if (
      typeof event.settings?.height === 'string'
      && event.settings.height !== 'auto'
      // When the height is not auto, the dialog widget code totally assumes it
      // must be a number. But Drupal components are sometimes setting it to be
      // a percentage string (e.g. '75%'). When this happens, the dialog
      // momentarily loses almost all height. So when an element in it is given
      // focus, the contents are scrolled to show it.
      && isNaN(event.settings.height - 0)
    ) {
      const $element = $dialog || $(event.target);
      $element.scrollTop(0);
    }
  });

})(jQuery);

I add that in a javascript file, which I've then used a hook_library_info_alter() to add to the dialog library definition:

/**
 * Implements hook_library_info_alter().
 */
function MYMODULE_library_info_alter(&$libraries, $extension) {
  // Add to core's drupal.dialog library because otherwise the contents of
  // dialogs that use a percentage height are slightly scrolled. This is due to
  // jQuery UI giving focus to the first element inside the dialog, but also not
  // fully supporting percentages for the height option.
  if ($extension == 'core' && isset($libraries['drupal.dialog'])) {
    $path = '/' . \Drupal::service('extension.list.module')
      ->getPath('MYMODULE');

    $libraries['drupal.dialog']['js'][$path . '/dialog-scroll-fix.js'] = [];

    if (!in_array('core/jquery', $libraries['drupal.dialog']['dependencies'])) {
      $libraries['drupal.dialog']['dependencies'][] = 'core/jquery';
    }
  }
}

For a more general solution, I think we might need some accessibility/usability expert advice as to whether it's better to scroll to the top on opening a dialog, even if that pushes the focussed element out of the viewport, or what alternative should be pursued.

Starterkit already has a dialog library that extends core/drupal.dialog, so I was able to just add the js in there for my theme without the library info hook.

https://git.drupalcode.org/project/drupal/-/blob/11.x/core/themes/starte...