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

Reduce paragraph block CSS specificity #13025

Merged
merged 4 commits into from
Feb 11, 2019

Conversation

m-e-h
Copy link
Member

@m-e-h m-e-h commented Dec 19, 2018

Description

Move drop-cap:focus style to editor.css.
Reduce specificity on front-end drop-cap by removing the :not(:focus) pseudo-class.
Show a visual hint that a dropcap is present while focused by:

  • make the first letter a little larger, 1.2em
  • make the first letter bolder by using a text-shadow of the same color
  • keep the uppercase text-transform.

How has this been tested?

Visual tests in browser.

Screenshots

dropcap

Types of changes

Bug fix (non-breaking change which fixes an issue)

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.

@m-e-h
Copy link
Member Author

m-e-h commented Dec 19, 2018

This PR also helps to chip away at some of
#11677
and some of
#11668

@m-e-h m-e-h changed the title Fix/dropcap visual on focus Drop cap visual on focused editor Dec 19, 2018
Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

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

Hi @m-e-h, I agree with the logic of these changes, I think focus styles that are editor specific should not be referenced on the front end styles.
Unfortunately, I think we will not be able to differentiate the first letter with this styles, as even this simple styles change causes the bug shown in the following gif:
dec-20-2018 15-30-34

@m-e-h
Copy link
Member Author

m-e-h commented Dec 20, 2018

Thanks for looking at this @jorgefilipecosta!

That appears to be a Chrome bug. Works fine in Firefox.
I'll see if I can find out what's going on.

@m-e-h
Copy link
Member Author

m-e-h commented Dec 20, 2018

Yep, it's a known bug in Chrome. https://bugs.chromium.org/p/chromium/issues/detail?id=174349
Probably accounts for some of the earlier issues the editor had with Drop cap.

The cursor bug becomes present even with an empty rule in the stylesheet:

.has-drop-cap:focus::first-letter {}

So it looks like :not(:focus) is the only solution. At least until this gets fixed in Chrome.

@m-e-h
Copy link
Member Author

m-e-h commented Dec 21, 2018

Though it would be nice to ultimately remove that :not(:focus) style from the front end, I don't see any way around it in the foreseeable future. Not much activity on that Chromium bug page.

Should I close this?

One sort of separate thing I'd also like to do here with the dropcap styles is remove the p qualifier preceding each selector.

Does anyone know if there's a reason for it being there?

@gziolo gziolo added [Block] Paragraph Affects the Paragraph Block Needs Design Feedback Needs general design feedback. labels Feb 7, 2019
@gziolo gziolo added this to the 5.1 (Gutenberg) milestone Feb 7, 2019
@gziolo gziolo requested review from jasmussen and kjellr February 7, 2019 13:09
@jasmussen
Copy link
Contributor

Nice work here. Some GIFs will help.

This branch:

this branch

Master:

master

I appreciate what you're trying to do here, notably the reduction in CSS specificity. But the bold first letter goes a little bit against the principle that the unselected block is a preview and the selected block can be an edit mode. I deeply appreciate what you're trying to do here, but honestly I feel like what we have in master with regards to the select and edit behavior is the better solution.

But the change to CSS specificity in style.scss is a total no-brainer.

Can you change this PR so the only change is the reduction in CSS specificity in the style.scss file? I think that'd be worth shipping.

@m-e-h
Copy link
Member Author

m-e-h commented Feb 8, 2019

I removed the dropcap changes that I tried on the editor.scss.
I removed the qualifying p tag from each selector in style.scss, thereby decreasing the specificity.

But @jasmussen I'm bringing back the editor dropcap argument if and when Chrome ever fixes it's :focus:first-letter bug. 😃

@m-e-h m-e-h changed the title Drop cap visual on focused editor Reduce paragraph block CSS specificity Feb 8, 2019
Copy link
Contributor

@kjellr kjellr left a comment

Choose a reason for hiding this comment

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

Looks good, thanks @m-e-h!

@kjellr kjellr merged commit a4412e6 into WordPress:master Feb 11, 2019
&.is-small-text {
font-size: 14px;
}
.is-small-text {
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this selector a bit too generic? What about other blocks that use a class with this same name? Shouldn't this class be either namespaced (e.g. .wp-block-paragraph__is-small-text or changed to something along the lines of .wp-block-paragraph.is-small-text?

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably should've been .wp-block-paragraph__is-small-text.
In the case of this PR though, does the p qualifier really make it any less generic?

I think .wp-block-paragraph.is-small-text is too specific for a core style. Especially with something like font-size which is likely one of the first things that a theme will override.

Copy link
Member Author

Choose a reason for hiding this comment

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

For the record though, I would prefer .wp-block-paragraph.is-small-text over p.is-small-text. Since there are more appropriate hooks in place for themes to override the Paragraph Block font-sizes.

As for being generic, the one that sticks out to me here is .has-background. That's not even "text" specific and we're adding some hefty padding here. Is that class used in any other blocks?

Copy link
Member Author

@m-e-h m-e-h Feb 11, 2019

Choose a reason for hiding this comment

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

Dang! This is going to have to be fixed. The Button block also uses .has-background and is getting the padding from the Paragraph block CSS. Guess We're going to have to increase specificity for the paragraph block rather than decrease.

@kjellr How should I go about fixing? A new PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah yes, good catch. I missed that selector. Open up a new PR and I'll take a look shortly.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the has-background and has-text-color selectors are the only ones that definitely should be changed for now. I don't believe the text size and dropcap ones are used elsewhere.

Copy link
Contributor

Choose a reason for hiding this comment

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

Had a few minutes, so I opened #13821 to take care of this. 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @kjellr!
Totally my fault. The PR was originally for the Dropcap and sort of morphed into being about specificity. I didn't put as much thought into it as I should have.

I agree that the other selectors don't look as problematic as has-background. Even if something like is-small-text is used in other blocks, we're only setting the font size, which would be expected for such a class name and would likely be the same or overridden by the other block.

It's the padding on a generic "background" selector that made this troublesome.
It could be debated that the padding style is more appropriate in theme.css. But that's a topic for another day. 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

You're all amazing. Thanks for the hard work here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Totally my fault. The PR was originally for the Dropcap and sort of morphed into being about specificity. I didn't put as much thought into it as I should have.

No problem at all! It's all set now. Thanks again for your contribution, and thanks @ZebulanStanphill for starting the discussion that led to us catching this. 👍

kjellr added a commit that referenced this pull request Feb 11, 2019
This extra specificity was removed in #13025, but should be restored because other blocks (buttons, pullquotes, etc),  use the `has-background` and `has-text-color` classes. We don't want the styles here to interfere.

Using a `p` selector here, rather than something like `.wp-block-paragraph` so that this gets picked up correctly on the front-end as well.
}
.is-larger-text {
font-size: 48px;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the right file for these stylesheets? If I remember properly, these are theme options + editor styles? shouldn't we move these styles to the build-int editor-styles.css instead?

kjellr added a commit that referenced this pull request Feb 12, 2019
…13821)

This extra specificity was removed in #13025, but should be restored because other blocks (buttons, pullquotes, etc),  use the `has-background` and `has-text-color` classes. We don't want the styles here to interfere.

Using a `p` selector here, rather than something like `.wp-block-paragraph` so that this gets picked up correctly on the front-end as well.
youknowriad pushed a commit that referenced this pull request Mar 6, 2019
* move dropcap focus to editor.css. Show hint of DC on focus

* Move dropcap focus to editor.css. Reduce specificity

* Easing up on paragraph block specificity.
youknowriad pushed a commit that referenced this pull request Mar 6, 2019
…13821)

This extra specificity was removed in #13025, but should be restored because other blocks (buttons, pullquotes, etc),  use the `has-background` and `has-text-color` classes. We don't want the styles here to interfere.

Using a `p` selector here, rather than something like `.wp-block-paragraph` so that this gets picked up correctly on the front-end as well.
youknowriad pushed a commit that referenced this pull request Mar 6, 2019
* move dropcap focus to editor.css. Show hint of DC on focus

* Move dropcap focus to editor.css. Reduce specificity

* Easing up on paragraph block specificity.
youknowriad pushed a commit that referenced this pull request Mar 6, 2019
…13821)

This extra specificity was removed in #13025, but should be restored because other blocks (buttons, pullquotes, etc),  use the `has-background` and `has-text-color` classes. We don't want the styles here to interfere.

Using a `p` selector here, rather than something like `.wp-block-paragraph` so that this gets picked up correctly on the front-end as well.
This was referenced Apr 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Paragraph Affects the Paragraph Block Needs Design Feedback Needs general design feedback.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants