Modal dialog style update

Postponed as design is not final yet

Is this issue duplicate to #3023311: Modal dialog style update?

@lauriii, this is #3023311: Modal dialog style update

Oops, I was supposed to reference #3023312: Dialog style update

Just closed #3023312: Dialog style update as duplicate of this one.
Also shortening the issue summaty.

Re #7:

I think that it's totally fine to have a separate issue for Dialog/Modal since the off-canvas dialogs are totally different.

Check the attached screenshots.

Core is looking into replacing jQuery UI with another library. We might want to consider doing the same here, even if that's before core makes any moves on this.

Updating issue status & giving credit to Kishan's great design work.

I did some research on this and found some pretty exiting libraries for showing dialogs/modals.
Searching them, I focused on open source, wide adoption, accessibility, clean API ... just to mention the key factors.

vex

https://github.com/hubspot/vex
vex is a modern dialog library which is highly configurable, easily stylable, and gets out of the way.
You'll love vex because it's tiny (5.6kb minified and gzipped), has a clear and simple API, works on mobile devices, and can be customized to match your style in seconds.

• Drop-in replacement for alert, confirm, and prompt
• Easily configurable animations which are smooth as butter
• Lightweight with no external dependencies
• Looks and behaves great on mobile devices
• Open multiple dialogs at once and close them individually or all at once
• Built in CSS spinner for asynchronous dialogs (TODO)
• UMD support

Demo http://github.hubspot.com/vex/docs/welcome/
Doc http://github.hubspot.com/vex/

a11y-dialog

https://github.com/edenspiekermann/a11y-dialog
a11y-dialog is a lightweight (1.4Kb) yet flexible script to create accessible dialog windows.

• No dependencies
• Leveraging the native  element
• Closing dialog on overlay click and ESC
• Toggling aria-* attributes
• Trapping and restoring focus
• Firing events
• DOM and JS APIs
• Fast and tiny

Demo http://edenspiekermann.github.io/a11y-dialog/example/
Doc https://github.com/edenspiekermann/a11y-dialog

Micromodal

https://github.com/ghosh/Micromodal
Tiny, dependency-free javascript library for creating accessible modal dialogs. The aim of this library is to make modal dialogs accessible and easy to include in your project with minimum configuration. It's only ~1.8kb minified and gzipped - A tiny library for big change.
Following are some of the interactions handled by the library:-

• Closing modal on overlay click
• Closing modal on esc button press
• Toggling aria-hidden attribute on modal
• Trapping tab focus within the modal
• Maintaining focus position before and after toggling modal
• Focusing on the first focusable element within the modal
• A11y-enabled

Demo https://micromodal.now.sh/#introduction
Doc https://micromodal.now.sh/#configuration

Re. #10:

Core is looking into replacing jQuery UI with another library. We might want to consider doing the same here, even if that's before core makes any moves on this.

Please, let's NOT do that! Instead let's just style the core dialogs (whatever library that happens to use).

We are going to have to replace jQuery UI in core, because it's unmaintained now. That's going to involve a lot of accessibility review and manual regression testing. I know that themers can over-ride JS libraries if they choose, but I think it would be a bad idea for Claro to do that since it's intended to become the core admin theme.

Nitpick: the terminology here is wrong. It's not "dialogs and modals" - it's just dialogs. Dialogs can be modal or non-modal, but that describes their behaviour rather than their appearance. They both look the same.

Overlay refers to the transparent grey background behind the dialog, right? That's usually the only difference between modal and non-modal dialogs.

Re. #18:
Thanks for finding these @fhaeberle. I hadn't heard of Vex or micromodal before. These can all be considered in the core issue to replace the jQuery UI dialog. (Note that whatever library we end up using, it will have to support both modal and non-modal behaviours. I'm suspicious of the micromodal description.)

dialog test module added to Clarodist Tools!

I don't think we should work on this before core has chosen which direction they are going to go.

Let's unblock this given that the current dialog solution will be around for at least the Drupal 9 life cycle. So let's start with styling the jQuery UI dialog modals here.

We need a follow-up for non-modal dialogs such as settings tray.

Somehow, Views UI modals are forced to have lower z-index that the required (see Z-indexes in Drupal 8 doc).

And this bug exists even with Stark or Seven, and seems to be related to the View UI view edit form. (Other modals are file, e.g. the ones that the Editor module provides.)

