WordPress.org

Make WordPress Core

Opened 3 months ago

Closed 3 months ago

Last modified 3 months ago

#53560 closed defect (bug) (fixed)

Twenty Twenty-One: Dropdown arrow missing in the widget editor

Reported by: zieladam Owned by: desrosj
Milestone: 5.8 Priority: normal
Severity: normal Version: 5.8
Component: Bundled Theme Keywords: has-patch commit fixed-major dev-reviewed
Focuses: Cc:

Description (last modified by zieladam)

In the widget editor, the <select /> arrow disappears when focused when twentytwentyone theme is active:

https://user-images.githubusercontent.com/205419/123951705-5f803100-d9a5-11eb-92c6-94d072e01d8a.png

This is caused by reset.css overriding style-editor.css from the theme:

/* reset.css */
.editor-styles-wrapper select:focus {
    color: revert;
    border-color: revert;
    background: revert;
    box-shadow: revert;
    text-shadow: revert;
    cursor: revert;
    transform: revert;
}
/* style-editor.css */
.editor-styles-wrapper select {
        border: var(--form--border-width) solid var(--form--border-color);
        border-radius: var(--form--border-radius);
        color: var(--form--color-text);
        font-size: var(--form--font-size);
        -moz-appearance: none;
        -webkit-appearance: none;
        appearance: none;
        padding: var(--form--spacing-unit) calc(3 * var(--form--spacing-unit)) var(--form--spacing-unit) var(--form--spacing-unit);
        background: var(--global--color-white) url(data:image/svg+xml;utf8,<svg xmlns='http://www.w3.org/2000/svg' width='10' height='10' fill='%2328303d'><polygon points='0,0 10,0 5,5'/></svg>) no-repeat;
        background-position: right var(--form--spacing-unit) top 60%;
}

The rogue reset.css rule was introduced in https://github.com/WordPress/gutenberg/pull/30141 to remove the opinionated style from the theme. At the same time, we want to use at least some theme styles here. There are two ways to approach this:

  1. Fully rely on wp-admin core styles when it comes to form elements in the widgets editor with "use theme styles" feature enabled
  2. Make the theme CSS more specific to overcome the .editor-styles-wrapper select:focus rule from reset.css in this specific case

I went for 2 because it seems safer, especially at RC1/RC2 stage. I don't think this is an ideal solution though – I wonder what y'all think about 1. Would it make sense? Or would it completely defeat the purpose of use theme styles ? Personally I don't mind seeing the "native" wp-admin CSS in the edit mode widgets since, as a user, I mostly care about the preview.

This is how it looks like with https://github.com/WordPress/wordpress-develop/pull/1454 applied:

https://user-images.githubusercontent.com/205419/123951704-5f803100-d9a5-11eb-9366-f22e4bfeba3d.png

Props to @spacedmonkey for reporting this in https://github.com/WordPress/gutenberg/pull/33040

Change History (22)

This ticket was mentioned in PR #1454 on WordPress/wordpress-develop by adamziel.


3 months ago

  • Keywords has-patch added

Problem:

In the widget editor, the <select /> arrow disappears when focused when twentytwentyone theme is active:

<img width="764" alt="Zrzut ekranu 2021-06-30 o 13 01 10" src="https://user-images.githubusercontent.com/205419/123951705-5f803100-d9a5-11eb-92c6-94d072e01d8a.png"/>

This is caused by reset.css overriding style-editor.css from the theme.

reset.css

.editor-styles-wrapper select:focus {
    color: revert;
    border-color: revert;
    background: revert;
    box-shadow: revert;
    text-shadow: revert;
    cursor: revert;
    transform: revert;
}

style-editor.css:

.editor-styles-wrapper select {
	border: var(--form--border-width) solid var(--form--border-color);
	border-radius: var(--form--border-radius);
	color: var(--form--color-text);
	font-size: var(--form--font-size);
	-moz-appearance: none;
	-webkit-appearance: none;
	appearance: none;
	padding: var(--form--spacing-unit) calc(3 * var(--form--spacing-unit)) var(--form--spacing-unit) var(--form--spacing-unit);
	background: var(--global--color-white) url(data:image/svg+xml;utf8,<svg xmlns='http://www.w3.org/2000/svg' width='10' height='10' fill='%2328303d'><polygon points='0,0 10,0 5,5'/></svg>) no-repeat;
	background-position: right var(--form--spacing-unit) top 60%;
}

