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

Remove padding inheritance on lists in editor-styles #23080

Merged
merged 1 commit into from
Jun 11, 2020

Conversation

oxyc
Copy link
Member

@oxyc oxyc commented Jun 10, 2020

Description

Currently <ul> and <ol> elements displayed in the editor inherit their padding from their parent elements for a (to me) unknown reason. @jasmussen can you recall why it was set?

The issue is that if a theme sets a padding on the inner container of eg. Media & Text, Cover or Group, on the editor side that same padding gets applied to child lists as well. Something that's becoming more apparent now with the lighter block DOM I guess.

Overall I'm not confident if it can be unset and someone else should probably have a look at this too.

The one thing it does change is that first level lists and lists within Media & text blocks no longer have a padding-right inherited but I dont see the purpose of this. Eg. paragraphs do not have this padding.

How has this been tested?

I tested by enabling twentytwenty and toggling the removal of the attribute after creating a list block as a direct child of the post as well as within a Media & Text block, a Cover Block and a Group block.

Types of changes

Bug fix

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@jasmussen
Copy link
Contributor

Hello, and thanks so much for the PR!

@jasmussen can you recall why it was set?

Yes, the problem is CSS bleed from wp-admin. In /wp-admin/css/common.css you have the following:

ul,
ol {
	padding: 0;
}

The editing canvas is not protected from CSS bleed, because it's not in an iframe (but that might happen in #21102, at which point this conversation would be different).

This means that if a theme does not provide an editor style, you'll end up with lists that look like this:

Screenshot 2020-06-11 at 08 02 06

I tried both unset and initial, but neither of those values correctly reset the ul to its browser-defaults.

So yes, I really want to remove this property, and indeed as many others as we can. But outside of a patch to improve wp-admin itself for better scoping, or an iframe surrounding the editing canvas, I don't see an easy way to do this.

What do you think?

@oxyc
Copy link
Member Author

oxyc commented Jun 11, 2020

@jasmussen right, since then 2ea41bf was added though so lists now have the padding-left explicitly set (sorry it was late and forgot to mention that in the issue). Should it not be fine to remove the generic inherit now?

@jasmussen
Copy link
Contributor

Oh yes indeed, I suppose that does make it fine to remove the generic inherit! One moment let me test some things.

@jasmussen jasmussen self-requested a review June 11, 2020 10:45
Copy link
Contributor

@jasmussen jasmussen left a comment

Choose a reason for hiding this comment

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

Nice work, thank you! Tests well for me with various themes, and even with groups, and groups inside columns, and media & text. Things appear to look good.

Screenshot 2020-06-11 at 12 44 08

Screenshot 2020-06-11 at 12 44 31

Screenshot 2020-06-11 at 12 44 42

Screenshot 2020-06-11 at 12 45 00

Are you able to merge this yourself or do you need me to press the button? Thanks again for the PR.

@jasmussen jasmussen added the [Feature] Custom Editor Styles Functionality for adding custom editor styles label Jun 11, 2020
@oxyc
Copy link
Member Author

oxyc commented Jun 11, 2020

Are you able to merge this yourself or do you need me to press the button? Thanks again for the PR.

I dont have permission so you go :)

@jasmussen jasmussen merged commit a63375c into WordPress:master Jun 11, 2020
@github-actions github-actions bot added this to the Gutenberg 8.4 milestone Jun 11, 2020
@oxyc oxyc deleted the editor-list-padding branch June 11, 2020 11:13
This was referenced Jun 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Custom Editor Styles Functionality for adding custom editor styles
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants