From 13351dd9379a09d1e431836aea999a208ddd6d37 Mon Sep 17 00:00:00 2001 From: Matthew Dapena-Tretter Date: Mon, 31 Mar 2014 21:40:11 -0400 Subject: [PATCH 1/6] Test for correct handling of "download" attribute These tests currently fail as there is no special treatment for this kind of attribute. Related: GH-1337 --- .../__tests__/DOMPropertyOperations-test.js | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/src/browser/ui/dom/__tests__/DOMPropertyOperations-test.js b/src/browser/ui/dom/__tests__/DOMPropertyOperations-test.js index 83e44161d4..8055d17807 100644 --- a/src/browser/ui/dom/__tests__/DOMPropertyOperations-test.js +++ b/src/browser/ui/dom/__tests__/DOMPropertyOperations-test.js @@ -99,6 +99,33 @@ describe('DOMPropertyOperations', function() { )).toBe(''); }); + it('should create markup for minimizable properties', function() { + expect(DOMPropertyOperations.createMarkupForProperty( + 'download', + 'simple' + )).toBe('download="simple"'); + + expect(DOMPropertyOperations.createMarkupForProperty( + 'download', + true + )).toBe('download'); + + expect(DOMPropertyOperations.createMarkupForProperty( + 'download', + 'true' + )).toBe('download="true"'); + + expect(DOMPropertyOperations.createMarkupForProperty( + 'download', + false + )).toBe(''); + + expect(DOMPropertyOperations.createMarkupForProperty( + 'download', + 'false' + )).toBe('download="false"'); + }); + it('should create markup for custom attributes', function() { expect(DOMPropertyOperations.createMarkupForProperty( 'aria-label', From 422a8d9c2ce651f7034d2488e8e744c5e255df56 Mon Sep 17 00:00:00 2001 From: Matthew Dapena-Tretter Date: Mon, 31 Mar 2014 21:43:19 -0400 Subject: [PATCH 2/6] Support minimizable, non-boolean attributes Fixes GH-1337 --- src/browser/ui/dom/DOMProperty.js | 17 +++++++++++++++++ src/browser/ui/dom/DOMPropertyOperations.js | 6 ++++-- src/browser/ui/dom/DefaultDOMPropertyConfig.js | 3 ++- 3 files changed, 23 insertions(+), 3 deletions(-) diff --git a/src/browser/ui/dom/DOMProperty.js b/src/browser/ui/dom/DOMProperty.js index 85487cd799..6009f22347 100644 --- a/src/browser/ui/dom/DOMProperty.js +++ b/src/browser/ui/dom/DOMProperty.js @@ -34,6 +34,7 @@ var DOMPropertyInjection = { HAS_BOOLEAN_VALUE: 0x8, HAS_NUMERIC_VALUE: 0x10, HAS_POSITIVE_NUMERIC_VALUE: 0x20 | 0x10, + CAN_BE_MINIMIZED: 0x40, /** * Inject some specialized knowledge about the DOM. This takes a config object @@ -115,6 +116,8 @@ var DOMPropertyInjection = { propConfig & DOMPropertyInjection.HAS_NUMERIC_VALUE; DOMProperty.hasPositiveNumericValue[propName] = propConfig & DOMPropertyInjection.HAS_POSITIVE_NUMERIC_VALUE; + DOMProperty.canBeMinimized[propName] = + propConfig & DOMPropertyInjection.CAN_BE_MINIMIZED; invariant( !DOMProperty.mustUseAttribute[propName] || @@ -134,6 +137,12 @@ var DOMPropertyInjection = { 'DOMProperty: Cannot have both boolean and numeric value: %s', propName ); + invariant( + !DOMProperty.hasBooleanValue[propName] || + !DOMProperty.canBeMinimized[propName], + 'DOMProperty: Cannot have boolean value and be minimizable: %s', + propName + ); } } }; @@ -231,6 +240,14 @@ var DOMProperty = { */ hasPositiveNumericValue: {}, + /** + * Whether the property can be used as a flag as well as with a value. Removed + * when strictly equal to false; present without a value when strictly equal + * to true; present with a value otherwise. + * @type {Object} + */ + canBeMinimized: {}, + /** * All of the isCustomAttribute() functions that have been injected. */ diff --git a/src/browser/ui/dom/DOMPropertyOperations.js b/src/browser/ui/dom/DOMPropertyOperations.js index cbf2a36c66..8d4081d209 100644 --- a/src/browser/ui/dom/DOMPropertyOperations.js +++ b/src/browser/ui/dom/DOMPropertyOperations.js @@ -29,7 +29,8 @@ function shouldIgnoreValue(name, value) { return value == null || (DOMProperty.hasBooleanValue[name] && !value) || (DOMProperty.hasNumericValue[name] && isNaN(value)) || - (DOMProperty.hasPositiveNumericValue[name] && (value < 1)); + (DOMProperty.hasPositiveNumericValue[name] && (value < 1)) || + (DOMProperty.canBeMinimized[name] && value === false); } var processAttributeNameAndPrefix = memoizeStringOnly(function(name) { @@ -96,7 +97,8 @@ var DOMPropertyOperations = { return ''; } var attributeName = DOMProperty.getAttributeName[name]; - if (DOMProperty.hasBooleanValue[name]) { + if (DOMProperty.hasBooleanValue[name] || + (DOMProperty.canBeMinimized[name] && value === true)) { return escapeTextForBrowser(attributeName); } return processAttributeNameAndPrefix(attributeName) + diff --git a/src/browser/ui/dom/DefaultDOMPropertyConfig.js b/src/browser/ui/dom/DefaultDOMPropertyConfig.js index 88275995fe..865ee63028 100644 --- a/src/browser/ui/dom/DefaultDOMPropertyConfig.js +++ b/src/browser/ui/dom/DefaultDOMPropertyConfig.js @@ -29,6 +29,7 @@ var HAS_SIDE_EFFECTS = DOMProperty.injection.HAS_SIDE_EFFECTS; var HAS_NUMERIC_VALUE = DOMProperty.injection.HAS_NUMERIC_VALUE; var HAS_POSITIVE_NUMERIC_VALUE = DOMProperty.injection.HAS_POSITIVE_NUMERIC_VALUE; +var CAN_BE_MINIMIZED = DOMProperty.injection.CAN_BE_MINIMIZED; var DefaultDOMPropertyConfig = { isCustomAttribute: RegExp.prototype.test.bind( @@ -66,7 +67,7 @@ var DefaultDOMPropertyConfig = { defer: HAS_BOOLEAN_VALUE, dir: null, disabled: MUST_USE_ATTRIBUTE | HAS_BOOLEAN_VALUE, - download: null, + download: CAN_BE_MINIMIZED, draggable: null, encType: null, form: MUST_USE_ATTRIBUTE, From 77ae237be945b6e202016fb420e0bc246e6eed5b Mon Sep 17 00:00:00 2001 From: Matthew Dapena-Tretter Date: Thu, 3 Apr 2014 21:21:38 -0400 Subject: [PATCH 3/6] Rename CAN_BE_MINIMIZED to HAS_BOOLEANISH_VALUE --- src/browser/ui/dom/DOMProperty.js | 12 ++++++------ src/browser/ui/dom/DOMPropertyOperations.js | 4 ++-- src/browser/ui/dom/DefaultDOMPropertyConfig.js | 4 ++-- .../ui/dom/__tests__/DOMPropertyOperations-test.js | 2 +- 4 files changed, 11 insertions(+), 11 deletions(-) diff --git a/src/browser/ui/dom/DOMProperty.js b/src/browser/ui/dom/DOMProperty.js index 6009f22347..6c88b11622 100644 --- a/src/browser/ui/dom/DOMProperty.js +++ b/src/browser/ui/dom/DOMProperty.js @@ -34,7 +34,7 @@ var DOMPropertyInjection = { HAS_BOOLEAN_VALUE: 0x8, HAS_NUMERIC_VALUE: 0x10, HAS_POSITIVE_NUMERIC_VALUE: 0x20 | 0x10, - CAN_BE_MINIMIZED: 0x40, + HAS_BOOLEANISH_VALUE: 0x40, /** * Inject some specialized knowledge about the DOM. This takes a config object @@ -116,8 +116,8 @@ var DOMPropertyInjection = { propConfig & DOMPropertyInjection.HAS_NUMERIC_VALUE; DOMProperty.hasPositiveNumericValue[propName] = propConfig & DOMPropertyInjection.HAS_POSITIVE_NUMERIC_VALUE; - DOMProperty.canBeMinimized[propName] = - propConfig & DOMPropertyInjection.CAN_BE_MINIMIZED; + DOMProperty.hasBooleanishValue[propName] = + propConfig & DOMPropertyInjection.HAS_BOOLEANISH_VALUE; invariant( !DOMProperty.mustUseAttribute[propName] || @@ -139,8 +139,8 @@ var DOMPropertyInjection = { ); invariant( !DOMProperty.hasBooleanValue[propName] || - !DOMProperty.canBeMinimized[propName], - 'DOMProperty: Cannot have boolean value and be minimizable: %s', + !DOMProperty.hasBooleanishValue[propName], + 'DOMProperty: Cannot have boolean and booleanish value: %s', propName ); } @@ -246,7 +246,7 @@ var DOMProperty = { * to true; present with a value otherwise. * @type {Object} */ - canBeMinimized: {}, + hasBooleanishValue: {}, /** * All of the isCustomAttribute() functions that have been injected. diff --git a/src/browser/ui/dom/DOMPropertyOperations.js b/src/browser/ui/dom/DOMPropertyOperations.js index 8d4081d209..b364c81f86 100644 --- a/src/browser/ui/dom/DOMPropertyOperations.js +++ b/src/browser/ui/dom/DOMPropertyOperations.js @@ -30,7 +30,7 @@ function shouldIgnoreValue(name, value) { (DOMProperty.hasBooleanValue[name] && !value) || (DOMProperty.hasNumericValue[name] && isNaN(value)) || (DOMProperty.hasPositiveNumericValue[name] && (value < 1)) || - (DOMProperty.canBeMinimized[name] && value === false); + (DOMProperty.hasBooleanishValue[name] && value === false); } var processAttributeNameAndPrefix = memoizeStringOnly(function(name) { @@ -98,7 +98,7 @@ var DOMPropertyOperations = { } var attributeName = DOMProperty.getAttributeName[name]; if (DOMProperty.hasBooleanValue[name] || - (DOMProperty.canBeMinimized[name] && value === true)) { + (DOMProperty.hasBooleanishValue[name] && value === true)) { return escapeTextForBrowser(attributeName); } return processAttributeNameAndPrefix(attributeName) + diff --git a/src/browser/ui/dom/DefaultDOMPropertyConfig.js b/src/browser/ui/dom/DefaultDOMPropertyConfig.js index 865ee63028..82af1df961 100644 --- a/src/browser/ui/dom/DefaultDOMPropertyConfig.js +++ b/src/browser/ui/dom/DefaultDOMPropertyConfig.js @@ -29,7 +29,7 @@ var HAS_SIDE_EFFECTS = DOMProperty.injection.HAS_SIDE_EFFECTS; var HAS_NUMERIC_VALUE = DOMProperty.injection.HAS_NUMERIC_VALUE; var HAS_POSITIVE_NUMERIC_VALUE = DOMProperty.injection.HAS_POSITIVE_NUMERIC_VALUE; -var CAN_BE_MINIMIZED = DOMProperty.injection.CAN_BE_MINIMIZED; +var HAS_BOOLEANISH_VALUE = DOMProperty.injection.HAS_BOOLEANISH_VALUE; var DefaultDOMPropertyConfig = { isCustomAttribute: RegExp.prototype.test.bind( @@ -67,7 +67,7 @@ var DefaultDOMPropertyConfig = { defer: HAS_BOOLEAN_VALUE, dir: null, disabled: MUST_USE_ATTRIBUTE | HAS_BOOLEAN_VALUE, - download: CAN_BE_MINIMIZED, + download: HAS_BOOLEANISH_VALUE, draggable: null, encType: null, form: MUST_USE_ATTRIBUTE, diff --git a/src/browser/ui/dom/__tests__/DOMPropertyOperations-test.js b/src/browser/ui/dom/__tests__/DOMPropertyOperations-test.js index 8055d17807..d0870716d4 100644 --- a/src/browser/ui/dom/__tests__/DOMPropertyOperations-test.js +++ b/src/browser/ui/dom/__tests__/DOMPropertyOperations-test.js @@ -99,7 +99,7 @@ describe('DOMPropertyOperations', function() { )).toBe(''); }); - it('should create markup for minimizable properties', function() { + it('should create markup for booleanish properties', function() { expect(DOMPropertyOperations.createMarkupForProperty( 'download', 'simple' From 4b71cf2efe1d476f6d2217c7d7c9a17648eefda7 Mon Sep 17 00:00:00 2001 From: Matthew Dapena-Tretter Date: Thu, 3 Apr 2014 22:12:50 -0400 Subject: [PATCH 4/6] Combine valid value checks --- src/browser/ui/dom/DOMProperty.js | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/src/browser/ui/dom/DOMProperty.js b/src/browser/ui/dom/DOMProperty.js index 6c88b11622..bd8f507c9f 100644 --- a/src/browser/ui/dom/DOMProperty.js +++ b/src/browser/ui/dom/DOMProperty.js @@ -132,15 +132,11 @@ var DOMPropertyInjection = { propName ); invariant( - !DOMProperty.hasBooleanValue[propName] || - !DOMProperty.hasNumericValue[propName], - 'DOMProperty: Cannot have both boolean and numeric value: %s', - propName - ); - invariant( - !DOMProperty.hasBooleanValue[propName] || - !DOMProperty.hasBooleanishValue[propName], - 'DOMProperty: Cannot have boolean and booleanish value: %s', + !!DOMProperty.hasBooleanValue[propName] + + !!DOMProperty.hasNumericValue[propName] + + !!DOMProperty.hasBooleanishValue[propName] <= 1, + 'DOMProperty: Value can be one of boolean, booleanish, or numeric ' + + 'value, but not a combination: %s', propName ); } From 1c63a3a7f4c2f6be9b10ac870211032eea6ded1e Mon Sep 17 00:00:00 2001 From: Matthew Dapena-Tretter Date: Thu, 3 Apr 2014 22:20:19 -0400 Subject: [PATCH 5/6] Test more falsey values --- .../dom/__tests__/DOMPropertyOperations-test.js | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/browser/ui/dom/__tests__/DOMPropertyOperations-test.js b/src/browser/ui/dom/__tests__/DOMPropertyOperations-test.js index d0870716d4..47a18f8822 100644 --- a/src/browser/ui/dom/__tests__/DOMPropertyOperations-test.js +++ b/src/browser/ui/dom/__tests__/DOMPropertyOperations-test.js @@ -124,6 +124,21 @@ describe('DOMPropertyOperations', function() { 'download', 'false' )).toBe('download="false"'); + + expect(DOMPropertyOperations.createMarkupForProperty( + 'download', + undefined + )).toBe(''); + + expect(DOMPropertyOperations.createMarkupForProperty( + 'download', + null + )).toBe(''); + + expect(DOMPropertyOperations.createMarkupForProperty( + 'download', + 0 + )).toBe('download="0"'); }); it('should create markup for custom attributes', function() { From 32a7a1cedbe695a0d22cfd6f17ee58ec4aeee3a6 Mon Sep 17 00:00:00 2001 From: Matthew Dapena-Tretter Date: Mon, 14 Apr 2014 23:00:25 -0400 Subject: [PATCH 6/6] Rename "booleanish" to "overloaded boolean" --- src/browser/ui/dom/DOMProperty.js | 14 +++++++------- src/browser/ui/dom/DOMPropertyOperations.js | 4 ++-- src/browser/ui/dom/DefaultDOMPropertyConfig.js | 5 +++-- 3 files changed, 12 insertions(+), 11 deletions(-) diff --git a/src/browser/ui/dom/DOMProperty.js b/src/browser/ui/dom/DOMProperty.js index bd8f507c9f..8d3d6626d0 100644 --- a/src/browser/ui/dom/DOMProperty.js +++ b/src/browser/ui/dom/DOMProperty.js @@ -34,7 +34,7 @@ var DOMPropertyInjection = { HAS_BOOLEAN_VALUE: 0x8, HAS_NUMERIC_VALUE: 0x10, HAS_POSITIVE_NUMERIC_VALUE: 0x20 | 0x10, - HAS_BOOLEANISH_VALUE: 0x40, + HAS_OVERLOADED_BOOLEAN_VALUE: 0x40, /** * Inject some specialized knowledge about the DOM. This takes a config object @@ -116,8 +116,8 @@ var DOMPropertyInjection = { propConfig & DOMPropertyInjection.HAS_NUMERIC_VALUE; DOMProperty.hasPositiveNumericValue[propName] = propConfig & DOMPropertyInjection.HAS_POSITIVE_NUMERIC_VALUE; - DOMProperty.hasBooleanishValue[propName] = - propConfig & DOMPropertyInjection.HAS_BOOLEANISH_VALUE; + DOMProperty.hasOverloadedBooleanValue[propName] = + propConfig & DOMPropertyInjection.HAS_OVERLOADED_BOOLEAN_VALUE; invariant( !DOMProperty.mustUseAttribute[propName] || @@ -134,9 +134,9 @@ var DOMPropertyInjection = { invariant( !!DOMProperty.hasBooleanValue[propName] + !!DOMProperty.hasNumericValue[propName] + - !!DOMProperty.hasBooleanishValue[propName] <= 1, - 'DOMProperty: Value can be one of boolean, booleanish, or numeric ' + - 'value, but not a combination: %s', + !!DOMProperty.hasOverloadedBooleanValue[propName] <= 1, + 'DOMProperty: Value can be one of boolean, overloaded boolean, or ' + + 'numeric value, but not a combination: %s', propName ); } @@ -242,7 +242,7 @@ var DOMProperty = { * to true; present with a value otherwise. * @type {Object} */ - hasBooleanishValue: {}, + hasOverloadedBooleanValue: {}, /** * All of the isCustomAttribute() functions that have been injected. diff --git a/src/browser/ui/dom/DOMPropertyOperations.js b/src/browser/ui/dom/DOMPropertyOperations.js index b364c81f86..b7b07d686a 100644 --- a/src/browser/ui/dom/DOMPropertyOperations.js +++ b/src/browser/ui/dom/DOMPropertyOperations.js @@ -30,7 +30,7 @@ function shouldIgnoreValue(name, value) { (DOMProperty.hasBooleanValue[name] && !value) || (DOMProperty.hasNumericValue[name] && isNaN(value)) || (DOMProperty.hasPositiveNumericValue[name] && (value < 1)) || - (DOMProperty.hasBooleanishValue[name] && value === false); + (DOMProperty.hasOverloadedBooleanValue[name] && value === false); } var processAttributeNameAndPrefix = memoizeStringOnly(function(name) { @@ -98,7 +98,7 @@ var DOMPropertyOperations = { } var attributeName = DOMProperty.getAttributeName[name]; if (DOMProperty.hasBooleanValue[name] || - (DOMProperty.hasBooleanishValue[name] && value === true)) { + (DOMProperty.hasOverloadedBooleanValue[name] && value === true)) { return escapeTextForBrowser(attributeName); } return processAttributeNameAndPrefix(attributeName) + diff --git a/src/browser/ui/dom/DefaultDOMPropertyConfig.js b/src/browser/ui/dom/DefaultDOMPropertyConfig.js index 82af1df961..4126589758 100644 --- a/src/browser/ui/dom/DefaultDOMPropertyConfig.js +++ b/src/browser/ui/dom/DefaultDOMPropertyConfig.js @@ -29,7 +29,8 @@ var HAS_SIDE_EFFECTS = DOMProperty.injection.HAS_SIDE_EFFECTS; var HAS_NUMERIC_VALUE = DOMProperty.injection.HAS_NUMERIC_VALUE; var HAS_POSITIVE_NUMERIC_VALUE = DOMProperty.injection.HAS_POSITIVE_NUMERIC_VALUE; -var HAS_BOOLEANISH_VALUE = DOMProperty.injection.HAS_BOOLEANISH_VALUE; +var HAS_OVERLOADED_BOOLEAN_VALUE = + DOMProperty.injection.HAS_OVERLOADED_BOOLEAN_VALUE; var DefaultDOMPropertyConfig = { isCustomAttribute: RegExp.prototype.test.bind( @@ -67,7 +68,7 @@ var DefaultDOMPropertyConfig = { defer: HAS_BOOLEAN_VALUE, dir: null, disabled: MUST_USE_ATTRIBUTE | HAS_BOOLEAN_VALUE, - download: HAS_BOOLEANISH_VALUE, + download: HAS_OVERLOADED_BOOLEAN_VALUE, draggable: null, encType: null, form: MUST_USE_ATTRIBUTE,