The rogue reset.css rule was introduced in https://github.com/WordPress/gutenberg/pull/30141 to remove the opinionated style from the theme. At the same time, we want to use at least some theme styles here. There are two ways to approach this:

  1. Fully rely on wp-admin core styles when it comes to form elements in the widgets editor with "use theme styles" feature enabled
  2. Make the theme CSS more specific to overcome the .editor-styles-wrapper select:focus rule from reset.css in this specific case

I went for 2 because it seems safer, especially at RC1/RC2 stage. I don't think this is an ideal solution though – I wonder what y'all think about 1. Would it make sense? Or would it completely defeat the purpose of use theme styles ? Personally I don't mind seeing the "native" wp-admin CSS in the edit mode widgets as I mostly care about the preview.

Test plan:

  1. Activate TwentyTwentyOne theme
  2. Install Web stories plugin
  3. Go to the widgets editor
  4. Add "Web stories" widget
  5. Put focus on any select in the form
  6. Confirm the dropdown arrow is visible

<img width="726" alt="Zrzut ekranu 2021-06-30 o 13 00 03" src="https://user-images.githubusercontent.com/205419/123951704-5f803100-d9a5-11eb-9366-f22e4bfeba3d.png">

Trac ticket: #53560

#2 @zieladam
3 months ago

  • Description modified (diff)

#3 @SergeyBiryukov
3 months ago

  • Component changed from General to Bundled Theme
  • Summary changed from Dropdown arrow missing in the widget editor (twentytwentyone theme) to Twenty Twenty-One: Dropdown arrow missing in the widget editor

#4 @zieladam
3 months ago

  • Component changed from Bundled Theme to General
  • Description modified (diff)
  • Summary changed from Twenty Twenty-One: Dropdown arrow missing in the widget editor to Dropdown arrow missing in the widget editor (twentytwentyone theme)

#5 @desrosj
3 months ago

  • Component changed from General to Bundled Theme
  • Summary changed from Dropdown arrow missing in the widget editor (twentytwentyone theme) to Twenty Twenty-One: Dropdown arrow missing in the widget editor
  • Version set to trunk

I think these changes were reverted accidentally when updating the ticket description.

#6 @desrosj
3 months ago

@poena Do you have any thoughts here?

#7 @poena
3 months ago

I don't have time to look into the issue or the PR within the 5.8 time frame.

-The reset.css is not likely to be reverted.

-If this is a third party plugin issue, not reproducable without the plugin, then it should be fixed in the plugin.
So, are core (widget) select lists still working?

-Accessibility is more important than matching the styles for the editor and front.

#8 @isabel_brison
3 months ago

-If this is a third party plugin issue, not reproducable without the plugin, then it should be fixed in the plugin.
So, are core (widget) select lists still working?

It's not only third party plugins. The issue can be reproduced on the Navigation legacy widget: if there is more than one menu, a select will appear.

-Accessibility is more important than matching the styles for the editor and front.

The issue isn't matching styles in editor and front end, it's that all legacy widget selects in the editor inherit some of the theme styles, but in this case the select background image is being overridden by the editor reset styles on focus. The only thing Adam's PR does is make sure that background image style is also applied to the select in its focused state, which doesn't interfere with theme focus styling in any way, so is unlikely to create an accessibility issue. It's also a lower impact change than removing styles from the editor reset stylesheet would be, which is important at this point in the release cycle.

#9 @poena
3 months ago

Isabel I never wrote that there was an accessibility problem with the PR,-I specifically wrote that I did not have time to look at the PR.

Accessibility is most important, please interpret that as:
-My thought on the issue, as requested.
-My opinion is that the arrow must always be visible. That anything else is secondary.

#10 @zieladam
3 months ago

  • Keywords commit added

