Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use a specific CSS class for the CSS reset mixin. #16856

Merged
merged 3 commits into from
Aug 2, 2019

Conversation

afercia
Copy link
Contributor

@afercia afercia commented Aug 1, 2019

Description

After #14509, the CSS rules for form controls target also the metaboxes area, conflicting with styles provided by plugins.

See for example Yoast/wordpress-seo#13316

For now, the new form controls styles are used only in Gutenberg. Eventually, when these styles will be used in core, WordPress would expect plugins to honor the new styles but their CSS selectors would need to have a lower specificity.

In fact, WordPress styles form controls using CSS selectors with a relatively low specificity. For example: input[type="text"]. Instead, the selectors used in Gutenberg need to have a higher specificity. By doing so, their associated CSS rules tend to override the CSS provided by plugins.

Once the new form controls styles will be in core, this problem will go away. For now, I'd like to propose to reinforce the principle that Gutenberg should do its best to not create conflicts with CSS provided by plugins.

This PR seeks to better scope the reset mixin:

  • introduces a new CSS class block-editor-styles-reset: its sole responsibility is to apply the reset mixin
  • sets the block-editor-styles-reset class on the relevant places in the UI

Open to better naming for the new CSS class, of course.

Added bonus
If plugins want to use the new form controls styles, they can opt for that by simply applying the new CSS class on their UI.

Test instructions:
Check all form controls have the new styles in the following places in the UI:

  • the post content with the list of blocks (e.g. insert a Table block and check the input fields for the number of columns/rows)
  • the post permalink UI
  • the normal sidebar
  • the publish sidebar
  • all the popovers (e.g. the inserter or the publish date one)
  • all the modal dialogs (e.g. the Block Manager or the Options one)
  • install / activate one or more plugins that render some UI in the metaboxes area (e.g. Advanced Custom Fields, Yoast SEO)
  • verify the new Gutenberg form controls styles do not apply to the plugins UI within the metaboxes area
  • activate the "Custom Fields" panel from the "Options" Gutenberg modal dialog
  • verify the new Gutenberg form controls styles do not apply to the Custom Fields area

// The modals are shown outside the .block-editor wrapper, they need these styles
.components-modal__frame {
// Target the editor UI but exclude the metaboxes and custom fields areas.
.block-editor-styles-reset {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit concerned about all these reset classes added everywhere. Can't we replace the previous .block-editor with something more specific instead?

Also, ideally we should try to remove that "reset" entirely by moving the styles to the specific components making the components more reusable without relying on global styles.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

block-editor contains the meta boxes and custom fields area. The only way would be to use a :not(), and I'm not sure if that would be any better. Another approach would be to target each UI section individually, as it worked before. Something like:

.editor-post-permalink,
.edit-post-sidebar,
.editor-post-publish-panel,
.editor-styles-wrapper,
.components-popover,
.components-modal__frame { ... }

but that would produce several lines of CSS selectors, while a single class would produce way less lines. No strong preferences on my side.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ideally we should try to remove that "reset" entirely by moving the styles to the specific components making the components more reusable without relying on global styles.

I'd agree. A bit out of the scope of this PR though.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have a preference for the :not approach if it works otherwise listing the selectors is better. That way we avoid introducing a block-editor-styles-reset class in a generic component like "modal". A Modal is agnostic of where it's being used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK thanks, will try the :not() approach as soon as I have some time.

@afercia
Copy link
Contributor Author

afercia commented Aug 1, 2019

Tried the :not() approach, but it increases the selector specificity thus triggering borders on element that are not expected to have borders. Potentially triggering other unexpected effects.

Screenshot 2019-08-01 at 15 12 00

Screenshot 2019-08-01 at 15 15 17

These form controls rules should use selectors with a specificity as low as possible.

@afercia
Copy link
Contributor Author

afercia commented Aug 1, 2019

Switched to the "list selectors individually" approach.

.block-editor,
// The modals are shown outside the .block-editor wrapper, they need these styles
// Target the editor UI excluding the metaboxes and custom fields areas.
.editor-styles-wrapper,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we replace this one with .edit-post-visual-editor instead?
What about the text editor .edit-post-text-editor?
I also noticed that the notices are shown outside that div should we add the reset for them or is it unnecessary:

.edit-post-layout__content > .components-notice-list,
.edit-post-layout__content > .components-snackbar-list

Copy link
Contributor

Choose a reason for hiding this comment

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

What about the top bar as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we replace this one with .edit-post-visual-editor instead?

Aren't they equivalent? If you mean for "semantic" reasons, no objections!

What about the text editor .edit-post-text-editor?

Yup, looks like it's needed for the permalinks UI there. Seems to me that's the only case of form controls within the text-editor area. Will add.

I also noticed that the notices ar

Yep, I did notice the notices. Not sure: is Gutenberg going to use form controls within the notices? It may need the box-sizing: border-box; part.

What about the top bar as well?

Same: will there be form controls within the top bar? I think the top bar needs only the box-sizing: border-box; part.

Copy link
Contributor

Choose a reason for hiding this comment

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

Aren't they equivalent? If you mean for "semantic" reasons, no objections!

yes, mosty for semantic but also to prevent bugs as editor-styles-wrapper can be used in several places to say "apply the editor styles here" (HTML prevew, block previews...)

@afercia
Copy link
Contributor Author

afercia commented Aug 1, 2019

Last commit should cover the points above.

I'd tend to think the "reset" for box-sizing should be in a separated mixin. For example, it doesn't make much sense to put all the form controls rules in a selector that targets the top bar, as the top bar doesn't make use of any form control (at least for now).

Applying box-sizing: border-box; directly on the top bar container wouldn't work because it needs to target all its children as well via .selector * { box-sizing: inherit; }.

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

Thanks, it seems to work as intended.

I agree with avoiding the global border-box and I tend to think we should try to do a push to remove all these resets at some point.

@afercia
Copy link
Contributor Author

afercia commented Aug 2, 2019

Thanks for the reviews!

Yup, maybe we should consider to switch core to border-box everywhere.
/me runs

@afercia afercia merged commit 2d369dc into master Aug 2, 2019
@afercia afercia deleted the update/better-scope-css-reset branch August 2, 2019 10:26
@youknowriad youknowriad added this to the Gutenberg 6.3 milestone Aug 9, 2019
@senadir
Copy link
Contributor

senadir commented Aug 16, 2019

@youknowriad wouldn't this pattern fix a lot of box sizing inheritance issues?

html {
    box-sizing: border-box;
}

*,
*:before,
*:after {
    box-sizing: inherit;
}

makes fixing a group things much easier since this PR caused two content box regressions so far

@youknowriad
Copy link
Contributor

@senadir I think the resets should be eliminated, this pattern is not that different it makes the component depend on a global style. Ideally, this is applied to each component so when a user loads the component in its own application, it works without requiring global resets.

gziolo pushed a commit that referenced this pull request Aug 29, 2019
* Use a specific CSS class for the CSS reset mixin.

* List selectors individually instead.

* Add selectors for the missing UI parts.
gziolo pushed a commit that referenced this pull request Aug 29, 2019
* Use a specific CSS class for the CSS reset mixin.

* List selectors individually instead.

* Add selectors for the missing UI parts.
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