Olivero: Modal dialog buttons not aligned (for example, in the "Leave preview" dialog)

Override the classes in
/core/assets/vendor/jquery.ui/themes/base/dialog.css
Added new file dialog.css.
Here is the fixed review the patch attached before after patch screen shot for the reference.

Couple of issues by looking at the code:

1) The CSS needs to be only loaded when needed. You should do this by extending an existing library
2) Why the jQuery UI copyight?
3) Indentation is incorrect
4) this will affect every jQuery UI. Is this intentional? If so, we need to make sure there are no regressions.
5) Indentation is incorrect. You'll also want to conform to the nesting style that we're using throughout Olivero.

Did all the changes as mentioned in #4.
There are no regressions checked with other theme also working properly as expected.
Review the patch attached before after patch screen shot for the reference.

What's the difference between the core CSS and the CSS in this patch? Why not extend the library instead of completely overriding the CSS?

Please review the patch.
Created a separate library and extended dialog using libraries-extend.

Please review the patch #11.
Created a separate library and extended dialog using libraries-extend.

Hi @komalkolekar,

Steps to test:
1) Download and install the Olivero themes inside the themes folder.
2) Enable and set as default and set 'Olivero' as the admin theme also.
3) Edit the node and click the Preview button.
4) Click the site title and check the dialog box.
5) There Preview and cancel button should be in an aligned position.

After patch:

Before patch:

@komalkolekar,
Please try to fix the linting errors. Please have a look at the below screenshot.

Updated linting file.

Linting errors:

@Ramya Balasubramanian Thank you for the review.
Please review the patch #14 fixing linting issues as mentioned in #13.

Hi @komalkolekar patch looking good, but am experiencing one issue by default button text font-weight:700 but on hover,focus,active it's changing to font-weight:normal(overriding by core/assets/vendor/jquery.ui/themes/base/theme.css) which is not expected behaviour we need to keep the font-weight:700 for all states.

current behaviuor:

Expected:

@Indrajith KB Thank you for the review.

by default button text font-weight:700 but on hover, focus, active it's changing to font-weight:normal(overriding by core/assets/vendor/jquery.ui/themes/base/theme.css) which is not expected behaviour we need to keep the font-weight:700 for all states.

#15 did the changes attached the screen short for the reference

hi @komalkolekar please see the #9 comment and check both css. Right now you just copied all the styles from core css (we extending the library so no need to write the styles again). just add your styles which helps to fix the issue. please update the patch.

@Indrajith KB Thank you for the review did the changes mentioned in #17.

refer the patch #20

HI @komalkolekar patch is looking good. Can you please try to follow the proper nesting style which we are using throughout Olivero (Comment #4 and 5th point from @mherchel). It will be more appreciable if you are updating accordingly.

@Indrajith KB Thank you for review update the patch with proper nesting style.

forget to run the css lint.
Review the patch #23.

HI @komalkolekar great work thanks for the patch.
Issue has found fixed, not adding the screenshots again.

am adding as RTBC +1

We can wait for the review from @mherchel

jQuery UI is on it's way out of core https://www.drupal.org/node/3067969 and gone in D9 and this theme is aiming for core inclution. Would not including code like this make it harder to get it approved and committed?

Just a reroll, but honestly i don't like the way about to manage inline buttons from this patch at all. Styles provided are not universal for all languages:

For example FR language

It can be any language actually and styles might be broken if language is not EN in terms of this patch.

I see 2 points how we can deal with it:
1. I'm not against if one button will be under another, i mean why it must be inline ?
2. In case if inline is really expected - then this task needs additional fixes or with data-dialog-options attribute with wider width property, or we can even rewrite width for all dialogs -> by default width is 300px

Or maybe we can totally rewrite dialog component from core with own design and custom styles ?

This is a relatively minor issue that most people won't encounter. I don't want to add technical debt by doing a complicated fix.

@kostyashupenko is correct that this should work across language. and that having the buttons wrap isn't too bad.

Instead of the fix above, maybe we can reduce the width/height/font-size of the buttons so they fit? This would be a global fix to the jQueryUI dialogs within the theme.

I like what @kostyashupenko said, not being against if one button is under another. Comparing to small screens, this also happens with other buttons if I narrow screen to 300px wide and the buttons width is bigger than parent.

I agree with the points that @Kostyashupenko has mentioned in #26, having a button under the other and not inline is not that bad and it should be a global fix to all the dialogs within the theme.

I'll take a look at this one OR @kostyashupenko since this is your suggestion, you can assign it to yourself
I'll probably start on it in the second half tomorrow, if you wish to pick this one up, please feel free to assign it to yourself :)

Patch details:

1. Extended the core's library for dialog.
2. Fixed the buttons to be inline.
3. Added few more styles to give ui-dialog box the Olivero touch.

Please review.

Before Patch:

After Patch:

Thanks!

Hi @hansa11

Great work on the dialog box, I like how you have matched it to the Olivero styles.

I have reviewed it and this applies well to me.

Looking into this.

Before Patch:

After Patch:

Please review.

Thanks!

This is looking good (with one change). I really like the idea of extending core/drupal.dialog, as there are a number of jQueryUI issues that are caused by our larger form controls.

The only change is that we need to set flex-wrap: wrap on .ui-dialog-buttonset, so that the text will wrap if the button text is longer (which can happen when the text is translated - see #26)

@mherchel: Thank you for the review :)

Added the wrap for long words, please review.

Thanks!

Looks great. Tested with long text (buttons wrap as expected), and verified CSS linting.

Sorry for the delay on my review. I was taking time off for 2 weeks ☀️

This change looks like a great improvement! I think this is at least worth a normal priority so changing that.

+++ b/core/themes/olivero/css/components/dialog.pcss.css
@@ -0,0 +1,83 @@
+    word-break: break-word;

According to https://developer.mozilla.org/en-US/docs/Web/CSS/word-break#Values, word-break: break-word is deprecated. Wondering if we could leave this out from here and come up with a solution to this in #3177475: Olivero: Ensure long words break properly when zoomed in to provide proper reflow.

Worked on #48.
Instead of word-break: break-wordusingoverflow-wrap: break-word; work as expected
Attached screenshot for reference.
Review the patch.

The above patch works fine.
Before patch

After patch

+++ b/core/themes/olivero/css/components/dialog.pcss.css
@@ -33,12 +33,13 @@
   & .ui-button {
+    display: inline-block;

Why do we need to change this to inline-block? Asking because I didn't find any relevant feedback and there's not explanation in #49.

1. word-wrap: break-word; - Not sufficient to get the desired output the word overflows out of the window.
2. To avoid the overflow - display: inline-block;
Attached the screenshot for reference one is without using display: inline-block; and one is with using.
Please correct me if I am wrong.

Hi,

#49 is resolving this bug for prove i shared attachment please review,

Thanks!

According to the CI, the CSS source file is not synced with the compiled CSS file.

Hello, the thing asked in previous comment is fixed - now both files synced. But can anyone please point me of why overflow-wrap: break-word; and display: inline-block; were used for .ui-button selector? On my side all works fine without these two properties. I did tests in Chrome/FF/Safari/IE11. Tests were done with short text and long text (like multiple lines for each button). Maybe i don't understand something, but i removed these two for now.

So this task needs re-tests

Please do not ask the testbot to try again until #3207086: [HEAD BROKEN] Consistent failure in MonthDatePluginTest is fixed.

#3207086: [HEAD BROKEN] Consistent failure in MonthDatePluginTest is fixed. I have not reviewed the code.

Based on #59, this still needs a review.

Patch #56, seems fine to me. Adding after-patch screenshot for reference.

Moving to RTBC +1.

Restoring status after HEAD was broken.

Since this impacts positioning of buttons in all dialogs, I think it would be useful to get a review from the Olivero maintainers.

i think the patch #56 is applicable for drupal 9.3.x-dev.
i tested the patch with drupal 9.3.x-dev. i got the solution correctly.Adding screenshot for the reference

Verified and tested patch#56 on Olivero 9.3.0-dev version with 9.3.x-dev

Patch applied successfully and result is Acceptable.

Testing Steps:

1. Install Drupal 9.3.x-dev version.
2. Go to Appearance -> Set Olivero theme as admin and default theme
3. Edit the node and click the Preview button.
4. Click the site title and check the dialog popup.
5. Preview and cancel button should be in an aligned position.
6. Now apply the patch and clear the cache.
7. Verify the popup dialog box button alignment for Cancel & Preview

Testing Output:

After applying the patch the button alignment for the Cancel & preview button started showing properly in the popup dialog box

#70 Please review the patch

Haven't reviewed this yet, but I want to add a note that jQueryUI is being deprecated.

Verified and tested patch #71.
The patch does not apply successfully.

Please refer attached screenshots.
Moving to Needs work.

re-roll patch #71
please review the patch.

Verified and tested patch #74.
The patch does not apply successfully.

Please refer attached screenshots.
Moving to Needs work.

Normal priority bug under the normal issue priority definition:

Bugs for site visitors that do not interfere with site use, for example, visual layout issues.

(If it were actually only related to jQuery UI it could be demoted to minor, or it can be re-demoted if it turns out that the Dialog replacement, once added, does not have the same issue. But in this case it's about the modal.)

I closed #3213125: Olivero: Save button and discard changes button are not aligned to each other in Discard Changes? the dialog box for quick-edit. as a duplicate of this issue.

The current patches seem to add a good amount of CSS for something that seems like it can be addressed with only a few lines. All the additional CSS is of particular concern since it would impact all dialogs, not just the node preview one.

Two options come to mind:

• The issue as reported could be addressed if the dialog was 30px wider. If you can find a way to set a min-width on just this dialog (not styling all dialogs, as the current patches do), that would take care of it. It seems reasonable to make the dialog default width a bit wider in Olivero since text and spacing is larger than its predecessor.
• Something that would impact all dialogs, but may be nondisruptive enough to be approved by Olivero's designers is reducing button size in some way within dialog buttonpanes:

  For example, this single rule addresses the reported issue without altering the button appearance much.

  .ui-dialog-buttonpane .button {
  padding-left: 1.3rem;
  padding-right: 1.3rem;
  }

Also worth considering that the button lengths are going to be different depending on the language the site is in. Whatever solution works for Engligh should probably grant some additional spaces for potentially longer labels on non-English sites. (though also important to keep in mind that buttons apoearing on different lines is also not a major concern)

Applied patch #74 working fine
After patch the button alignment is good and working fine for ref sharing screenshot

patch #74 working fine sharing screenshot --

Removing credit for #81, a second set of screenshots is not necessary immediately after ones were provided, and the colors are different from Olivero so it's not clear if Olivero is even the theme represented.

The next best steps are addressing #79 -- the current fix is very elaborate and difficut to truly review, it adds a bunch of CSS not specific to the reported issue. A small amount of CSS that targets this specific issue is preferable.

Per bnjmnm's comments in #79 and #82, here is a patch that targets just this dialog in this theme, sets the min-width to 400px and aligns the buttons on the left.

This is 100px wider than the default, granting additional space for longer button labels in other languages.

Mobile remains unchanged.

Fixed CS issue.

@cindytwilliams It's useful to provide an interdiff when updating a patch: https://www.drupal.org/docs/develop/git/using-git-to-contribute-to-drupa...

Checked and the patch applies cleanly to 9.3, 9.4, and 10. Needs testing for all versions so tagging.

I have verified with the version 9.4 and It looks good to me.
Tested patch #84

Testing steps :

1. Install Drupal 9.4.x-dev version.
2. Go to Appearance -> Set Olivero theme as admin and default theme
3. Edit the node and click the Preview button.
4. Click the site title/logo and check the dialog popup.
5. Preview and cancel button should be in an aligned position.
6. Now apply the patch and clear the cache.
Verify the popup dialog box button alignment for Cancel & Preview

Testing Output:

After applying the patch the button alignment for the Cancel & preview button showing properly in the popup dialog box

@Satyajit1990 Thanks for testing. Would you test on versions 9.3 and 10 as well? The same patch applies cleanly to all versions. You can use simplytest.me for testing the other versions as, right now, DrupalPod only is defaulting the Drupal version so it's the same as what's on the issue (9.4 in this case). Thanks.

Hi Kristen,
Verified with the version 9.3 by using simplytest.me
And It works for me after applying patch #84

Screenshot for reference :

I am unable to verify with the version 10 as getting syntax error after applying the patch

Thanks for testing on Drupal 9.3.

1. Testing on Drupal 10 is still needed.

2. This needs code review from a frontend developer. The !important makes me wonder if this is a good approach.

I have found out from @catch that, since the patch applies cleanly to 9.3, 9.4, and 10 and it was already tested on 9.4 then we don't have to test on 10.

At this point, it just needs a review from the Olivero team so I'll mark RTBC since:

1. Patch applies cleanly to 9.3, 9.4, and 10.

2. Patch appears to only address issue in the issue summary.

3. Title and issue summary are fairly clear (though it's not using the template).

4. Tests pass.

5. Manual testing passes on 9.4.

6. Not sure specific test could be added for this.

Still needs a +1 from @mherchel

+++ b/core/themes/olivero/css/components/ui-dialog.pcss.css
@@ -9,3 +12,12 @@
+    float: inherit !important;

It would be nice if we could use css layers.

Putting jquery UI css into a lower layer would avoid the need for !important here I assume

1. +++ b/core/themes/olivero/css/components/ui-dialog.css
   @@ -4,13 +4,25 @@
   +  html[data-once="node-preview"] .ui-dialog {
   

   The issue summary used the node preview dialog as an example of the issue, but the solution should not be targeted at just node preview dialogs. this is something that could happen with any dialog with multiple buttons in the pane where they occupy > 300px (the jQuery ui dialog default width)

2. +++ b/core/themes/olivero/css/components/ui-dialog.css
   @@ -4,13 +4,25 @@
   +    min-width: 25rem;
   

   Providing a specific width so this specific dialog is wider doesn't address the underlying issue either. An explicit width assumes the buttons will not be any longer when translated into other languages. We need a solution where the modal width is dynamically calculated based on it's content as it initializes

3. Notice that the dialog settings are to already set automatically determine width. This likely means that the width is calculated to the default of 300 before the buttonpane is even present
   
   This could be addressed by listening to the dialog:aftercreate event and setting the dialog minimum width to accommodate the combined width of the button pane buttons (+ their margins)

Thanks @bnjmnm for the suggestion, Ive tried to use dialog:aftercreate event to calculate the min-width for the UI dialog. I have made some css changes too to get rid of specific min-width

Thanks for the update. Tagging for testing with new patch.

@ameymudras,
Tested the bug with version 10.1 and tried the patch 3149863-95.patch that's working fine for me.
attached the screenshot, good for RTBC.

1. +++ b/core/themes/olivero/css/components/ui-dialog.pcss.css
   @@ -9,3 +12,13 @@
    }
   +
   

   The problem with the width calculation is that .ui-dialog-buttonset is set to float: right. Fortuately, fewer floats are a good thing.

   Give .ui-dialog-buttonset a float: none and a margin-left: auto (and reverse it for RTL)

   .ui-dialog-buttonset {
     display: flex;
     float: none;
     margin-left: auto; // this will need an RTL equivalent
   }
   

   Then make .ui-dialog-buttonpane display:flex as well. Doing this makes it so the margin-left: auto in .ui-dialog-buttonset positions it the same way float: right BUT! in a manner that results in a properly calculated dialog width.

   .ui-dialog-buttonpane {
     display: flex;
   }
   

2. +++ b/core/themes/olivero/css/components/ui-dialog.pcss.css
   @@ -9,3 +12,13 @@
   +@media (--sm) {
   +  .ui-dialog-buttonset {
   +    display: flex;
   +  }
   +}
   

   Remove the media query here so it is always flex and the buttons stack when the viewport is narrower than their total width

3. +++ b/core/themes/olivero/js/ui-dialog.es6.js
   @@ -0,0 +1,35 @@
   +  Drupal.behaviors.UiDialogUpdateSize = {
   +    attach() {
   +      $(window).on(
   +        'dialog:aftercreate',
   +        (event, dialog, $element, settings) => {
   +          const $buttonPane = $('.ui-dialog .ui-dialog-buttonpane');
   +          let minWidth =
   +            parseFloat($buttonPane.css('paddingRight')) +
   +            parseFloat($buttonPane.css('paddingLeft')) +
   +            20;
   +          $buttonPane.find('.ui-button').each(function () {
   +            minWidth +=
   +              $(this).outerWidth() +
   +              parseFloat($(this).css('marginRight')) +
   +              parseFloat($(this).css('marginLeft'));
   +          });
   +          $('.ui-dialog').css('min-width', minWidth);
   +        },
   +      );
   +    },
   +  };
   

   On mobile devices, this approach can result in a dialog wider than the viewport, which shouldn't happen. Fortunately, I think this JavaScript can be removed entirely. I think it's a clever solution, but it also looks like quite a bit of logic for a fairly small UI change which should be avoided when possible if only to have less code to maintain long term. Fortunately, I believe a CSS only solution is possible.

Also, the issue summary has things like screenshots of CSS lint and the simpletest home screen that should be cleaned up. Those don't belong in an issue summary as their absence would not result in any less understanding of the issue reported. The issue summary does not need to mirror the comments, it only needs to be updated with broad information regarding diagnosis and outcome.

Cleaned up Issue summary.

Tried to address comment #99 feedback. I have added patch for same. Please review

Tried to fix CCF for #102.
Please review.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Fixed CCF, attached interdiff for same.
Also we're not using any variables in the file, so I have removed the import of variable file as well.

Verified the patch #105 and tested it on Drupal version 10.1.x with Olivero theme. The patch works fine and I have added the before and after screenshots for reference.

#101 updated issue summary so removing that tag.

+1 For me as this does solve the issue at hand. Not moving as still need review from olivero maintainers.

This looks good, and I think would probably be fine, but the fact that we're targeting all ui-dialogs in Olivero is reason enough to make sure this is bulletproof. As much as the design looks better with the buttons in alignment, it's more important that we don't introduce regressions in the UI.

.ui-dialog {
  & .ui-button {

    &:last-child {
      margin-inline-end: 0;
    }
  }
}

This is targeting all buttons that are the last child in their wrapper, including the close button. This doesn't appear to change anything as that button doesn't have a margin, but since we're already digging into the buttonpane and buttonset wrappers later, we might as well limit the scope of this rule.

.ui-dialog {

  & .ui-dialog-buttonpane {
    padding-inline-start: 0.2em;
    padding-inline-end: 0.2em;

    & .ui-dialog-buttonset {
      display: flex;
      float: none;
    }
  }
}

Since we're setting the buttonset to display:flex;, I think we ought to consider a few things:

1. If we add flex-wrap: wrap; to the buttonset element, that will allow buttons to wrap to the second line if they need it, before wrapping extremely long text and overflowing the button's boundaries.

   
   

2. If we space the buttons in the buttonpane using the gap property, we don't have to worry about margins on any hypothetical 3rd buttons.

   

Hello @andy-blum,

Added flex-wrap: wrap in ui-dialog-buttonset for global implementation.

Please review.

I know it's sandwiched between the images, but still needs point #2 from my earlier review:

If we space the buttons in the buttonpane using the gap property, we don't have to worry about margins on any hypothetical 3rd buttons.

added patch and fixed CCF #109

Hello @andy-blum,

Added gap property for button layout to handle global implementation.

Please review.

This looks good to me. Thanks @pradipmodh13

Seems to be random failure.

Committed 1d97f05 and pushed to 10.1.x. Thanks!

It looks like manual testing was done in #144. I am removing the tag.