Since @isabel_brison approved the Github PR and the resolution is consistent with @poena guidelines (that the arrow is visible), I'll add the commit keyword here

#11 @desrosj
3 months ago

  • Owner set to desrosj
  • Status changed from new to reviewing

#12 @desrosj
3 months ago

The change in the PR will also need to be made to assets/sass/04-elements/forms-editor.scss, otherwise it will be removed when npm run build is run next in the theme. That will ensure the change is applied to the ie-editor.css file as well.

I can handle that before committing!

#13 @desrosj
3 months ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 51296:

Twenty Twenty-One: Ensure the dropdown arrow displays for <select> elements when focused.

Props isabel_brison, zieladam, poena.
Fixes #53560.

#14 @desrosj
3 months ago

  • Keywords fixed-major added

Reopening for backport consideration.

#15 @desrosj
3 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

#16 @zieladam
3 months ago

Thank you @desrosj !

#17 @desrosj
3 months ago

  • Keywords dev-feedback added

#18 @jorbin
3 months ago

  • Keywords dev-reviewed added; dev-feedback removed

Looks good for backporting

#19 @SergeyBiryukov
3 months ago

It seems weird that these rules are fully duplicated in the assets/css/ie-editor.css file:

select {
	border: 3px solid #39414d;
	border-radius: 0;
	color: #28303d;
	font-size: 1.125rem;
	-moz-appearance: none;
	-webkit-appearance: none;
	appearance: none;
	padding: 10px 30px 10px 10px;
	background: #fff url("data:image/svg+xml;utf8,<svg xmlns='http://www.w3.org/2000/svg' width='10' height='10' fill='%2328303d'><polygon points='0,0 10,0 5,5'/></svg>") no-repeat;
	background-position: right 10px top 60%;
}

select:focus {
	border: 3px solid #39414d;
	border-radius: 0;
	color: #28303d;
	font-size: 1.125rem;
	-moz-appearance: none;
	-webkit-appearance: none;
	appearance: none;
	padding: 10px 30px 10px 10px;
	background: #fff url("data:image/svg+xml;utf8,<svg xmlns='http://www.w3.org/2000/svg' width='10' height='10' fill='%2328303d'><polygon points='0,0 10,0 5,5'/></svg>") no-repeat;
	background-position: right 10px top 60%;
}

while they are properly combined in assets/css/style-editor.css:

select,
select:focus {
	border: var(--form--border-width) solid var(--form--border-color);
	border-radius: var(--form--border-radius);
	color: var(--form--color-text);
	font-size: var(--form--font-size);
	-moz-appearance: none;
	-webkit-appearance: none;
	appearance: none;
	padding: var(--form--spacing-unit) calc(3 * var(--form--spacing-unit)) var(--form--spacing-unit) var(--form--spacing-unit);
	background: var(--global--color-white) url("data:image/svg+xml;utf8,<svg xmlns='http://www.w3.org/2000/svg' width='10' height='10' fill='%2328303d'><polygon points='0,0 10,0 5,5'/></svg>") no-repeat;
	background-position: right var(--form--spacing-unit) top 60%;
}

Looking closer, ie-editor.css has a lot more unnecessarily duplicated rules, for example:

.has-background .has-link-color a {
	color: #28303d;
}

.has-background.has-link-color a {
	color: #28303d;
}

I would expected to see this instead:

.has-background .has-link-color a,
.has-background.has-link-color a {
	color: #28303d;
}

Could it be an issue with PostCSS configuration? I've noticed that postcss.config.js for the theme requires the postcss-discard-duplicates module, but it seems like it's not being used, or I'm misunderstanding how it works.

This should probably be investigated in a new ticket though, and should not be a blocker for backporting [51296].

Version 0, edited 3 months ago by SergeyBiryukov (next)

#20 @desrosj
3 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 51336:

Twenty Twenty-One: Ensure the dropdown arrow displays for <select> elements when focused.

Props isabel_brison, zieladam, poena.
Merges [51296] to the 5.8 branch.
Fixes #53560.

#21 @desrosj
3 months ago

I've opened up #53605 to investigate the duplicate rules flagged above.

Note: See TracTickets for help on using tickets.