Problem/Motivation

While investigating #3631339: [random test failure] FieldTest::testModalDialogTitle fails intermittently, I found that Drupal dialogs first open at the wrong position, which is then corrected about 20ms later:

Before (1) Before (2)

This fitted surprisingly well with a subconscious aversion to Drupal dialogs that I'd had for some time. Like an unexplainable, involuntary, autonomic reaction, something just felt off.

Now I can explain it, managed to capture the quite perceptible jump in a video, and we may now fix its cause.

While not reaching the three flashes threshold of WCAG 2.3.1/2.3.2, the abrupt repositioning is still distracting and may cause averse reactions in other users as well. Unnecessarily abrupt visual motion may even contribute to dizziness, nausea or headaches in motion-sensitive users, as described in Understanding SC 2.3.3.

Steps to reproduce

While not necessarily restricted to Views, to reproduce the issue please:

  1. Edit some view in Views UI.
  2. Scroll down and open some dialog.
  3. Notice (or film in slow-motion) the dialog first appearing too far down, then jumping upwards.

Proposed resolution

To remove the delayed initial resize, we need to calculate the dialog's size and position correctly before opening the dialog. Getting there however requires a few related bugfixes:

  1. Normalize numeric strings with or without "px" to numbers, and convert percentage heights to pixels.
  2. Normalize autoResize to a boolean after beforecreate listeners had a chance to modify it.
  3. Set position: fixed for dialogs with autoResize enabled before jQuery UI calculates the initial coordinates.
  4. Preserve positioning options that keep non-modal dialogs within the viewport.
  5. Retain autoResize update on viewport and displacement changes using the shared calculation methods.

To avoid regressions, expand our quite a bit lacking test coverage for dialog sizes and positions, including modal and non-modal dialogs, reopening, and option changes.

Remaining tasks

Accessibility and Javascript review.

User interface changes

None, besides dialogs opening at their correct position immediately:

After

Introduced terminology

None.

API changes

None.

Data model changes

None.

Release notes snippet

Dialogs no longer jump into position after opening.

CommentFileSizeAuthor
no-flickering-after.jpg121.09 KBpancho
flickering-before (2).jpg110.95 KBpancho
flickering-before (1).jpg117.71 KBpancho

Issue fork drupal-3633792

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

pancho created an issue. See original summary.

pancho opened merge request !17617

pancho’s picture

pancho
Location UTC+2 🇪🇺 EU
commented
Issue summary: View changes
Status: Active » Needs review

Ready for review. This should also resolve #3631339: [random test failure] FieldTest::testModalDialogTitle fails intermittently.

The underlying problems in unpatched Core are:

In unpatched Core, jQuery UI first positions the dialog with position: absolute.
It's only after the dialog has already opened that Drupal's autoResize code changes it to position: fixed, however without recalculating its coordinates. On a scrolled page, this can put the dialog too far down, potentially leaving its title outside the viewport. A delayed correction then makes the dialog jump into position.

Dialogs with percentage-based height

The delayed resizing corrects the position by triggering _position(), but only after the dialog has already opened. This makes the dialog jump into position, for both modal and non-modal dialogs.

Dialogs with numeric height

With a numeric maxHeight, the delayed resize does not perform the percentage conversion nor the appropriate position correction.
Additional problem is: after the AJAX round trip, the height value reaches our JS as the string "400" and is passed as such to jQuery UI, which explicitly checks its typeof and then discards the value rather than silently converting it into 400.
And as resetPosition() replaces the position options without preserving collision: 'fit', the top of an oversized non-modal dialog can move outside the viewport. Views UI normally uses 95%, but numeric sizes are relevant for other callers.

Resulting random test failures

For modal dialogs, there is another consequence: bodyScrollLock.lock() sets overflow: hidden on the root element. The misplaced title can then be clipped outside the viewport, so ChromeDriver treats it as hidden. getText() may return an empty string, while waitForElementVisible() may time out.
This may explain the intermittent failures in #3631339: [random test failure] FieldTest::testModalDialogTitle fails intermittently, when the test checks the title before the delayed correction happens. The timing differences between sometimes failing CI runs and mostly passing local test runs are not yet fully understood.