I also noticed that after #3074267: Replace jQuery UI position() with PopperJS is added, closing an off-canvas-top dialog that has a customized height makes the page be scrolled down.

This is ready for the first review.
@ckrina, please check the states of the dialog close button!

I was able to fix the z-index issue mentioned in #27.

For development and testing this, I was using the Views UI dialogs and the Dialog Test (dialog) submodule of Clarodist Tools.

The needed follow-up: #3088531: Off-canvas dialog style update.

I think that the normal (non-offcanvas) dialog can be handled here.

Since we have answers for #3, I remove the NSU tag.

I didn't review this in detail but I recognized the close buttons hover styling and wanted to question this as it's not defined in the design spec.

How should the hover(/focus) of the close button look like?

At the moment we're using a thick white border and in the focused state additionally the green outline – is this intended?

I could not reproduce the green focus outline. Neither in current stable Chrome version on MacOS, nor on IE11.

However, the modal does not correctly respond on mobile devices. Tested on iPhone 6S with Safari and the modal is positioned wrongly. The mentioned lack of responsiveness is also easily checkable in Chrome dev tools.

Re #33:

The issue you highlighted is not a Claro-specific issue, this happens in Classy, Bartik and Seven as well. It is more related to #3068696: Tables overflow on mobile.

Fair enough. If you remove the tables from the page, then the modal is shown correctly. Let's handle the tables in a different context. I am marking that issue RTBC.

1. 

   Should we remove the focus effect from the modal?

2. +++ b/core/themes/claro/css/src/base/off-canvas.theme.css
   --- /dev/null
   +++ b/core/themes/claro/css/src/base/off-canvas.theme.pcss.css
   

   What is the expected state of the off-canvas as a result of this patch?

