Problem

The current settings tray dialog CSS is fairly messy, with resets scattered around the codebase. It's very hard to style elements within the dialog, and unintentional styles also tend to leak in (see overrides within Claro and Olivero)

See

Solution(s)

Proper resets

We can do proper resets by using a combination of all: revert, :where(), and selector arguments of :not(). For example the following modern CSS can replace several hundred lines of code, and be more resilient at the same time.


#drupal-off-canvas *:where(:not(svg, svg *, .ck-reset *, [data-drupal-ck-style-fence] *)) {
  all: revert;
  box-sizing: border-box;
  -webkit-font-smoothing: antialiased;
  &:after,
  &:before {
    all: revert;
    box-sizing: border-box;
  }
}

Make it theme-able

Right now, it’s a real pain if we want to change the look and feel of the setting tray. Modern CSS can let us fix that. 

#drupal-off-canvas-wrapper {
  --off-canvas-background-color: #444;
  --off-canvas-text-color: #ddd;
  --off-canvas-button-background-color: blue;
  --off-canvas-button-text-color: white;
  --off-canvas-text-size: 14px;
  --off-canvas-spacing-unit: 8px;
  --off-canvas-font-family: "Lucida Grande", "Lucida Sans Unicode", sans-serif;
}

Benefits

There are tons of benefits of refactoring

  • Less code (we may be able to delete the CK5 “fence”)
  • More resilient code (if a new element is introduced (eg dialog), we don’t have to add it)
  • Easily theme-able (admin themes and distributions will likely take advantage of this)
  • Easier to understand, which will lead to more innovation

Testing instructions

Testing for this isn’t as terrible as it seems. The patch is large (over 200k) and has over 50 commits. Looking at the diffs will be difficult because of the styles moving around.

Tugboat link: https://3291797-off-canvas-6-ggpezyyiyfqyuna635hb6gpahwycstwb.tugboatqa.....
Ping me in Drupal Slack (or here) for admin credentials to test.

To test:

  1. Create a custom block with as many different types of form elements as you can (tabledrag, autocomplete, select, etc).
  2. Enable a content type to use Layout Builder and enable customization of each node.
  3. Create a node of that content type and go to the layout tab
  4. Add a your custom block. Try this out in different browsers. Try it out in different core themes.
  5. Verify lists of items look good. Note that link colors are not accessible. This will be a followup issue.
  6. To test dropbuttons, enable the core settings tray module, and then use the settings tray to re-order the primary menu
  7. Test the workspace module (which uses a top horizontal off-canvas dialog) by enabling the Workspace module creating several workspaces, and testing the UI

Draft release note

The off-canvas dialog’s CSS has been completely refactored in Drupal 10.0.0. This means that if your module or theme previously implemented custom CSS for the off-canvas dialog, it may need to be re-implemented (or removed if it was solely bug fixes).

CommentFileSizeAuthor
#44 3291797-44-d9.5.9-off-canvas-reset.patch228.25 KBxaqrox
#41 3291797-d9.5.x-off-canvas-reset.patch228.24 KBmherchel
#23 focus-trap.png76.83 KBandy-blum
#23 workspaces-mobile.png98.02 KBandy-blum
#23 workspaces-desktop.png318.43 KBandy-blum
#23 offcanvas-autocomplete.png32.16 KBandy-blum
#15 focus-loss.png11.74 KBandy-blum
#15 tableweight.png14.62 KBandy-blum
#15 resized.png68.05 KBandy-blum
#15 menu.png152.93 KBandy-blum
#15 autocomplete.png47.69 KBandy-blum
#15 tabledrag.png35.53 KBandy-blum
#15 overall.png203.71 KBandy-blum

Issue fork drupal-3291797

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
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mherchel created an issue. See original summary.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
CreditAttribution: mherchel commented
Issue summary: View changes
mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
CreditAttribution: mherchel commented
Issue summary: View changes
mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
CreditAttribution: mherchel commented
Issue summary: View changes

mherchel opened merge request !2421

AaronMcHale’s picture

AaronMcHale
Primary language English
Location Edinburgh, Scotland
CreditAttribution: AaronMcHale commented

Here's a thought, are we at a point (in terms of browser compatibility) where we can start to use some of the more interesting approaches, like Web Components and the Shadow DOM. Maybe it's not an appropriate use case though, but perhaps worth considering non-the-less.

mherchel’s picture

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

Here's a thought, are we at a point (in terms of browser compatibility) where we can start to use some of the more interesting approaches, like Web Components and the Shadow DOM. Maybe it's not an appropriate use case though, but perhaps worth considering non-the-less.

I would say now's the time!

That being said, refactoring settings tray into web components is significantly more difficult undertaking than doing the CSS.

I'd love for us to use web components (my vote is to start with the dropbutton component). But that'd be another issue.

AaronMcHale’s picture

AaronMcHale
Primary language English
Location Edinburgh, Scotland
CreditAttribution: AaronMcHale commented

I would say now's the time!

That being said, refactoring settings tray into web components is significantly more difficult undertaking than doing the CSS.

I'd love for us to use web components (my vote is to start with the dropbutton component). But that'd be another issue.

Yeah that makes sense, this issue probably is better focused on just the CSS then :)

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
CreditAttribution: mherchel commented
Status: Active » Needs review

This is at a point were I get get some initial feedback.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
CreditAttribution: mherchel commented
Issue summary: View changes

Starting testing instructions

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
CreditAttribution: mherchel commented
Issue summary: View changes

Updating summary

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
CreditAttribution: mherchel commented
Issue summary: View changes

Updating testing instructions

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
CreditAttribution: mherchel commented
Issue summary: View changes
mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
CreditAttribution: mherchel commented
Issue summary: View changes

Adding tugboat link (https://3291797-off-canvas-5-jbtiljbitslsos1dwwwzw64y8c7qnzfh.tugboatqa....)

andy-blum’s picture

andy-blum
he/him/his
Primary language English
Location Ohio, USA
CreditAttribution: andy-blum at Lullabot commented
Status: Needs review » Reviewed & tested by the community
FileSize
overall.png203.71 KB
tabledrag.png35.53 KB
autocomplete.png47.69 KB
menu.png152.93 KB
resized.png68.05 KB
tableweight.png14.62 KB
focus-loss.png11.74 KB

General impression

This is a massive upgrade over what is presently in core which is - at best - barely usable.

I am of the opinion that this ticket is so much better than what's currently in core that I think this should go in as-is and we can open up followup tickets to continue improving. The unpatched version of this component has literally zero focus indicators, and some components aren't even functional. Frankly, I have no idea how any of this was approved and added into core given our commitment to quality accessible user interfaces.

Side-by-side comparisons

The styles present a generally more polished appearance within the offcanvas component. Some field widgets are immediately visibly better.

overall side-by-side comparison of offcanvas. autocomplete and tabledrag widgets already visibly better

Interacting with the tabledrag widget in the unpatched version is broken, items cannot be reordered. This is fixed in the patched version.

The autocomplete widget is functional in both versions, but shows drastic improvements in the patched version. Previously, only hovering the text of the autocomplete results showed any style changes. Now, hovering over any portion of the menu highlights the item in that portion.

This widget still needs work for keyboard accessibility - while it's functional, there's no indication that items are being selected as keyboard focus is moved up and down the list.

Altering the menu in the offcanvas component also shows dramatic improvement. Indentation levels in the tabledrag are likely too small, but at the very least it works and the buttons can be read.

Still needs improved

The offcanvas is resizable, when it is resized, the contents are too wide to display, causing some x-axis overflow. Additionally, navigating within the offcanvas or closing/re-opening the offcanvas always resets the component to the default width. It would be nice to store the preferred width in sessionStorage and maintain a consistent appearance at the author's preferred width.

When the tabledrag widget is asked to show weights, the select element does not match the rest of the select boxes in the theme.

When using keyboard navigation to go through the form, adding items to the reorderable unlimited-cardinality field drops focus to the top of the offcanvas menu, forcing authors to tab all the way back to the appropriate field

mherchel’s picture

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

@andy-blum thanks for the thorough review! I agree that it's a massive upgrade.

This [autocomplete] widget still needs work for keyboard accessibility - while it's functional, there's no indication that items are being selected as keyboard focus is moved up and down the list.

This is fixed with the latest commit. Good catch!

It would be nice to store the preferred width in sessionStorage and maintain a consistent appearance at the author's preferred width.

Agree. Thats out of the scope of this issue, though.

When the tabledrag widget is asked to show weights, the select element does not match the rest of the select boxes in the theme.

This is intentional to have it take up as little horizontal space as necessary.

When using keyboard navigation to go through the form, adding items to the reorderable unlimited-cardinality field drops focus to the top of the offcanvas menu, forcing authors to tab all the way back to the appropriate field

This sounds like a core tabledrag issue.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs release manager review

We need to also decide what to do about Stable and Stable 9 when it comes to these changes. The current CSS is pretty broken as pointed by #15. There are other serious accessibility and UX issues that I was able find by just doing couple minutes of testing. For example, text in details elements is completely illegible.

I don't think we should continue to ship the broken CSS in Stable and Stable 9. In fact, what I think we should do is remove off-canvas related CSS from Stable and Stable 9 and mark these CSS files internal. We would do this only in Drupal 10 because the refactoring can only happen in Drupal 10 since it's done so that it won't be compatible with IE 11.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented

The review in #15 is essential for understanding this issue; thanks so much @andy-blum!

We already have a policy that accessibility overrides theme stability, so if CSS or markup changes are required to fix accessibility bugs, the change can be backported to the production branch instead of being minor-only. I think it might make sense to apply that same thinking to the restrictions on stable base themes as well.

We should try to notify developers somehow since if this is committed to 10.0.x it will violate the continuous upgrade path policy. Not sure yet what the best thing to do is, but just wanted to document that we have some precedent for accessibility fixes being allowed when theme changes normally wouldn't be.

Also, the Starterkit base theme should definitely be fixed for this.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Priority: Normal » Critical

I think this is critical, especially with tabledrag being unusable and so forth.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented

This will be fixed in the Starterkit theme so long as this is fixed in Stable 9 because it's not overriding the off-canvas styles.

We should probably add warning to upgrade_status for any CSS that includes #drupal-off-canvas to give developers a heads up that they should at least test the CSS with Drupal 10 to make sure it's still compatible.

I think we should simply remove the CSS from Stable and Stable 9 and mark it as internal since it's highly specific and we shouldn't recommend overriding it. Ideally this would live in the admin theme but we have a pre-existing issue for that: #2195695: Admin UIs on the front-end are difficult to theme.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
CreditAttribution: mherchel commented
Issue summary: View changes
Status: Needs work » Needs review

I did a significant amount of work refactoring the Workspaces CSS. The old CSS was built on top of the old rickety off-canvas CSS, and needed work.

All feedback is handled, and this is ready for re-review.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
CreditAttribution: mherchel commented
Issue summary: View changes

Updating Tugboat link in summary to https://3291797-off-canvas-6-ggpezyyiyfqyuna635hb6gpahwycstwb.tugboatqa....

andy-blum’s picture

andy-blum
he/him/his
Primary language English
Location Ohio, USA
CreditAttribution: andy-blum at Lullabot commented
FileSize
offcanvas-autocomplete.png32.16 KB
workspaces-desktop.png318.43 KB
workspaces-mobile.png98.02 KB
focus-trap.png76.83 KB

Autocomplete

The autocomplete widget now has good :focus and :hover indicators

Workspaces

I'm not sure what workspaces looked like before this patch, but they look good with it. The offcanvas dialog opens, traps focus, has clear focus/hover states, and resizes well across device widths

 

Focus trapping

This is beyond the scope of this particular issue, but the offcanvas dialogs have some brittle focus trapping. I'm not 100% sure how it works, but I think it's set up to listen for focusout events on the first and last focusable elements. The issue with this strategy is evident in the layout builders "add a new block" dialog, as the block types are collapsible, allowing use to hide elements from view and tabindex. If we collapse the final category and tab through, focus wrongly leaves the dialog box and moves out to the viewport UI.

In the screenshot below:

  • Tabbing forward past the green arrow item correctly wraps focus to the start of the dialog
  • Closing the red arrow item and then tabbing forward moves focus out of the dialog and into the viewport UI

Overall Conclusion

I think this issue is largely ready to be merged and to have child issues opened for the out-of-scope items mentioned in this issue and in #15. Moving to RTBC

andy-blum’s picture

andy-blum
he/him/his
Primary language English
Location Ohio, USA
CreditAttribution: andy-blum at Lullabot commented
Status: Needs review » Reviewed & tested by the community
mherchel’s picture

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

Followup for the bug that @andy-blum discovered in #23: #3302103: Settings tray focus trap can inadvertently allow escaping of focus

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs release note, +Needs change record

Discussed with @mherchel and @bnjmnm to come up with a proposal on what to do about Stable and Stable 9. We thought that how significant of an improvement this is for off-canvas, we'd like to ship this fix for as many sites as possible. This includes sites using Stable and Stable 9. This impacts themes that are explicitly supporting off-canvas. That same group is the one of the beneficiaries of this benefit so we feel that for them this should be a net positive change regardless of the BC break.

To ensure that people are aware of this change, this issue needs a change record and a release note. The change record should include instructions for how a theme or module should support both Drupal 9 and Drupal 10. We should also add a warning to Upgrade Status for any modules that include #drupal-off-canvas in CSS.

mherchel’s picture

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

Another note with @bnjmnm and @lauriii is that we need to add a .off-canvas-revert CSS class onto the #off-canvas element. This will enable modules that support both Drupal 9 and Drupal 10 to use the #off-canvas:not(.off-canvas-revert) selector when shipping their D9 specific off-canvas styles. That selector is compatible with IE11.

mherchel’s picture

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

@lauriii also asked me to mark the off-canvas CSS as @internal, which is done in the last commit.

The final item left to do is add a .off-canvas-revert onto the #drupal-off-canvas element. I can't figure out how to do this (outside of using JavaScript).

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
CreditAttribution: mherchel commented
Status: Needs work » Needs review

Was able to add the .off-canvas-revert #drupal-off-canvas element. This is ready for re-re-re-re-review.

andy-blum’s picture

andy-blum
he/him/his
Primary language English
Location Ohio, USA
CreditAttribution: andy-blum at Lullabot commented
Status: Needs review » Reviewed & tested by the community

code changes since my last review look good. RTBC pending release notes/change record.

mherchel’s picture

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

Started working on a change record at https://docs.google.com/document/d/1Ls23F_iBfpMu2yEXX9dCVsgEMEL48Rod_t7G...

Thoughts/edits appreciated!

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Issue tags: -Needs release manager review

I think #3291797-26: Refactor Drupal 10 settings tray / off-canvas to use modern CSS is a good justification here, and we're on our way towards deprecating stable/stable9 anyway, so removing the RM review tag.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Status: Reviewed & tested by the community » Needs work

For the few minor comments on the MR

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
CreditAttribution: mherchel commented
Status: Needs work » Reviewed & tested by the community

Setting this back to RTBC per @lauriii (since these changes were extremely minor).

mherchel’s picture

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

Opened followup #3306385: Create new site-wide visual styles for off-canvas dialog

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Issue summary: View changes
Issue tags: -Needs release note, -Needs change record

Filed #3306543: Add warning for custom off-canvas CSS and added a draft release note.

  • lauriii committed c2af575 on 10.1.x 
    Issue #3291797 by mherchel, andy-blum, lauriii, xjm, catch:  Refactor...

  • lauriii committed 983562f on 10.0.x 
    Issue #3291797 by mherchel, andy-blum, lauriii, xjm, catch:  Refactor...
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Status: Reviewed & tested by the community » Fixed
Issue tags: +10.0.0 release notes

Did some manual testing on forced colors to confirm there aren't regressions to forced colors users.

Committed c2af575 and pushed to 10.1.x. Also cherry-picked to 10.0.x. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
CreditAttribution: mherchel commented
FileSize
3291797-d9.5.x-off-canvas-reset.patch228.24 KB

Attached is a back-port patch for 9.5.x if anyone wants to use this.

**This is not IE11 compatible**

This contains

  • New reset using #off-canvas-wrapper.
  • Removes redundant fixes from Claro and Olivero
  • Makes both Workspaces and Layout Builder modules compatible with this.
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

Looks like this has potentially caused a regression: #3341737: CKEditor 5 content squashed in off-canvas sidebar.

acbramley’s picture

acbramley CreditAttribution: acbramley at PreviousNext commented

Re #42 it's caused by the removal of the blockSelectors and subsequent CSS.

https://git.drupalcode.org/project/drupal/-/commit/983562f41496eed45777a...

xaqrox’s picture

xaqrox
he/him
Primary language English
Location Washington, D.C.
CreditAttribution: xaqrox commented
FileSize
3291797-44-d9.5.9-off-canvas-reset.patch228.25 KB

reroll of #41 for 9.5.9

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented

Follow-up to fix a bug that was introduced here: #3361839: Accidental use of CSS nesting in misc/dialog/off-canvas/css/details.css

Anybody’s picture

Anybody
Primary language German
Location Porta Westfalica
CreditAttribution: Anybody as a volunteer and at DROWL.de commented

FYI: Added a feature-request to provide a class to opt-out from reset in contrib, if needed: https://www.drupal.org/project/drupal/issues/3395617
Would be happy about your feedback on the idea!