This MR converts the requested sizes and sets fixed positioning before jQuery UI calculates the initial coordinates, preserving collision handling. Dialogs now open at their intended size and position, without needing the delayed initial correction. autoResize updates when the viewport or Drupal displacement offsets change remain supported.

smustgrave’s picture

smustgrave commented

Was this written with AI by chance?

pancho’s picture

pancho
Location UTC+2 🇪🇺 EU
commented

Stephen,

yes, as I already explained when you last asked me, I do use AI for assistance. But no, it's not written by AI. I do take responsibility for every single word I chose and every deficit I overlooked.

Coming from the flaky tests issue, I have, however, put hours over hours – including a whole sleepless night – into thoroughly investigating this problem, analyzing ChromeDriver's behaviour, confirming and documenting the UI bug in practice, developing a quick fix, expanding it into a proper, thorough solution, making sure all relevant scenarios are properly taken care of, and expanding the test coverage.

Being confronted with yet another gotcha question about AI, without any concrete concern about the contribution itself, is disheartening. I may be on the more verbose side, but I think the level of detail is warranted by the complexity of this issue. Whenever my issue summaries for rather obvious tasks or fixes are on the shorter side, such as in:

you're not ok with it either.

I've tried to take it in good humour, but that's getting increasingly difficult.

If there's something wrong with my explanation, design decisions, technical scope, implementation or test coverage, I'm very happy to discuss it. That's the feedback I'm hoping for and that I would find most helpful. Just tell me what's wrong and I'll fix it and learn from the constructive criticism.

But you'll understand that I value my time, my work and particularly my dignity too much to just keep playing gotcha games. I'd rather take some time off.

smustgrave’s picture

smustgrave commented

Per the policy please disclose per ticket

smustgrave’s picture

smustgrave commented

Hit submit before I finished

But disclosing is needed for those tickets.

It’s not meant to discourage and it’s good you review the submissions but it’s for the reviewers and committers who need to now look at it.

xjm’s picture

xjm
she/her
Primary language English
commented

@pancho, apologies for it feeling like a "gotcha" question. It's really not meant to be. It's just part of our contribution policy to document it, to help reviewers, ensure transparency, and be able to reference in the future. So please do document it each time when you are drafting code or text with the help of AI; this can really be something simple like:

MR created with Claude; I've reviewed the generated code.

Or:

Comment drafted with help of AI tools

Or whatnot.

Also please note that verbosity in general is a real difficulty, not just something folks are using to fingerprint your contributions. More words is more words for the maintainer to read and process. I recommend editing more by hand to reduce words in comments you draft, and adding skills to your LLM to make it write less verbose prose if such is available.

xjm’s picture

xjm
she/her
Primary language English
commented

Oh, and please do use the template header section when creating new issues also. Thanks!

smustgrave’s picture

smustgrave commented

And I do apologize if that’s how I came off. I review so many issues sometimes quick. So note to slow down to myself to not come off like that.

pancho’s picture

pancho
Location UTC+2 🇪🇺 EU
commented

<3

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented

Couple notes on this:

  • I'm all about removing layout shifts and flashes of unstyled content in Drupal. It makes the UI seem clunkier. Anything we can do to get rid of these is great, so thank you!
  • There's work to replace the jQueryUI version of the modals with HTMX and the <dialog> element in #2158943: Add a native dialog element using HTMX to deprecate the jQuery UI dialog. Just FYI
  • It seems to me we don't really need to be calculating all the heights and widths anymore. Back when most of this JS was written (likely D6 days), CSS didn't have the tools to do sizing. But now we have units like vw, vh, dvw, dvh, cqw, blah blah. We also have min(), max(), and clamp() functions built into CSS that can do much of this. Is it possible to use these instead of all the JS?