3. +++ b/core/themes/claro/css/src/base/off-canvas.theme.pcss.css
   @@ -0,0 +1,69 @@
   +.ui-dialog.ui-dialog-off-canvas {
   ...
   +  z-index: var(--jui-dialogoffcanvas-zindex);
   
   +++ b/core/themes/claro/css/src/base/variables.pcss.css
   @@ -152,6 +152,22 @@
   +  --jui-dialog-zindex: 1260;
   +  --jui-dialogoffcanvas-zindex: 501;
   
   +++ b/core/themes/claro/css/src/components/jquery.ui/theme.pcss.css
   @@ -16,6 +16,10 @@
   +.ui-dialog {
   +  z-index: var(--jui-dialog-zindex);
   
   @@ -332,9 +336,9 @@
    .ui-widget-overlay {
   +  z-index: calc(var(--jui-dialog-zindex) - 1);
   

   Checked that these are line with https://www.drupal.org/docs/8/theming/z-indexes-in-drupal-8 👍

4. +++ b/core/themes/claro/css/src/components/ajax-progress.module.pcss.css
   @@ -105,6 +107,10 @@
   +.ui-dialog .ajax-progress--throbber .ajax-progress__message {
   +  display: none;
   +}
   

   Where is this needed?

5. +++ b/core/themes/claro/css/src/components/dialog.pcss.css
   @@ -16,70 +25,111 @@
   +.ui-dialog .ajax-progress--throbber {
   +  z-index: calc(var(--jui-dialog-zindex) + 1);
   +}
   

   Where is this needed?

6. +++ b/core/themes/claro/css/src/components/dialog.pcss.css
   @@ -16,70 +25,111 @@
   +.ui-dialog .ui-widget-content.ui-dialog-content > :first-child,
   +.ui-dialog .ui-widget-content.ui-dialog-content form > :first-child {
   +  margin-top: 0;
   +}
   +
   +.ui-dialog .ui-widget-content.ui-dialog-content > :last-child,
   +.ui-dialog .ui-widget-content.ui-dialog-content form > :last-child {
   +  margin-bottom: 0;
    }
   

   This doesn't seem super reliable. This doesn't work with Views UI for example. I'm wondering if we should at least document our assumptions here?

7. +++ b/core/themes/claro/css/src/components/dialog.pcss.css
   @@ -16,70 +25,111 @@
   +.ui-dialog-buttonpane .ui-dialog-buttonset .button--primary {
   +  order: 1;
   +}
   

   This might have some accessibility implications because this changes the visual order of the elements.

Re-rolled #29.

Addressing #36.4, #36.5 and #366.

Re #36:

1. I do not have any preference. I applied the focus effect just because also Seven reveals it for dialogs. I think that we need a UX feedback here :)

   

2. Off-canvas dialogs are inheriting most of their styles from normal dialogs, so I decided to apply the same look&feel for them instead of restoring the original state (that those had before these patches).

   Without this override:
   

   With this override:
   

3. 😊
4. These should target .views-ui-dialog. Fixed.
5. I can't figure out why I thought I needed this. (Accidental leftover?..) Removed.
6. Refactored (simplified). Based on my understanding, these should have no effect on Views UI dialogs (otherwise they would look very strange).
7. I highly agree with you. I just followed the design specs, and I applied the simplest changes to met the AC.

   If I'd change the way Drupal.behaviors.dialog.prepareDialogButtons works (and make the primary buttons be the last in the DOM as well and not just visually), the primary button (that is the most important button) would be the last form action that user can reach by keyboard navigation (accessibility?).

   I recommend that we do not do this reorder at all, but as I mentioned above, it was not my decision.

Re-rolled #39.

Re #39:
1. It looks like for the feedback gotten on the #accessibility channel the focus might not be needed, so maybe it can be discussed on a follow-up how to address now it's handled by core.
2. Great call @huzooka! +1 on that direction so when we start that components we already know what's being inherited.
7. I think it's a UX+a11y conflict that we should address together and with proper discussion, so +1 to move it to a follow-up too.

Reported #3096805: Dialogs are focusable in Seven and Claro for addressing #36.1 (and #43.1).

I also removed the button reorder styles and added #3096793: Dialog button order in Claro as follow-up to #36.7 (#43.7).

Re. #32: focus on the close button in the dialog header.

How should the hover(/focus) of the close button look like? At the moment we're using a thick white border and in the focused state additionally the green outline – is this intended?

It would be good to use the green outline when the dialog header close button has focus. The intention behind the green outline was to use the same distinctive focus signifier everywhere, and the green colour isn't used for anything else. Seven has wildly inconsistent focus styles which makes it harder to follow focus around, and we're trying to improve on that. The focus green has sufficient contrast against the dark dialog title bar background.

Hover shouldn't use the green focus outline - hover isn't focus. The user needs to be confident about which one will be activated by a keypress. If hover uses the focus style it can be misleading; one control might have keyboard focus while another element has pointer hover.

Re. #36.1: when the dialog container has focus...

Should we remove the focus effect from the modal?

Leave it in place please. It's important to see what has focus.

The discussion on Slack got mixed up between:

1. The focus behaviour - what element should get focus, and when?
2. The visible styling of focus - when something has focus, what should it look like?

As a theme, Claro only needs to address the second part. It's permissible for a dialog container to have focus, and this should be visible when it does.

Some of our dialogs get focus when it would be better to focus an element inside them, but that depends on the dialog contents. It's application logic, not look-and-feel.

Re. button order:

Keep the DOM, tabbing, and visual reading order in sync. Anything else is confusing to sighted keyboard users, whether using assistive technology or not.

Using CSS flexbox/grid order to rearrange operable controls (or containers with operable controls inside) is a fast route to a WCAG level-A failure (Focus Order).

#39.7:

If I'd change the way Drupal.behaviors.dialog.prepareDialogButtons works (and make the primary buttons be the last in the DOM as well and not just visually), the primary button (that is the most important button) would be the last form action that user can reach by keyboard navigation (accessibility?).

I don't really think there's an issue with that, so long as the DOM, visual, and tabbing order are in harmony. Predictability of tabbing is much more important than the number of key presses needed. But can Drupal.behaviors.dialog.prepareDialogButtons produce a different button order per theme?

It would be good to get confirmation from @andrewmacpherson that this is valid, but it looks like non-primary submit buttons in the buttonset do not meet Success Criterion 1.4.11: Non-text Contrast: Active User Interface Components. which includes the following

For active controls such as buttons and form fields: any visual information provided that is necessary for a user to identify that a control is present and how to operate it must have a minimum 3:1 contrast ratio with the adjacent colors.

In this case, the contrast ratio between button and background is 1.34:1. Subjectively - on one display I tried it on it was quite difficult to distinguish button from background.

@bnjmnm Thank you! That seems like valid feedback that should be addressed on the design level. Moving back to needs work.

It seems like the work being done is based on the screenshot in the issue summary, as opposed to the the Figma link which goes to a comp labeled "work in progress". In many other Claro issues I've worked on, the Figma link is the canonical reference while it's understood the screenshot can potentially be outdated. If this isn't the case for this particular issue, perhaps that can be made more clear in the issue summary.

Thanks for bringing this up!

Does anybody knows what should ideally be the contrast the button background should have towards the region background?

It was already defined in the previous comment:

must have a minimum 3:1 contrast ratio with the adjacent colors

Thanks @bnjmnm!

@ckrina the success criterion 1.4.11 requires color contrast of 3:1 for controls.

It seems like this is a pre-existing problem which is just worse in modals. The button has a contrast of 1.47:1 against white background.

In reviewing the guildlines on 1.4.11, I don't actually think that the button background here needs to meet the 3:1 contrast requirements as long as the text contrast ratio is met.

Understanding Success Criterion 1.4.11: Non-text Contrast states "A button which has a distinguishing indicator such as position, text style, or context does not need a contrasting visual indicator to show that it is a button." The listed example has a 1.3:1 contrast between the button background and page background.

I also believe in what DyanneNova stated (according to the guidelines in WCAG 1.4.11):

This success criteria does not require that controls have a visual boundary indicating the hit area, but if the visual indicator of the control is the only way to identify the control, then that indicator must have sufficient contrast. If text (or an icon) within a button or placeholder text inside a text input is visible and there is no visual indication of the hit area then the Success Criterion is passed. If a button with text also has a colored border, since the border does not provide the only indication there is no contrast requirement beyond the text contrast (1.4.3 Contrast (Minimum)). Note that for people with cognitive disabilities it is recommended to delineate the boundary of controls to aid in the recognition of controls and therefore the completion of activities.

and

A button which has a distinguishing indicator such as position, text style, or context does not need a contrasting visual indicator to show that it is a button, although some users are likely to identify a button with an outline that meets contrast requirements more easily

So the current implementation should pass, but we might want to find another solution as an improvement for easier access after all. To not block this ticket after all I've created a new related ticket to explore improvements to the secondary button #3103261: Improve button default variation accessibility.

Awesome, thank you @DyanneNova and @saschaeggi! It would be nice to get confirmation from either @bnjmnm or one of the accessibility maintainers if this addresses the concerns raised in #48.

Updating the issue summary.

My recommendation would be to ensure that a button should be easily identified as a button for those with low vision and/or color blindness. Consider the first paragraph under Active User Interface Components...

For active controls any visual information provided that is necessary for a user to identify that a control is present and how to operate it must have a minimum 3:1 contrast ratio with the adjacent colors. Also, any visual information necessary to indicate state, such as whether a component is selected or focused must also ensure that the information used to identify the control in that state has a minimum 3:1 contrast ratio.

Rerolled patch attached

The followup issue for the secondary button contrast is fine by me, especially since it may require more discussion. I do think that it technically passes accessibility as the button text does communicate its purpose, but it's also not ideal for several reasons (among others, I can barely see the button on some displays) and we can dig into that in the followup.

I see some differences in the Figma designs and the patch.
Screenshots are from cd_tools modal, and confirmed the same happens with ckeditor's "add link". In some cases, the views modals better match Figma, which is documented.
Figma:

Patch:

• Figma's close button is thinner and focus outline is round
• Figmas buttons in buttonpane have 16px worth of top/bottom spacing. In the patch it is 30px as it combines the padding from .ui-dialog-buttonpane and the margins from .button (this is fine with views modals, however)
• This one may need additional input regarding intent - the Figma design shows the content in .ui-dialog-content with 24px of top margin/padding, while the patch currently has 32. However, the Figma comp does not display content in this area, it is just a placeholder box. It's possible that the placeholder box assumes additional padding/margin. Subjectively, 32px feels like a bit much, particularly on modals with less content.
• Figma buttonpane background color is #F3F4F9 (this is fine with views modals, however)
• Black titlebar

==================

Figma's close button is thinner and focus outline is round

✓ Fixed
==================

Figmas buttons in buttonpane have 16px worth of top/bottom spacing. In the patch it is 30px as it combines the padding from .ui-dialog-buttonpane and the margins from .button (this is fine with views modals, however)

❗I can't reproduce it in any dialog, on my side it is always 16px
==================

This one may need additional input regarding intent - the Figma design shows the content in .ui-dialog-content with 24px of top margin/padding, while the patch currently has 32. However, the Figma comp does not display content in this area, it is just a placeholder box. It's possible that the placeholder box assumes additional padding/margin. Subjectively, 32px feels like a bit much, particularly on modals with less content.

✓ Fixed
==================

Figma buttonpane background color is #F3F4F9 (this is fine with views modals, however)

❗On my side it has always epxected bg-color.
==================

Black titlebar

❗ On my side it is always black (#222330)
==================

I have used Drupal v9.1.x branch. I tested the following dialogs:
1. Here node/add/article with ckeditor field, "Add link" and "Add image" buttons
2. "Add" button from here /admin/structure/views/view/content
3. cd_tools module (https://github.com/zolhorvath/cd_tools)

The changes are looking good! Whatever was preventing styles from fully being applied on my local is now addressed, so several of the things I spotted in #59 were in fact specific to my local.

Here's a line-by-line:

1. It looks like the close button icon is not quite centered

   

   

2. +++ b/core/themes/claro/css/base/off-canvas.theme.pcss.css
   @@ -0,0 +1,69 @@
   +  background: transparent url(../../images/core/ffffff/pencil.svg) no-repeat 0 50%;
   

   This needs an /* LTR */

3. +++ b/core/themes/claro/css/base/off-canvas.theme.pcss.css
   @@ -0,0 +1,69 @@
   +  padding-right: calc(var(--space-m) + var(--space-s)); /* LTR */
   

   Remove LTR comment, this is in a RTL block

4. +++ b/core/themes/claro/css/base/off-canvas.theme.pcss.css
   @@ -0,0 +1,69 @@
   +  font-size: 14px;
   

   This could be replaced with --font-size-s, but also possible this is better scoped for the off-canvas issue.

5. +++ b/core/themes/claro/css/components/dialog.pcss.css
   @@ -1,88 +1,131 @@
   -  -webkit-font-smoothing: antialiased;
   

   The presentation is more consistent across browsers if this declaration isn't removed, and (in my opinion at least)the change happens to be an improvement.

6. +++ b/core/themes/claro/css/components/ajax-progress.module.css
   @@ -163,6 +168,10 @@
   +.views-ui-dialog .ajax-progress--throbber .ajax-progress__message {
   +  display: none;
   +}
   

   In core, the progress message is hidden in all dialogs, not just views.

   .ui-dialog .ajax-progress-throbber .throbber,
   .ui-dialog .ajax-progress-throbber .message {
     display: none;
   }

   This should be the case here unless that's part of the new design I overlooked.

7. +++ b/core/themes/claro/css/theme/views_ui.admin.theme.css
   @@ -790,15 +837,17 @@ td.group-title {
   +  right: 4rem; /* LTR */
   

   Does this need an LTR if it's not changed in the rtl equivalent?

Fixed all feebacks, except of:
1.

It looks like the close button icon is not quite centered

I can't reproduce it in Chrome, FF, Safari and IE. But property background-size: 12px 12px was added.

6.

In core, the progress message is hidden in all dialogs, not just views.

Markup for ajax-progress was overridden in Claro => .ajax-progress--throbber .ajax-progress__message selector is not same as . ajax-progress-throbber .message

7.

Does this need an LTR if it's not changed in the rtl equivalent?

rtl equivalent exist, please check again :)

As per the comments on #62 comment, I have verified the patch#64. All changes suggested are covered in patch#64.
Except:
1)

It looks like the close button icon is not quite centered

I can't even reproduce it from my side in Chrome, FF, Safari, and IE.

Not marking it as RTBC, further discussions are required.

Re #64.1 I investigated the close centering further and it does not seem to happen in IE, Safari or OSX Firefox. In Chrome and Windows Firefox, this occurs at specific zoom settings and may not be visible at the default.

OSX Chrome 110x

OSX Chrome 125x

Windows Firefox 150x

Windows Firefox 120x

Windows Chrome 125x

#64.6

Markup for ajax-progress was overridden in Claro => .ajax-progress--throbber .ajax-progress__message selector is not same as . ajax-progress-throbber .message

They seem equivalent to me. For example, this is the "Opening media library" throbber in Seven:

<div class="ajax-progress ajax-progress-throbber">
	<div class="throbber">
		&nbsp;
	</div>
	<div class="message">
		Opening media library.
	</div>
</div>

<div class="ajax-progress ajax-progress--throbber">
	<div class="ajax-progress__throbber">
		&nbsp;
	</div>
	<div class="ajax-progress__message">
		Opening media library.
	</div>
</div>

So targeting .ajax-progress-throbber .message in Seven is equivalent to targeting .ajax-progress--throbber .ajax-progress__message in Claro.
With this being the case, if .ui-dialog .ajax-progress-throbber .message is set to display: hidden; in Seven, the Claro equivalent should also be hidden unless the design specified otherwise.

Screenshot examples of this:
In Seven, the throbber message is hidden if it is inside .ui-dialog

In Claro, it is visible, which (unless there was a design change), it shouldn't be

We should be probably using this design for the throbber:

It doesn't seem like that supports the message. I don't think we have to solve that here. However, it might make sense to open a follow-up to discuss if we should add a variation to the floating throbber that would allow displaying the message.

According to the designs, the while border shouldn't be visible on the focus effect of the close button.

The current implementation:

The current design in the design system:

Thanks for such detailed review and feedback, gonna kill it

@kostyashupenko thank you for working on this 🙌

1. 
   The close icon is not in the center of the element on some resolutions on Chrome.

   For some reason, the position is calculated incorrectly on Chrome (only with even numbered viewport widths 🤷‍♂️). It seems like we could add background-origin: border-box declaration to change how the background is positioned which solves this inconsistency, at least for me.

2. 
   It seems like there are some regressions in the workspaces UI as a result of this.

3. +++ b/core/themes/claro/css/components/ajax-progress.module.pcss.css
   @@ -93,7 +94,8 @@
   +.ui-dialog .ajax-progress--throbber .ajax-progress__throbber {
   

   Is there some reason to have .ajax-progres--throbber in the selector? Wouldn't this be more consistent if this was just .ui-dialog .ajax-progress__throbber?

4. +++ b/core/themes/claro/css/components/ajax-progress.module.pcss.css
   @@ -105,6 +107,10 @@
   +.ui-dialog .ajax-progress--throbber .ajax-progress__message {
   +  display: none;
   +}
   

   Let's explain here that we're rendering throbbers in dialogs with the fullscreen variation where the message should be hidden.

5. +++ b/core/themes/claro/css/components/dialog.pcss.css
   @@ -1,88 +1,132 @@
   +  display: block;
   

   Why do we need to declare this element as block element?

6. +++ b/core/themes/claro/css/components/dialog.pcss.css
   @@ -107,6 +106,10 @@
   +.ui-dialog:not(.views-ui-dialog) > .ui-dialog-content form > :first-child {
   +  margin-top: 0;
   +}
   

   What is this needed for? Can we add comment explaining that and why this is excluded in Views UI?

7. +++ b/core/themes/claro/css/theme/views_ui.admin.theme.css
   @@ -790,15 +837,17 @@ td.group-title {
    .views-ui-dialog .views-progress-indicator {
   

   Where is this element visible?

1.

The close icon is not in the center of the element on some resolutions on Chrome.

I tried to manage it again, please test it. background-origin: border-box; will not help 100%. This is common scaling problem of any browser actually, if my fix will not help - idk how to fix it, really.

2.

It seems like there are some regressions in the workspaces UI as a result of this.

We may have a follow up for it.

3-6. fixed

7.

Where is this element visible?

Oh.. this is very interesting part. In few words - i don't know. Source code is here https://git.drupalcode.org/project/drupal/-/blob/9.1.x/core/modules/view..., but actually it comes from 2010th, from views module for Drupal 7, sources are here https://git.drupalcode.org/project/views/-/commit/3b93bcb992ef3aa2825815...

I tried to reproduce this indicator, but i couldn't, without hardcode in devtools. I'm not sure if this code is still actual. So some help is needed here to understand what is going on and how it can be reproduced.

For now - i propose to keep this code. But if this indicator can not be reproduced anymore - we need a follow-up to remove its implementations in views_ui module and all themes with .views-progress-indicator selector attached.

With hardcode in devtools visual styling of this indicator is the following:

#73.1: Interesting 🤔 I'm curious to know which browsers were broken with background-origin: border-box? I tested the solution in #73 and unfortunately, it's not working properly in Firefox:

#73.2: Hmm, I don't think we should move fixing this to a follow-up since it's a regression which prevents people from using workspaces and is caused by this patch. 😔

#71.7: @bnjmnm pointed out that the progress indicator is visible after adding multiple fields to a View at once.

This may open up some potential solution for the centering issue. It looks like the "X" is nicely centered in its container at different zoom levels. It looks like it's the position of the focus outline that varies.

Since the <button> receives the focus, but the icon is added to a <span> inside it - perhaps there's a way to position the span that is more tolerant of different zoom distances.

This patch

• Fixes centering of the close icon (at least according to my manual tests). There were several positioning rules in the styles leading up to the icon that weren't necessary to get the desired result. Removing them fixed the issue, I suspect due to the browser having less to process.
• Fixed the Workspaces regression. This was due to: (1) min-heights on the title div which aren't necessary as the dialog renderer will always add a title that takes care of this min-height. (2) line-height being assigned to .ui-dialog-title <span>. Moving that rule to the parent div results in the same styling of the span, but without forcing the element height in Workspaces when that is not desired. (3) Removed the first-child/last-child margins and replaced with top and bottom padding of the dialog content div to achieve the same benefits.
• Removed variables not needed after the Workspaces fixes
• Changed a variable name to match the convention of its siblings.
• Removed the added pencil SVG since it is a copy of core's, which is no longer needed. Opened the followup #3133823: Refactor references to images in Claro's images/core directory. (also aware that the close icon is inlined although we've agreed to not inline in .pcss.css. This will get refactored along with the other inlined backgrounds when the tool for inlining on build is added)

Hi, here is a problem with Media Modal.

Added same margin for count text

I think we should probably move the counter to left on Claro because we've moved the button to the right side of the modal. Ideally this would be done as part of #3062751: Media and media library but since this is a regression, it might make sense to fix it here.

Hi laurii, can you check now ? what about the margin ?

#80 won't work as it makes a change to media library that impacts all themes. I suspect that is why there's a failing test, too. This patch modifies the approach so only Claro is impacted. Media library's JS is changed to trigger a new event, but that won't disrupt other themes.

Tagging "needs followup" since it would work best to have the Media Library counter be abstracted into a theme function.

1. +++ b/core/modules/media_library/js/media_library.ui.es6.js
   @@ -388,6 +388,7 @@
   +          $(window).trigger('dialog:aftercreate__media-library');
   

   Is there any pre-existing event we could use for this? I think we should only add this as a new API only as the last resort.

2. +++ b/core/themes/claro/css/components/dialog.pcss.css
   @@ -1,88 +1,121 @@
   +.ui-dialog:not(.views-ui-dialog) > .ui-dialog-content {
   

   Why do we have to take Views UI dialog into account here?

3. The close button isn't rendered correctly on IE 11 high contrast:
   

Followup created and added some @todo items to this patch referencing it #3134526: Create theme function for selection counter

Re #83.1

Is there any pre-existing event we could use for this? I think we should only add this as a new API only as the last resort.

- not that I could find. This occurs inside the final event fired during dialog creation. I tried adding another listener to the same event and seeing what options there were to control the order, but the only way I'm aware of would require refactoring both listeners to not use anonymous function and would be uglier than either creating the theme function or trigger that edge-case event. There's a @todo next to the event trigger to remove it in the followup, at least

#83.2

Why do we have to take Views UI dialog into account here?

Without excluding views UI there's a regression so the top bar with the search elements are not flush with the dialog titlebar. I added a @todo referencing #3066006: Convert Views UI to new design system to remove the "not()" selector. It shouldn't be needed once the Views style update is complete.

#83.3 High contrast close button added:

Also moved the media library CSS rule out of dialog and into a Media Library UI specific file.

@lauriii pointed out that the new event being triggered in #84 may be difficult to remove when it's no longer needed as it would technically be a BC break. This increased our interest in pursuing #3134526: Create theme function for selection counter, and in that issue (and in Slack), Media Library maintainer @phenaproxima expressed some reasonable concerns about adding this ala carte as there are broader issues that this could complicate.

So instead of a custom event or theme function I went with a MutationObserver, which fully keeps the logic in Claro. MutationObserver is supported by all browsers Drupal supports, and the other use of MutationObserver happens to be in core. My biggest concern about using MutationObserver - a mysterious process running in the background at all times - isn't an issue here. It only exists if a Media Library dialog is opened and the selection counter is not yet present in that dialog. As soon as the selection counter is added, the observer will move it to the desired area of the buttonpane then stop observing.

The test in #85 failed due to core/modules/media_library/js/media_library.ui.es6.js not getting fully reverted, so I fixed that here.

My concern about the contrast in #48 was addressed by #54 and #55.

We can probably also remove the accessibility review tag based on #88 because the tag was added to address #48.

I like the MutationObserver approach over the previous approach that required adding a new event. We can probably get rid of that once #3134526: Create theme function for selection counter has been resolved.

One small problem I discovered was that on the high contrast mode the close button X is not center of the button. If this can be resolved easily, it would be nice to fix here, but otherwise we can also fix it in a follow-up.

The centering of the close button in high contrast mode is not an easy fix, I tried several approaches but neglected to mention when the patch was posted. I had some ideas on how to get around this, but it would require enough iteration that it would be best in its own issue.

Followup created #3137015: In High contrast mode, Dialog close button "icon" not fully centered.

1. I think we should keep the changes from impacting off-canvas as much as we can because that will be redesigned in #3088531: Off-canvas dialog style update.

   We should at least change the background color back to its original state, since the cold grey on the title doesn't work well with the warm grey used on the rest of the off-canvas.

   Before (from Seven):
   

   After (Claro with this patch):
   

2. +++ b/core/themes/claro/css/base/variables.pcss.css
   @@ -155,6 +155,21 @@
   +  --jui-dialog-zindex: 1260;
   +  --jui-dialog-offcanvas-zindex: 501;
   

   Nitpick 🧐: For consistency s/zindex/z-index

3. +++ b/core/themes/claro/css/components/dialog.pcss.css
   @@ -1,88 +1,122 @@
    .ui-dialog .ui-dialog-title {
   ...
   +  font-size: var(--font-size-h4);
   

   Should this element be a heading? Maybe this could be a follow-up but just wondering based on the styling 🤔

#92.1
Changed the background color. Tried out changing a few other styles to better resemble the default off-canvas styling and this is the only change that worked well

#92.2 Renamed the z-index

#92.3 created followup #3137839: Consider making dialog title a heading.

#93 addresses all of my feedback from #92. As part of #92 I did extensive cross browser compatibility testing on Chrome, Firefox, Safari (Mac + iOS), Chrome for Android, IE 11 and Edge. I think this is finally ready 🎉

Didn't look at the CSS.

On the JS minor nitpick, still RTBC for me. I like the mutation observer, that's a good one.

We can have a bit more specificity in selecting the dialog:

        .on('dialog:aftercreate', (event, dialog, $element, settings) => {

          //  ...
          const $buttonPane = $element.closest('.media-library-widget-modal').find('.ui-dialog-buttonpane');

That will make sure we're targeting this specific modal, not that nested modals should happen anyway.

Moving back to needs work for #95.

Updated the patch as #95 suggested.

Please make sure you check with eslint before submitting the patch, the code doesn't follow our coding standards. take the habbit or running yarn run lint:core-js-passing before you submit a core patch with javascript changes.

+++ b/core/themes/claro/js/media-library.ui.es6.js
@@ -0,0 +1,67 @@
+          const $buttonPane =$element.closest('.media-library-widget-modal')

missing a space after the "=", closest on the next line and the .find with the proper indentation.

Updated patch as per the changes mentioned in #98, please review.

Thanks for the update, if you run yarn run lint:core-js-passing you can see there is still an issue.

@codersukanta: you should not edit the generated file /core/themes/claro/js/media-library.ui.js manually, this is generated by the build step for JS, see: https://www.drupal.org/docs/frontend-developer-tools-for-drupal-core

Updating the patch to fix the JS part

Changes since #94 look good! Tested manually to confirm that everything still works as expected.

Some small spelling mistakes noticed by cspell. In core we have always used off-canvas so let's not introduce a new word here.

+1 to the changes in #104

+++ b/core/themes/claro/css/components/jquery.ui/theme.pcss.css
@@ -332,9 +336,9 @@
diff --git a/core/themes/claro/css/theme/ckeditor-frame.css b/core/themes/claro/css/components/media-library.ui.css

diff --git a/core/themes/claro/css/theme/ckeditor-frame.css b/core/themes/claro/css/components/media-library.ui.css
similarity index 92%

similarity index 92%
copy from core/themes/claro/css/theme/ckeditor-frame.css

copy from core/themes/claro/css/theme/ckeditor-frame.css
copy to core/themes/claro/css/components/media-library.ui.css

Here's why my patch file is smaller... it's because I have

[diff]
        renames = copies
        renameLimit = 7000

in my .gitconfig file.

Committed and pushed 74bad3ce90 to 9.1.x and d2cf243c49 to 9.0.x. Thanks!

Backported to 9.0.x as claro is experimental.

This issue took a wrong turn at #68. The two-colour focus outline IS intended. Filed a follow-up at #3172056: Restore two-colour focus indicator