Fix `select` widget in Drupal Claro by smokris · Pull Request #14917 · darkreader/darkreader · GitHub
Skip to content

Fix select widget in Drupal Claro#14917

Open
smokris wants to merge 2 commits into
darkreader:mainfrom
smokris:patch-1
Open

Fix select widget in Drupal Claro#14917
smokris wants to merge 2 commits into
darkreader:mainfrom
smokris:patch-1

Conversation

@smokris

@smokris smokris commented Jan 8, 2026

Copy link
Copy Markdown

The Drupal content management system's default admin theme styles <select> elements, adding a custom arrow image. Dark Reader's adjustments cause the arrow image to be rendered too large. This PR adds a CSS override to improve the styling.

Default (Dark Reader disabled) Dark Reader before Dark Reader after
drupal-claro-light drupal-claro-dark-before drupal-claro-dark-after

In the "Dark Reader before" screenshot above, I added pink arrows pointing to the misrendered <select> arrow.

@alexanderby

Copy link
Copy Markdown
Member

Could you please screenshot the actual CSS background value of the icon? If it's SVG, it may be potentially fixed in the next release.

@smokris

smokris commented Jan 8, 2026

Copy link
Copy Markdown
Author

Sure, here are screenshots that include the Inspector view of the CSS values:

Dark Reader disabled Dark Reader enabled
drupal-claro-light-css drupal-claro-dark-css

@alexanderby

Copy link
Copy Markdown
Member

Thanks, the issue was those SVGs with viewBox but no width and height were behaving differently to those the Dark Reader was making. Should be fixed in the next release.

@Myshor

Myshor commented Jan 18, 2026

Copy link
Copy Markdown
Collaborator

So... there is new version of Dark Reader released. @smokris can you check if your page works fine with it? If yes then this PR should be closed without merge.

@smokris

smokris commented Jan 18, 2026

Copy link
Copy Markdown
Author

Thanks for the info. I updated to Dark Reader 4.9.119 just now, but unfortunately the problem remains.

Here's a simplified test case that reproduces the problem: test.html

I think there are 2 separate problems here:

  1. The recent SVG parsing changes (8093d80 and 54a14b9) don't handle the way this particular SVG is encoded. I just pushed changes to handle that encoding, including an automated test.

  2. The original CSS sets [background](background: var(--input-bg-color)), and a higher-priority rule specifies background-image, background-repeat, background-position, and background-size. Then Dark Reader injects a rule that sets background: var(--darkreader-bg--input-bg-color);. Since this rule is for background (rather than specifically background-color), it resets the background-repeat, background-position, and background-size values. Would it be possible for Dark Reader to override only the background-color? I didn't immediately see how to implement that, so for now I left in the change to dynamic-theme-fixes.config.

@Myshor

Myshor commented Jan 19, 2026

Copy link
Copy Markdown
Collaborator

@alexanderby I am not that much experienced into code outside config folder. Maybe you can take a look here again. 😉

@alexanderby

Copy link
Copy Markdown
Member

@smokris Thank you for providing a test case and for pointing out the single quotes and the URL encoding. Brilliant!

This is fixed now, as well as overriding of the background-size property, a long-lasting issue.

indexOf has a slight performance and readability advantage over regexps here, so I preferred to use those. But I will happily merge your test case.

--darkreader-bg--white: 23, 23, 23 !important;
--darkreader-text--black: 228, 224, 218 !important;
}
body:has(> div[data-drupal-claro-processed-toolbar]) .form-element--type-select {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The static fix should be unnecessary now.

else {
svgText = decodeURIComponent(svgText);
}
const svgOpeningMatch = svgText.match(/^<svg( .*?)?>/);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I improved the old solution based on indexOf, so the code changes are unnecessary now.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you could put expect(details.useViewBox).toBe(true); that would be awesome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants