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 inline style colors atributes from paragraph. #3722

Closed
wants to merge 6 commits into from

Conversation

jorgefilipecosta
Copy link
Member

@jorgefilipecosta jorgefilipecosta commented Nov 29, 2017

Description

This is a second take on #3669 and tries to accomplish the first step of the tasks discussed in #2862
. The objective is to remove inline styles. Here we are using #ids instead of .classes so our custom attributes have higher specificity. The styles are not saved at all in the post content they are generated by PHP on render time.

The styles are targeted using anchor if the user set one that anchor is used, if not anchor exists, one is automatically generated when needed (and the user is able to change it, but not remove it).
The inline styles remaining in paragraph size and align will be replaced by classes.

This is a WIP in progress when adding the anchor support to paragraph (even just than change) we starting getting tinymce erros in the console, but things work as expected. This changes the markup of paragraph so some blocks may now be invalid some migration/version should be added.

How Has This Been Tested?

Add a paragraph write some text.
Add other paragraphs change text color in one, background color in other and both attributes in another. Verify HTML anchors are automatically added when the first style is chosen. Verify you can change the anchor.
Save the post and view it verify it shows as expected.

Types of changes

Added anchor support to paragraph.
Added a hook to implements automatic addition of anchors when needed.
Added a PHP mechanism to generate the styles in the server.
Remove in style colors from paragraph block saving.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows has proper inline documentation.

@jorgefilipecosta jorgefilipecosta added [Feature] Blocks Overall functionality of blocks [Status] In Progress Tracking issues with work in progress labels Nov 29, 2017
@jorgefilipecosta jorgefilipecosta self-assigned this Nov 29, 2017
@codecov
Copy link

codecov bot commented Nov 29, 2017

Codecov Report

Merging #3722 into master will decrease coverage by 0.55%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3722      +/-   ##
==========================================
- Coverage   38.24%   37.68%   -0.56%     
==========================================
  Files         282      280       -2     
  Lines        6859     6751     -108     
  Branches     1256     1229      -27     
==========================================
- Hits         2623     2544      -79     
+ Misses       3563     3544      -19     
+ Partials      673      663      -10
Impacted Files Coverage Δ
blocks/hooks/anchor.js 80% <ø> (+21.17%) ⬆️
blocks/hooks/index.js 100% <100%> (ø) ⬆️
blocks/library/paragraph/index.js 23.07% <100%> (ø) ⬆️
blocks/hooks/custom-anchor-styles.js 40% <40%> (ø)
blocks/api/serializer.js 94.33% <50%> (-3.67%) ⬇️
blocks/block-controls/index.js 0% <0%> (-100%) ⬇️
components/panel/row.js 0% <0%> (-100%) ⬇️
editor/components/warning/index.js 0% <0%> (-100%) ⬇️
components/panel/color.js 0% <0%> (-100%) ⬇️
blocks/alignment-toolbar/index.js 16.66% <0%> (-83.34%) ⬇️
... and 59 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e619be5...b64945f. Read the comment docs.

@jorgefilipecosta jorgefilipecosta force-pushed the update/remove-inline-styles-take-2 branch 4 times, most recently from bad8e68 to 38dc117 Compare December 1, 2017 18:56
@jorgefilipecosta jorgefilipecosta removed the [Status] In Progress Tracking issues with work in progress label Dec 1, 2017
Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

Here we are using #ids instead of .classes so our custom attributes have higher specificity.

Which specificity are we seeking to defeat, and are we sure ID specificity is enough to do so?

lib/blocks.php Outdated
}
}
if ( '' !== $rules ) {
$styles .= sprintf( "\n#%1\$s { %2\$s\n}", $attributes['anchor'], $rules );
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to escape the anchor?

$rules = '';
foreach ( $map_attrs_styles as $attr_key => $css_property ) {
if ( isset( $attributes[ $attr_key ] ) ) {
$rules .= "\n\t" . esc_attr( sprintf( '%1$s: %2$s;', $css_property, $attributes[ $attr_key ] ) );
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to do the whitespace prettifying here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Without this whitespace prettifying when we check the source code the styles are messy and all on the same line but they work. I'm not certain what we normally do in this cases.

lib/blocks.php Outdated
'textColor' => 'color',
'backgroundColor' => 'background-color',
);
$styles = '';
Copy link
Member

Choose a reason for hiding this comment

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

Minor: If we added a newline before this, we could avoid the awkward = alignment, and more accurately associate the $styles variable with the foreach block where its value is accumulated.


class AddAnchorWhenNeeded extends Component {
componentWillReceiveProps( nextProps ) {
if ( ! nextProps.attributes.anchor && CUSTOM_STYLE_ATTRIBUTES.some( attr => nextProps.attributes[ attr ] ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

Minor: Personally I find the test within the condition to be a bit hard to read (and make the line long), vs. assigning to a separate variable e.g. hasCustomStyleAttribute = ...

Copy link
Member

Choose a reason for hiding this comment

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

Yes, extracting a variable which explains the condition is always a good idea 👍

const CUSTOM_STYLE_ATTRIBUTES = [ 'backgroundColor', 'textColor' ];

class AddAnchorWhenNeeded extends Component {
componentWillReceiveProps( nextProps ) {
Copy link
Member

Choose a reason for hiding this comment

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

Are we sure this is only necessary to check when props change, not also on the initial mount? Thinking a block which is prepopulated with custom styling attributes.

Copy link
Member Author

Choose a reason for hiding this comment

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

If a block is prepopulated with custom styles my expectation is that the anchor is already present so we don't need to add it.
Regarding posts created before this change that's problematic. This change changes the markup e.g: changes inline style attributes so, right now we display the message saying the block is invalid. I have not yet addressed the migration, as soon as #3673 or a similar approach is landed I will use the approach to migrate blocks

@@ -34,9 +34,6 @@ export function addAttribute( settings ) {
settings.attributes = assign( settings.attributes, {
anchor: {
type: 'string',
source: 'attribute',
Copy link
Member

Choose a reason for hiding this comment

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

Why is this attributes definition modified?

Copy link
Member Author

Choose a reason for hiding this comment

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

This change allows the server to have access to anchor attribute which is required to generate the styles in the server.

Copy link
Member

Choose a reason for hiding this comment

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

This change allows the server to have access to anchor attribute which is required to generate the styles in the server.

This will also mean existing blocks with anchors will lose the attribute value after this change (since it was not encoded in the comment delimiters).

Copy link
Member

Choose a reason for hiding this comment

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

I see this was mentioned in #3722 (comment)

/**
* Returns the block's default classname from its name
*
* @param {String} blockName The block name
* @return {string} The block's default class
*/
export function getBlockDefaultClassname( blockName ) {
// Drop common prefixes: 'core/' or 'core-' (in 'core-embed/')
return 'wp-block-' + blockName.replace( /\//, '-' ).replace( /^core-/, '' );
return `wp-block-${ friendlyBlockName( blockName ) }`;
Copy link
Member

Choose a reason for hiding this comment

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

Not sure it's worth concerning about, but there's a decent amount of redundancy between the class name and style applied to a block with custom styles, i.e.:

<p id="wp-block-paragraph-z60dssg" class="wp-block-paragraph">
</p>

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree the naming was redundant, I changed the id's so now they are in format 'paragraph-....'.

@jorgefilipecosta jorgefilipecosta force-pushed the update/remove-inline-styles-take-2 branch 2 times, most recently from e4b742f to 48771ef Compare December 6, 2017 16:48
@jorgefilipecosta
Copy link
Member Author

Hi @aduth thank for your review, some improvements were applied based on the ideas you provided.

Which specificity are we seeking to defeat, and are we sure ID specificity is enough to do so?

The original example was something like "#content p" which as you well noted in a previous talk our approach did not provide enough specificity. I made a change in all blocks with custom styles we add a "has-custom-styles" class. By default we target using .has-custom-styles#blockid. We are using a class and id specify which I think is enough the majority of cases.

The fact we are using an id allows us to target from class/attribute specificity using [id=...blockid] to a hacky multiple id approach e.g: "#blockid#blockid". We will never be able to make sure our default approach for custom styles is enough so I think an option we have right now is provide a default approach that we are comfortable with and is ok for majority cases and then we can provide a way for themes/plugins to change the specificity of our custom styles between a set of allowed options.
I'm not certain if providing a way to set configure specificity is useful or not so I'm open to suggestions.

@aduth
Copy link
Member

aduth commented Dec 11, 2017

We are using a class and id specify which I think is enough the majority of cases.

This would have the same specificity of #content p, meaning it could depend on the order in which the two styles are defined in the page which would take precedence.

@jorgefilipecosta
Copy link
Member Author

This would have the same specificity of #content p, meaning it could depend on the order in which the two styles are defined in the page which would take precedence.

"#content p" has one id and one element selector. Our approach ".has-custom-styles#blockid" has one id and one class selector so I think we have higher specificity against the "#content p" use case (unless I'm missing something).


/**
* Returns a random base 36 string, 36^7 possible strings, maximum seven chars
* According to the birthday paradox repetition occur after sqrt(36^7) = 279936 random strings are generated
Copy link
Member

Choose a reason for hiding this comment

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

If this is random, is it really the case that repetition could only occur after this number of strings has been generated, or is it rather there's a 1 in 279936 chance that a collision could occur? Meaning a post with two blocks could potentially share the same ID (though with a very low chance).

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, @aduth I missed your comment. I changed the comments to be more accurate. The number I wanted to refer is the expected number of ids we have to generate (but I used a wrong formula) to have the first collision. I added the formula to compute the probability of collision given k strings, and the correct number of expected ids we have to generate before having the first collision. Both formulas come from this page https://en.wikipedia.org/wiki/Birthday_attack. A post with two blocks can share the same id but the probability is very very low 1.27*10^-11.

@@ -34,9 +34,6 @@ export function addAttribute( settings ) {
settings.attributes = assign( settings.attributes, {
anchor: {
type: 'string',
source: 'attribute',
Copy link
Member

Choose a reason for hiding this comment

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

I see this was mentioned in #3722 (comment)

}

export default function customAnchorStyles( { addFilter } ) {
addFilter( 'BlockEdit', 'core-anchor-inspector-control', addAnchorWhenStylesNeed );
Copy link
Member

Choose a reason for hiding this comment

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

This implementation would need to be updated per #3827 (not yet merged)

Copy link
Member

Choose a reason for hiding this comment

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

It was merged earlier today :)
Let me push fix.

@aduth
Copy link
Member

aduth commented Dec 11, 2017

"#content p" has one id and one element selector. Our approach ".has-custom-styles#blockid" has one id and one class selector so I think we have higher specificity against the "#content p" use case (unless I'm missing something).

Nope, you're right, I had a brain fart 😄

@gziolo gziolo force-pushed the update/remove-inline-styles-take-2 branch 2 times, most recently from baeb280 to b64945f Compare December 12, 2017 08:34
@gziolo
Copy link
Member

gziolo commented Dec 12, 2017

@jorgefilipecosta I rebased this PR with master and updated code to work with the latest changes around hooks.

}

export function addAnchorWhenStylesNeed( BlockEdit ) {
const WrappedBlockEdit = ( props ) => {
Copy link
Member

Choose a reason for hiding this comment

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

@jorgefilipecosta you might want to combine AddAnchorWhenNeeded and WrappedBlockEdit together and provide only one Component. It looks like AddAnchorWhenNeeded is there only for lifecycle method, so could be also a part of WrappedBlockEdit. This would simplify render method and help keep componentWillReceiveProps focused on logic related to anchor.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for rebasing and updating this PR @gziolo. I tried to simplify things a bit following your advice :)


const hasCustomStyles = ( attributes ) => (
CUSTOM_STYLE_ATTRIBUTES.some( attr => attributes[ attr ] )
);
Copy link
Contributor

Choose a reason for hiding this comment

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

This may be a bit too much magic, especially if we think outside the core block library. Why not something based on explicit support (blockType.supports.customStyles = …, hasBlockSupport)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @mcsf, I added explicit support for custom styles, but this verification still happens, because even if blocks have support for custom styles, this function still needs to execute to check if custom styles are in fact used. I changed the name to hasCustomStyleAttributes.

lib/blocks.php Outdated
*
* @return string CSS style block or empty string
*/
function compute_styles( $blocks ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

do_blocks_styles? gutenberg_generate_blocks_styles?

@@ -15,15 +15,45 @@ import { applyFilters } from '@wordpress/hooks';
*/
import { getBlockType, getUnknownTypeHandlerName } from './registration';

/**
* Returns the block's name with common prefixes: 'core/' or 'core-' (in 'core-embed/') dropped
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd wrap the examples with parens to make it slightly more readable: "Returns the block's name with common prefixes ('core/', 'core-') dropped".

@jorgefilipecosta jorgefilipecosta force-pushed the update/remove-inline-styles-take-2 branch from f427d3c to d4b313e Compare December 13, 2017 22:27
! nextProps.attributes.anchor &&
hasCustomStyleAttributes( nextProps.attributes )
) {
nextProps.setAttributes( {
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain what setAttributes is doing here? In general props should be readonly, but it's hard to find out if this call updates props or not. Given that we check whether nextProps.attributes.anchor is empty, I would assume this code updates this attribute. If I correctly assumed, we might want to introduce internal state here, and combine props and state when passing down to the <BlockEdit { ...this.props } /> in render method.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @gziolo, what we are doing here is verifying if no anchor was set yet ( by the user or automatically). If that's the case we check if attributes needing custom styles were used and if yes we set an automatically generated anchor attribute. I think it's not enough to pass a prop we really want to save the attribute for it to be serialized.

The general question here is what is our extensibility solution to allow to set an attribute depending on other. E.g: If I want to have a plugin that sets red text color attribute if the background is blue what would be the best way to do it? Right now it looks like using our hooks we can achieve this result using this solution, but maybe there is a simpler way.

Copy link
Member

Choose a reason for hiding this comment

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

Okey, setAttributes is some magic functions passed down to edit which does all magic. It must be good 👍

@gziolo
Copy link
Member

gziolo commented Dec 19, 2017

It looks like it needs another rebase and final check from @mtias once he is back.

@jorgefilipecosta jorgefilipecosta force-pushed the update/remove-inline-styles-take-2 branch from d4b313e to 240fd08 Compare December 19, 2017 16:18
@aduth
Copy link
Member

aduth commented Jan 11, 2018

Would it be possible to allow a block to provide their own inline styles to be included in the style tag, rather than just the small whitelisted set included here?

@jorgefilipecosta
Copy link
Member Author

Would it be possible to allow a block to provide their own inline styles to be included in the style tag, rather than just the small whitelisted set included here?

Hi @aduth, I think we can allow that by allowing blocks to provide a PHP styles render function that outputs the custom styles they want, similar to what we have for server-side blocks.

@cyrildc
Copy link

cyrildc commented Feb 15, 2018

Will it merge into master? IMO it would be a great feature to remove inline styles.

@gziolo gziolo deleted the update/remove-inline-styles-take-2 branch May 7, 2018 11:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Blocks Overall functionality of blocks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants