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

Parser: Fix boolean attribute explicit string value result #7610

Merged
merged 2 commits into from Jun 28, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
34 changes: 24 additions & 10 deletions blocks/api/parser.js
Expand Up @@ -2,7 +2,7 @@
* External dependencies
*/
import { parse as hpqParse } from 'hpq';
import { castArray, mapValues, omit } from 'lodash';
import { flow, castArray, mapValues, omit } from 'lodash';

/**
* WordPress dependencies
Expand All @@ -19,6 +19,22 @@ import { isValidBlock } from './validation';
import { getCommentDelimitedContent } from './serializer';
import { attr, prop, html, text, query, node, children } from './matchers';

/**
* Higher-order hpq matcher which enhances an attribute matcher to return true
Copy link
Member

Choose a reason for hiding this comment

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

Nice docs 👍

* or false depending on whether the original matcher returns undefined. This
* is useful for boolean attributes (e.g. disabled) whose attribute values may
* be technically falsey (empty string), though their mere presence should be
* enough to infer as a truthy value.
*
* @param {Function} matcher Original hpq matcher.
*
* @return {Function} Enhanced hpq matcher.``
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 what the backticks at the end are for 🤷‍♂️

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure what the backticks at the end are for 🤷‍♂️

Fat fingered 😄

Copy link
Member

Choose a reason for hiding this comment

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

That was my guess 😆

*/
export const toBooleanAttributeMatcher = ( matcher ) => flow( [
matcher,
( value ) => value !== undefined,
Copy link
Member

Choose a reason for hiding this comment

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

I almost asked "what about a false value?" but that should never happen. Not sure if it's worth adding docs just to remind the reader about that.

] );

/**
* Returns value coerced to the specified JSON schema type string.
*
Expand Down Expand Up @@ -68,7 +84,12 @@ export function asType( value, type ) {
export function matcherFromSource( sourceConfig ) {
switch ( sourceConfig.source ) {
case 'attribute':
return attr( sourceConfig.selector, sourceConfig.attribute );
let matcher = attr( sourceConfig.selector, sourceConfig.attribute );
if ( sourceConfig.type === 'boolean' ) {
matcher = toBooleanAttributeMatcher( matcher );
}

return matcher;
case 'property':
return prop( sourceConfig.selector, sourceConfig.property );
case 'html':
Expand Down Expand Up @@ -98,14 +119,7 @@ export function matcherFromSource( sourceConfig ) {
* @return {*} Attribute value.
*/
export function parseWithAttributeSchema( innerHTML, attributeSchema ) {
const attributeValue = hpqParse( innerHTML, matcherFromSource( attributeSchema ) );
// HTML attributes without a defined value (e.g. <audio loop>) are parsed
// to a value of '' (empty string), so return `true` if we know this should
// be boolean.
if ( 'attribute' === attributeSchema.source && 'boolean' === attributeSchema.type ) {
return '' === attributeValue;
}
return attributeValue;
return hpqParse( innerHTML, matcherFromSource( attributeSchema ) );
}

/**
Expand Down
55 changes: 55 additions & 0 deletions blocks/api/test/parser.js
@@ -1,3 +1,8 @@
/**
* External dependencies
*/
import { attr } from 'hpq';

/**
* Internal dependencies
*/
Expand All @@ -9,6 +14,7 @@ import {
getAttributesAndInnerBlocksFromDeprecatedVersion,
default as parse,
parseWithAttributeSchema,
toBooleanAttributeMatcher,
} from '../parser';
import {
registerBlockType,
Expand Down Expand Up @@ -55,6 +61,42 @@ describe( 'block parser', () => {
} );
} );

describe( 'toBooleanAttributeMatcher()', () => {
const originalMatcher = attr( 'disabled' );
const enhancedMatcher = toBooleanAttributeMatcher( originalMatcher );

it( 'should return a matcher returning false on unset attribute', () => {
const node = document.createElement( 'input' );

expect( originalMatcher( node ) ).toBe( undefined );
expect( enhancedMatcher( node ) ).toBe( false );
} );

it( 'should return a matcher returning true on implicit empty string attribute value', () => {
const node = document.createElement( 'input' );
node.disabled = true;

expect( originalMatcher( node ) ).toBe( '' );
expect( enhancedMatcher( node ) ).toBe( true );
} );

it( 'should return a matcher returning true on explicit empty string attribute value', () => {
const node = document.createElement( 'input' );
node.setAttribute( 'disabled', '' );

expect( originalMatcher( node ) ).toBe( '' );
expect( enhancedMatcher( node ) ).toBe( true );
} );

it( 'should return a matcher returning true on explicit string attribute value', () => {
const node = document.createElement( 'input' );
node.setAttribute( 'disabled', 'disabled' );

expect( originalMatcher( node ) ).toBe( 'disabled' );
expect( enhancedMatcher( node ) ).toBe( true );
} );
} );

describe( 'asType()', () => {
it( 'gracefully handles undefined type', () => {
expect( asType( 5 ) ).toBe( 5 );
Expand Down Expand Up @@ -132,6 +174,19 @@ describe( 'block parser', () => {
expect( value ).toBe( true );
} );

it( 'should return the matcher\'s true boolean attribute value on explicit attribute value', () => {
Copy link
Member

Choose a reason for hiding this comment

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

This is definitely a case where I'm like #7366 for the win 😉

const value = parseWithAttributeSchema(
'<audio src="#" loop="loop">',
{
type: 'boolean',
source: 'attribute',
selector: 'audio',
attribute: 'loop',
},
);
expect( value ).toBe( true );
} );

it( 'should return the matcher\'s false boolean attribute value', () => {
const value = parseWithAttributeSchema(
'<audio src="#" autoplay>',
Expand Down