Problem/Motivation

This is part of the CSS modernization initiative, and intended to be worked on by our Google Summer of Code student only.

Steps to reproduce

The stylesheet at https://git.drupalcode.org/project/drupal/-/blob/10.0.x/core/themes/claro/css/components/dropbutton.pcss.css needs to be refactored to make use of modern CSS and Drupal core's PostCSS tooling.

Proposed resolution

Use CSS Logical Properties where appropriate
Use CSS nesting where appropriate

Remaining tasks

We need two patches. One for Drupal 9.5.x and one for Drupal 10.0.x
We need a followup issue to refactor this component in Drupal 10.0.x to make use of component-level CSS custom properties and remove IE specific style definitions.

User interface changes

None. There should be no visual differences.

CommentFileSizeAuthor
#51 Screenshot at Dec 18 16-51-59.png90.64 KBshweta__sharma
#46 3303547-46.patch980 bytesAkshayAdhav
#45 dropbutton-wrapper-11.x.png640.07 KBAkshayAdhav
#45 dropbutton-wrapper-10.2.x.png597.63 KBAkshayAdhav
#42 interdiff-38_42.txt942 bytesGauravvvv
#42 3303547-42.patch16.31 KBGauravvvv
#39 Screenshot from 2023-10-24 16-34-37.png4.71 KBkostyashupenko
#39 Screenshot from 2023-10-24 16-34-34.png3.29 KBkostyashupenko
#39 Screenshot from 2023-10-24 16-34-19.png3.64 KBkostyashupenko
#39 Screenshot from 2023-10-24 16-34-10.png2.68 KBkostyashupenko
#38 interdiff_36-38.txt1.23 KBkostyashupenko
#38 3303547-38.patch16.53 KBkostyashupenko
#36 interdiff-34_36.txt5.94 KBGauravvvv
#36 3303547-36.patch16.28 KBGauravvvv
#34 3303547-34.patch17.12 KB_utsavsharma
#34 interdiff_32-34.txt950 bytes_utsavsharma
#32 3303547-32.patch17.23 KBkostyashupenko
#30 3303547-30.patch17.22 KB_utsavsharma
#30 interdiff_d11.txt17.22 KB_utsavsharma
#27 3303547-27.patch17.22 KBStanzin
#9 3303547-nr-bot.txt143 bytesneeds-review-queue-bot
#8 3303547-8.patch33.05 KBshivam-kumar
#5 3303547-5.patch33.05 KBSakthivel M
#3 3303547-version10.0.x.patch33.2 KBAditya4478
#2 3303547-version9.5.x.patch29.08 KBAditya4478

Issue fork drupal-3303547

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:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sasanikolic created an issue. See original summary.

Aditya4478’s picture

Status: Active » Needs review
FileSize
29.08 KB
Aditya4478’s picture

Version: 9.5.x-dev » 10.0.x-dev
FileSize
33.2 KB
sasanikolic’s picture

Status: Needs review » Needs work
  1. +++ b/core/themes/claro/css/components/dropbutton.pcss.css
    @@ -37,23 +37,23 @@
    +  .form-actions & {
    

    This & at the end is not needed.

  2. +++ b/core/themes/claro/css/components/dropbutton.pcss.css
    @@ -37,23 +37,23 @@
    +  [dir="rtl"] .form-actions & {
    

    Let's first revert the rtl changes.

  3. +++ b/core/themes/claro/css/components/dropbutton.pcss.css
    @@ -37,23 +37,23 @@
    +  .js .dropbutton-wrapper.open & {
    

    This is compiled to .dropbutton-widget .js .dropbutton-wrapper.open & {} but should instead be compiled to .js .dropbutton-wrapper.open .dropbutton-widget {}.

  4. +++ b/core/themes/claro/css/components/dropbutton.pcss.css
    @@ -64,23 +64,24 @@
    +  .js & {
    

    Same as above. Js should be the parent selector of .dropbutton.

I think this is a good file to start introducing better (local) css variables, but there is still an ongoing discussion on the naming convention in https://www.drupal.org/project/drupal/issues/3257287.

Sakthivel M’s picture

#5 Please review the patch

ckrina’s picture

ckrina’s picture

Issue tags: +CSS, +frontend
shivam-kumar’s picture

It seems patch could not be applied to 10.1.x-dev. Needs to be worked for 10.1.x.

needs-review-queue-bot’s picture

Status: Needs review » Needs work
FileSize
143 bytes

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.

Gauravvvv’s picture

Version: 10.0.x-dev » 10.1.x-dev

Gauravvvv’s picture

Status: Needs work » Needs review
smustgrave’s picture

Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative

More nesting can be done for

.dropbutton__toggle
.js.no-touchevents

large file but a lot of those can be folded into each other.

starshaped’s picture

Assigned: Unassigned » starshaped

andy-blum made their first commit to this issue’s fork.

andy-blum’s picture

Status: Needs work » Needs review

Opened a new MR using @Gauravvvv and @starshaped's cherrypicked commits from !3547 to untangle a botched rebase. Going forward, as long as this issue doesn't get bumped to 10.2.x, contributors should be fine to merge origin/10.1.x back into this MR branch.

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @andy-blum!

starshaped’s picture

Status: Reviewed & tested by the community » Needs work

@smustgrave, I actually wasn't quite done with my refactoring and still need to fix a few issues, so I'm moving this back to needs work. Thanks @andy-blum for fixing my mess, I'll work off that new MR!

andy-blum’s picture

Also #17 is not in any way a review - just getting things back on track.

Santosh_Verma’s picture

Hi @starshaped
In the last commit (7a272525 - Cleaned up CSS) you removed logical property and wrote normal css in place of that.
But I think this is wrong approach as in the Proposed resolution they asked to Use CSS Logical Properties where appropriate. :)

starshaped’s picture

Assigned: starshaped » Unassigned

@Santosh_Verma Yep, this was on purpose because the styles would not display correctly in RTL mode when using logical properties. Since using logical properties here caused a regression in the layout, I used non-logical properties instead.

Also, unassigning myself to this issue as I haven't had time to work on it--anyone, feel free to pick this one up!

Gauravvvv’s picture

Status: Needs work » Needs review
smustgrave’s picture

Status: Needs review » Needs work
Issue tags: +Needs screenshots

This has changed a few times. Could we get before/after screenshots please. Also screenshots of rtl please.

Thanks.

Santosh_Verma’s picture

I have tested the #MR3615

There are few error in the patch


Checking patch core/themes/claro/css/components/dropbutton.pcss.css...
error: while searching for:
  overflow: visible;
  margin: 0;
  list-style: none;
}
[dir="rtl"] .dropbutton {
  margin: 0;
}

.js .dropbutton {
  min-width: 6rem; /* Help mitigate long dropbutton text from obscuring other dropbuttons. */
  height: var(--dropbutton-toggle-size);
}

/* Variants. */
.js.no-touchevents .dropbutton--small {
  height: var(--dropbutton-small-toggle-size);
}

.js.no-touchevents .dropbutton--extrasmall {
  height: var(--dropbutton-extrasmall-toggle-size);
}

/**
 * First dropbutton list item.
 */
.js .dropbutton--multiple .dropbutton__item:first-of-type {
  margin-right: calc(var(--dropbutton-toggle-size) + var(--dropbutton-toggle-size-spacing)); /* LTR */
}
[dir="rtl"].js .dropbutton--multiple .dropbutton__item:first-of-type {
  margin-right: 0;
  margin-left: calc(var(--dropbutton-toggle-size) + var(--dropbutton-toggle-size-spacing));
}

/* First dropbutton list item variants */
.js.no-touchevents .dropbutton--multiple.dropbutton--small .dropbutton__item:first-of-type {
  margin-right: calc(var(--dropbutton-small-toggle-size) + var(--dropbutton-toggle-size-spacing)); /* LTR */
}
[dir="rtl"].js.no-touchevents .dropbutton--multiple.dropbutton--small .dropbutton__item:first-of-type {
  margin-right: 0;
  margin-left: calc(var(--dropbutton-small-toggle-size) + var(--dropbutton-toggle-size-spacing));
}

.js.no-touchevents .dropbutton--multiple.dropbutton--extrasmall .dropbutton__item:first-of-type {
  margin-right: calc(var(--dropbutton-extrasmall-toggle-size) + var(--dropbutton-toggle-size-spacing)); /* LTR */
}
[dir="rtl"].js.no-touchevents .dropbutton--multiple.dropbutton--extrasmall .dropbutton__item:first-of-type {
  margin-right: 0;
  margin-left: calc(var(--dropbutton-extrasmall-toggle-size) + var(--dropbutton-toggle-size-spacing));
}

/**

error: patch failed: core/themes/claro/css/components/dropbutton.pcss.css:62
error: core/themes/claro/css/components/dropbutton.pcss.css: patch does not apply

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Stanzin’s picture

Status: Needs work » Needs review
FileSize
17.22 KB
Aditya4478’s picture

Status: Needs review » Reviewed & tested by the community

Try to attach screenshot too Stan

Patch LGTM !

nod_’s picture

Status: Reviewed & tested by the community » Needs work

patch doesn't apply to 11.x

_utsavsharma’s picture

Patch for 11.x.

_utsavsharma’s picture

Status: Needs work » Needs review
kostyashupenko’s picture

Can't apply patch from #30, so did reroll again of the patch from #27 against 11.x

nod_’s picture

Status: Needs review » Needs work
+++ b/core/themes/claro/css/components/dropbutton.pcss.css
@@ -250,63 +236,63 @@
+  & > *:active {

The nesting is not right here. Resulting selector is
.dropbutton__item:first-of-type > * > *:active Where it should be

.dropbutton__item:first-of-type > *:active {
_utsavsharma’s picture

Status: Needs work » Needs review
FileSize
950 bytes
17.12 KB

Addressed pointer in #33.

nod_’s picture

Status: Needs review » Needs work

There are a few other selectors that needs to be fixed, check out the diff on the generated css file, anything with > * > * looks suspicious and probably needs to be fixed.

Gauravvvv’s picture

Status: Needs work » Needs review
FileSize
16.28 KB
5.94 KB

Addressed #35, also fixed some logical properties. Attached interdiff for same.

smustgrave’s picture

Status: Needs review » Needs work

For the screenshots but looking at the patch

    border: 0.125rem solid;
    border-width: 0.125rem 0.125rem 0 0;

Is this not overriding itself?

kostyashupenko’s picture

Status: Needs work » Needs review
FileSize
16.53 KB
1.23 KB
kostyashupenko’s picture

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Believe this is good to go.

nod_’s picture

+++ b/core/themes/claro/css/components/dropbutton.css
@@ -324,7 +309,7 @@
-.dropbutton__item:first-of-type > *:active {
+.dropbutton__item:first-of-type:active {

Is this change intended?

Gauravvvv’s picture

No, this is not intended. I have fixed it. I have attached the patch and interdiff for same.

ckrina’s picture

Status: Reviewed & tested by the community » Fixed

Committed 722c7c6 and pushed to 11.x. Thanks!

  • ckrina committed 722c7c63 on 11.x
    Issue #3303547 by Gauravvvv, starshaped, kostyashupenko, _utsavsharma,...
AkshayAdhav’s picture

Status: Fixed » Needs work
FileSize
597.63 KB
640.07 KB

In 10.2.x version the code for .form-actions .dropbutton-wrapper was:

.form-actions .dropbutton-wrapper,
.field-actions .dropbutton-wrapper {
  margin: var(--space-xs) var(--space-m) var(--space-xs) 0;
}

With this code, the dropdown had proper alignment.
dropbutton-wrapper-10.2.x

In 11.x version it has been changed to:

.form-actions .dropbutton-wrapper,
.field-actions .dropbutton-wrapper {
  margin-block: var(--space-xs);
  margin-inline: var(--space-m) 0;
}

And now it's breaking the dropdown alignment.
dropbutton-wrapper-11.x

Steps to reproduce it:
- Install the contrib module Workbench Moderation
- Now after enabling the module edit any content type inside the structure and enable the moderation from the Manage moderation tab.
- Now create new content of the respective content type and now you can see the issue.

AkshayAdhav’s picture

Creating a patch file instead of raising an MR as I am getting issues while creating a new branch from 11.x branch.

AkshayAdhav’s picture

Status: Needs work » Needs review
shweta__sharma’s picture

Hi @AkshayAdhav,
The alignment issue is handling in this - https://www.drupal.org/project/drupal/issues/3391800
Thanks

AkshayAdhav’s picture

Hi @shweta__sharma
The issue you have mentioned is about vertical displacement along with the issue of the button's text length.
As this particular issue was closed with wrong property values, it was adding left margin instead of right margin. You can check the screenshots attached in my comment #45.

shweta__sharma’s picture

Ohh! Thanks, @AkshayAdhav for the clarity. I tested your MR 5859 on Drupal 11 the MR is cleanly applied and the issue is fixed. The space is added between buttons now. Attached screenshot for reference.

after-patch

shweta__sharma’s picture

Status: Needs review » Reviewed & tested by the community
quietone’s picture

Status: Reviewed & tested by the community » Needs review

I'm triaging RTBC issues. I read the IS and the comments.

This was committed to 11.x and then re-opened for a problem with the contrib module, Workbench Moderation. My first question is this reproducible with just Drupal core? The same comment mentions 10.2.x, so is this a problem only on 10.2?

I have found that opening Fixed issues makes it difficult to track history of problems, so the discussion from #45 onwards may need to move to a new issue.

  • nod_ committed 39fb7eb7 on 11.x
    Issue #3303547 by Gauravvvv, AkshayAdhav, starshaped, kostyashupenko,...
nod_’s picture

Status: Needs review » Fixed
Issue tags: -Needs screenshots

Committed 39fb7eb and pushed to 11.x. Thanks!

I opted to commit it from here since this is a simple mistake and it should have been part of the initial patch. I do not see it as a sign there is a deeper issue with the previous patch.

nod_’s picture

and to anwser the questions.

The problem is not present in 10.2 because it doesn't have the refactor, 10.2 version was used as an example of what the code and behavior should be.
It is probably not possible to replicate with core since we got rid of dropbuttons on those kind of submits a while back.

quietone’s picture

@nod_, thanks!

Status: Fixed » Closed (fixed)

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