From 6ddfed6e20a7ec0f62f08f5197672db0ef6d9b31 Mon Sep 17 00:00:00 2001 From: Naina Raisinghani Date: Tue, 20 Feb 2018 13:56:43 +1100 Subject: [PATCH 01/11] WIP --- examples/selector.amp.html | 103 ++++++++++++++++++ extensions/amp-selector/0.1/amp-selector.js | 41 +++++++ .../0.1/test/test-amp-selector.js | 33 ++++++ spec/amp-actions-and-events.md | 16 +++ 4 files changed, 193 insertions(+) create mode 100644 examples/selector.amp.html diff --git a/examples/selector.amp.html b/examples/selector.amp.html new file mode 100644 index 000000000000..bc9f59f2f047 --- /dev/null +++ b/examples/selector.amp.html @@ -0,0 +1,103 @@ + + + + + + + + + + + + + + + +
+ + +
    +
  • +
  • +
  • Poo
  • +
  • Poop
  • +
  • Poo poo
  • +
  • None of the Above
  • +
+
+ +
+ + + + + + +
+ + +
    +
  • +
  • +
  • None of the Above
  • +
+
+ + + + + + + + + + + + + + + + + + +
+ + + + + + + + + + + + + + +
+ +
+
+ + +
+ + +
+ +
+
+
+
Non-live-list option
+
+ + diff --git a/extensions/amp-selector/0.1/amp-selector.js b/extensions/amp-selector/0.1/amp-selector.js index 0701a6c26350..ae2a43dcf0b2 100644 --- a/extensions/amp-selector/0.1/amp-selector.js +++ b/extensions/amp-selector/0.1/amp-selector.js @@ -113,6 +113,24 @@ export class AmpSelector extends AMP.BaseElement { this.element.addEventListener('click', this.clickHandler_.bind(this)); this.element.addEventListener('keydown', this.keyDownHandler_.bind(this)); } + + this.registerAction('selectUp', invocation => { + const args = invocation.args; + if (args && args['incrementPos'] !== undefined) { + this.select_(-1 * args['incrementPos']); + } else { + this.select_(-1); + } + }, ActionTrust.LOW); + + this.registerAction('selectDown', invocation => { + const args = invocation.args; + if (args && args['decrementPos'] !== undefined) { + this.select_(args['decrementPos']); + } else { + this.select_(1); + } + }, ActionTrust.LOW); } /** @override */ @@ -319,6 +337,29 @@ export class AmpSelector extends AMP.BaseElement { } } + /** + * Handles selectUp events. + * @param {!integer} incrementPos + */ + select_(incrementPos) { + + if (this.isMultiple_) { + this.clearSelection_(this.selectedOptions_[0]); + } + // Change the selection to the next element in the specified direction. + // The selection should loop around if the user attempts to go one + // past the beginning or end. + let selectedIndex_ = this.options_.indexOf(this.selectedOptions_[0]); + + selectedIndex_ = (selectedIndex_ + incrementPos) % this.options_.length; + if (selectedIndex_ < 0) { + selectedIndex_ = selectedIndex_ + this.options_.length; + } + + const selectedOption = this.options_[selectedIndex_]; + this.onOptionPicked_(selectedOption); + } + /** * Handles keyboard events. * @param {!Event} event diff --git a/extensions/amp-selector/0.1/test/test-amp-selector.js b/extensions/amp-selector/0.1/test/test-amp-selector.js index 1344cd4768ac..b3f0f519f49c 100644 --- a/extensions/amp-selector/0.1/test/test-amp-selector.js +++ b/extensions/amp-selector/0.1/test/test-amp-selector.js @@ -651,6 +651,39 @@ describes.realWin('amp-selector', { ampSelector, 'select', /* CustomEvent */ eventMatcher); }); + it('should trigger `select` action when user uses ' + + '`selectUp`/`selectDown` action with default skip value of 1', () => { + const ampSelector = getSelector({ + attributes: { + id: 'ampSelector', + }, + config: { + count: 6, + }, + }); + ampSelector.children[0].setAttribute('selected', ''); + ampSelector.build(); + + expect(ampSelector.hasAttribute('multiple')).to.be.false; + expect(ampSelector.children[0].hasAttribute('selected')).to.be.true; + + const button_down = win.document.createElement('button_down'); + button_down.setAttribute('on', 'tap:ampSelector.selectDown()'); + win.document.body.appendChild(button_down); + button_down.click(); + + // expect(ampSelector.children[0].hasAttribute('selected')).to.be.false; + // expect(ampSelector.children[1].hasAttribute('selected')).to.be.true; + + const button_up = win.document.createElement('button_up'); + button_up.setAttribute('on', 'tap:ampSelector.selectUp()'); + win.document.body.appendChild(button_up); + button_up.click(); + + // expect(ampSelector.children[1].hasAttribute('selected')).to.be.false; + // expect(ampSelector.children[0].hasAttribute('selected')).to.be.true; + }); + describe('keyboard-select-mode', () => { it('should have `none` mode by default', () => { diff --git a/spec/amp-actions-and-events.md b/spec/amp-actions-and-events.md index 1862c51cfc4f..6d5ae48457f6 100644 --- a/spec/amp-actions-and-events.md +++ b/spec/amp-actions-and-events.md @@ -362,6 +362,22 @@ event.response +### amp-selector + + + + + + + + + + + + + +
ActionDescription
selectUp(incrementPos=INTEGER)Changes the selected element to the currentPos-incrementPos. If no increment value is specified, the default is 1.
selectDown(decrementPos=INTEGER)Changes the selected element to the currentPos+decrementPos. If no decrement value is specified, the default is -1.
+ ### amp-sidebar From 524a18af185c04e36fb9634b9cb2b410ec11dd19 Mon Sep 17 00:00:00 2001 From: Naina Raisinghani Date: Tue, 20 Feb 2018 17:44:12 +1100 Subject: [PATCH 02/11] Finalize implementation and tests --- examples/selector.amp.html | 103 ------------------ extensions/amp-selector/0.1/amp-selector.js | 10 +- .../0.1/test/test-amp-selector.js | 49 +++++++-- 3 files changed, 41 insertions(+), 121 deletions(-) delete mode 100644 examples/selector.amp.html diff --git a/examples/selector.amp.html b/examples/selector.amp.html deleted file mode 100644 index bc9f59f2f047..000000000000 --- a/examples/selector.amp.html +++ /dev/null @@ -1,103 +0,0 @@ - - - - - - - - - - - - - - - -
- - -
    -
  • -
  • -
  • Poo
  • -
  • Poop
  • -
  • Poo poo
  • -
  • None of the Above
  • -
-
- - - - - - - - -
- - -
    -
  • -
  • -
  • None of the Above
  • -
-
- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
- -
-
- - -
- - -
- -
-
-
-
Non-live-list option
-
- - diff --git a/extensions/amp-selector/0.1/amp-selector.js b/extensions/amp-selector/0.1/amp-selector.js index ae2a43dcf0b2..3878485267da 100644 --- a/extensions/amp-selector/0.1/amp-selector.js +++ b/extensions/amp-selector/0.1/amp-selector.js @@ -339,17 +339,14 @@ export class AmpSelector extends AMP.BaseElement { /** * Handles selectUp events. - * @param {!integer} incrementPos + * @param {number} incrementPos */ select_(incrementPos) { - - if (this.isMultiple_) { - this.clearSelection_(this.selectedOptions_[0]); - } // Change the selection to the next element in the specified direction. // The selection should loop around if the user attempts to go one // past the beginning or end. let selectedIndex_ = this.options_.indexOf(this.selectedOptions_[0]); + const oldSelectedIndex_ = selectedIndex_; selectedIndex_ = (selectedIndex_ + incrementPos) % this.options_.length; if (selectedIndex_ < 0) { @@ -357,7 +354,8 @@ export class AmpSelector extends AMP.BaseElement { } const selectedOption = this.options_[selectedIndex_]; - this.onOptionPicked_(selectedOption); + this.setSelection_(selectedOption); + this.clearSelection_(this.options_[oldSelectedIndex_]); } /** diff --git a/extensions/amp-selector/0.1/test/test-amp-selector.js b/extensions/amp-selector/0.1/test/test-amp-selector.js index b3f0f519f49c..e9eb806782c0 100644 --- a/extensions/amp-selector/0.1/test/test-amp-selector.js +++ b/extensions/amp-selector/0.1/test/test-amp-selector.js @@ -663,25 +663,50 @@ describes.realWin('amp-selector', { }); ampSelector.children[0].setAttribute('selected', ''); ampSelector.build(); + const impl = ampSelector.implementation_; expect(ampSelector.hasAttribute('multiple')).to.be.false; expect(ampSelector.children[0].hasAttribute('selected')).to.be.true; - const button_down = win.document.createElement('button_down'); - button_down.setAttribute('on', 'tap:ampSelector.selectDown()'); - win.document.body.appendChild(button_down); - button_down.click(); + impl.executeAction({method: 'selectDown', satisfiesTrust: () => true}); + expect(ampSelector.children[0].hasAttribute('selected')).to.be.false; + expect(ampSelector.children[1].hasAttribute('selected')).to.be.true; + + impl.executeAction({method: 'selectUp', satisfiesTrust: () => true}); + + expect(ampSelector.children[1].hasAttribute('selected')).to.be.false; + expect(ampSelector.children[0].hasAttribute('selected')).to.be.true; + + }); + + it('should trigger `select` action when user uses ' + + '`selectUp`/`selectDown` action with user specified skip value', () => { + const ampSelector = getSelector({ + attributes: { + id: 'ampSelector', + }, + config: { + count: 6, + }, + }); + ampSelector.children[0].setAttribute('selected', ''); + ampSelector.build(); + const impl = ampSelector.implementation_; - // expect(ampSelector.children[0].hasAttribute('selected')).to.be.false; - // expect(ampSelector.children[1].hasAttribute('selected')).to.be.true; + expect(ampSelector.hasAttribute('multiple')).to.be.false; + expect(ampSelector.children[0].hasAttribute('selected')).to.be.true; - const button_up = win.document.createElement('button_up'); - button_up.setAttribute('on', 'tap:ampSelector.selectUp()'); - win.document.body.appendChild(button_up); - button_up.click(); + let args = {'incrementPos': 2}; + impl.executeAction( + {method: 'selectDown', args, satisfiesTrust: () => true}); + expect(ampSelector.children[0].hasAttribute('selected')).to.be.false; + expect(ampSelector.children[1].hasAttribute('selected')).to.be.true; - // expect(ampSelector.children[1].hasAttribute('selected')).to.be.false; - // expect(ampSelector.children[0].hasAttribute('selected')).to.be.true; + args = {'decrementPos': 2}; + impl.executeAction( + {method: 'selectUp', args, satisfiesTrust: () => true}); + expect(ampSelector.children[1].hasAttribute('selected')).to.be.false; + expect(ampSelector.children[0].hasAttribute('selected')).to.be.true; }); describe('keyboard-select-mode', () => { From bf28c8dedc1a88551a0b315360e6a4ec08b2a379 Mon Sep 17 00:00:00 2001 From: Naina Raisinghani Date: Tue, 20 Feb 2018 20:22:29 +1100 Subject: [PATCH 03/11] Implementation and tests --- extensions/amp-selector/0.1/amp-selector.js | 46 +++++++++++ .../0.1/test/test-amp-selector.js | 79 +++++++++++++++++++ spec/amp-actions-and-events.md | 4 + 3 files changed, 129 insertions(+) diff --git a/extensions/amp-selector/0.1/amp-selector.js b/extensions/amp-selector/0.1/amp-selector.js index 3878485267da..8d0574e59b82 100644 --- a/extensions/amp-selector/0.1/amp-selector.js +++ b/extensions/amp-selector/0.1/amp-selector.js @@ -131,6 +131,18 @@ export class AmpSelector extends AMP.BaseElement { this.select_(1); } }, ActionTrust.LOW); + + this.registerAction('toggle', invocation => { + const args = invocation.args; + if (args && args['index'] !== undefined) { + if (args['value'] !== undefined) { + this.toggle_(args['index'], args['value']); + } else { + this.toggle_(args['index'], + !this.options_[args['index']].hasAttribute('selected')); + } + } + }, ActionTrust.LOW); } /** @override */ @@ -337,6 +349,40 @@ export class AmpSelector extends AMP.BaseElement { } } + /** + * Handles toggle action. + * @param {number} index + * @param {boolean} value + */ + toggle_(index, value) { + // Change the selection to the next element in the specified direction. + // The selection should loop around if the user attempts to go one + // past the beginning or end. + const selectedIndex_ = this.options_.indexOf(this.selectedOptions_[0]); + + if (value == true) { + // If we are toggling the `selected` attribute to true: + // If selectedIndex_ == index then there is no change in the status. + + // If selectedIndex_ != index then the current selected element needs + // have the `selected` attribute removed and the element at position + // index needs to be selected. + if (selectedIndex_ != index) { + this.setSelection_(this.options_[index]); + this.clearSelection_(this.options_[selectedIndex_]); + } + } else { + // If we are toggling the `selected` attribute to false: + // If selectedIndex_ == index then the current selected element needs + // to have the `selected` attribute removed. + // If selectedIndex_ != index, then the current `selected` element + // can retain it's status but the element at the given index needs + // to have the `selected` attribute removed. + this.clearSelection_(this.options_[index]); + } + } + + /** * Handles selectUp events. * @param {number} incrementPos diff --git a/extensions/amp-selector/0.1/test/test-amp-selector.js b/extensions/amp-selector/0.1/test/test-amp-selector.js index e9eb806782c0..927292e9b893 100644 --- a/extensions/amp-selector/0.1/test/test-amp-selector.js +++ b/extensions/amp-selector/0.1/test/test-amp-selector.js @@ -709,6 +709,85 @@ describes.realWin('amp-selector', { expect(ampSelector.children[0].hasAttribute('selected')).to.be.true; }); + it('should trigger `toggle` action when no \'value\'' + + ' argument is specified', () => { + const ampSelector = getSelector({ + attributes: { + id: 'ampSelector', + }, + config: { + count: 6, + }, + }); + ampSelector.children[2].setAttribute('selected', ''); + ampSelector.build(); + const impl = ampSelector.implementation_; + + expect(ampSelector.hasAttribute('multiple')).to.be.false; + expect(ampSelector.children[2].hasAttribute('selected')).to.be.true; + + // test that if no 'value' argument is provided the 'selected' attribute + // is reversed. + const args = {'index': 4}; + impl.executeAction( + {method: 'toggle', args, satisfiesTrust: () => true}); + expect(ampSelector.children[2].hasAttribute('selected')).to.be.false; + expect(ampSelector.children[4].hasAttribute('selected')).to.be.true; + + impl.executeAction( + {method: 'toggle', args, satisfiesTrust: () => true}); + expect(ampSelector.children[4].hasAttribute('selected')).to.be.false; + }); + + it('should trigger `toggle` action when \'value\'' + + ' argument is specified', () => { + const ampSelector = getSelector({ + attributes: { + id: 'ampSelector', + }, + config: { + count: 6, + }, + }); + ampSelector.children[2].setAttribute('selected', ''); + ampSelector.build(); + const impl = ampSelector.implementation_; + + expect(ampSelector.hasAttribute('multiple')).to.be.false; + expect(ampSelector.children[2].hasAttribute('selected')).to.be.true; + + // if 'index' is set to the currently selected element and + // 'value' is true + let args = {'index': 2, 'value': true}; + impl.executeAction( + {method: 'toggle', args, satisfiesTrust: () => true}); + expect(ampSelector.children[2].hasAttribute('selected')).to.be.true; + + // if 'index' is different from the selected element and + // 'value' is true + args = {'index': 4, 'value': true}; + impl.executeAction( + {method: 'toggle', args, satisfiesTrust: () => true}); + expect(ampSelector.children[2].hasAttribute('selected')).to.be.false; + expect(ampSelector.children[4].hasAttribute('selected')).to.be.true; + + // if 'index' is set to the currently selected element and + // 'value' is false + args = {'index': 4, 'value': false}; + impl.executeAction( + {method: 'toggle', args, satisfiesTrust: () => true}); + expect(ampSelector.children[4].hasAttribute('selected')).to.be.false; + + // if 'index' is different from the selected element and + // 'value' is false + ampSelector.children[2].setAttribute('selected', ''); + args = {'index': 4, 'value': false}; + impl.executeAction( + {method: 'toggle', args, satisfiesTrust: () => true}); + expect(ampSelector.children[4].hasAttribute('selected')).to.be.false; + expect(ampSelector.children[2].hasAttribute('selected')).to.be.true; + }); + describe('keyboard-select-mode', () => { it('should have `none` mode by default', () => { diff --git a/spec/amp-actions-and-events.md b/spec/amp-actions-and-events.md index 6d5ae48457f6..3686f6b433ca 100644 --- a/spec/amp-actions-and-events.md +++ b/spec/amp-actions-and-events.md @@ -376,6 +376,10 @@ event.response
+ + + +
selectDown(decrementPos=INTEGER) Changes the selected element to the currentPos+decrementPos. If no decrement value is specified, the default is -1.
toggle(index=INTEGER, value=BOOLEAN)Sets the selected element's `selected` attribute if value is 'true', otherwise removes the attribute
### amp-sidebar From 2d3bd60a58456a47e256c565ab4955f246180b93 Mon Sep 17 00:00:00 2001 From: Naina Raisinghani Date: Wed, 21 Feb 2018 11:19:14 +1100 Subject: [PATCH 04/11] Make cvializ@ suggestions - change input argument name and clean up code --- extensions/amp-selector/0.1/amp-selector.js | 33 +++++++------------ .../0.1/test/test-amp-selector.js | 12 +++---- spec/amp-actions-and-events.md | 6 ++-- 3 files changed, 20 insertions(+), 31 deletions(-) diff --git a/extensions/amp-selector/0.1/amp-selector.js b/extensions/amp-selector/0.1/amp-selector.js index 3878485267da..cedd8c16f2ec 100644 --- a/extensions/amp-selector/0.1/amp-selector.js +++ b/extensions/amp-selector/0.1/amp-selector.js @@ -116,20 +116,14 @@ export class AmpSelector extends AMP.BaseElement { this.registerAction('selectUp', invocation => { const args = invocation.args; - if (args && args['incrementPos'] !== undefined) { - this.select_(-1 * args['incrementPos']); - } else { - this.select_(-1); - } + const delta = (args && args['delta'] !== undefined) ? -args['delta'] : -1; + this.select_(delta); }, ActionTrust.LOW); this.registerAction('selectDown', invocation => { const args = invocation.args; - if (args && args['decrementPos'] !== undefined) { - this.select_(args['decrementPos']); - } else { - this.select_(1); - } + const delta = (args && args['delta'] !== undefined) ? args['delta'] : 1; + this.select_(delta); }, ActionTrust.LOW); } @@ -339,23 +333,18 @@ export class AmpSelector extends AMP.BaseElement { /** * Handles selectUp events. - * @param {number} incrementPos + * @param {number} delta */ - select_(incrementPos) { + select_(delta) { // Change the selection to the next element in the specified direction. // The selection should loop around if the user attempts to go one // past the beginning or end. - let selectedIndex_ = this.options_.indexOf(this.selectedOptions_[0]); - const oldSelectedIndex_ = selectedIndex_; - - selectedIndex_ = (selectedIndex_ + incrementPos) % this.options_.length; - if (selectedIndex_ < 0) { - selectedIndex_ = selectedIndex_ + this.options_.length; - } + const previousIndex = this.options_.indexOf(this.selectedOptions_[0]); + const index = previousIndex + delta; + const normalizedIndex = index % this.options_.length; - const selectedOption = this.options_[selectedIndex_]; - this.setSelection_(selectedOption); - this.clearSelection_(this.options_[oldSelectedIndex_]); + this.setSelection_(this.options_[normalizedIndex]); + this.clearSelection_(this.options_[previousIndex]); } /** diff --git a/extensions/amp-selector/0.1/test/test-amp-selector.js b/extensions/amp-selector/0.1/test/test-amp-selector.js index e9eb806782c0..ff2b79d7a33d 100644 --- a/extensions/amp-selector/0.1/test/test-amp-selector.js +++ b/extensions/amp-selector/0.1/test/test-amp-selector.js @@ -652,7 +652,7 @@ describes.realWin('amp-selector', { }); it('should trigger `select` action when user uses ' + - '`selectUp`/`selectDown` action with default skip value of 1', () => { + '`selectUp`/`selectDown` action with default delta value of 1', () => { const ampSelector = getSelector({ attributes: { id: 'ampSelector', @@ -680,7 +680,7 @@ describes.realWin('amp-selector', { }); it('should trigger `select` action when user uses ' + - '`selectUp`/`selectDown` action with user specified skip value', () => { + '`selectUp`/`selectDown` action with user specified delta value', () => { const ampSelector = getSelector({ attributes: { id: 'ampSelector', @@ -696,16 +696,16 @@ describes.realWin('amp-selector', { expect(ampSelector.hasAttribute('multiple')).to.be.false; expect(ampSelector.children[0].hasAttribute('selected')).to.be.true; - let args = {'incrementPos': 2}; + let args = {'delta': 2}; impl.executeAction( {method: 'selectDown', args, satisfiesTrust: () => true}); expect(ampSelector.children[0].hasAttribute('selected')).to.be.false; - expect(ampSelector.children[1].hasAttribute('selected')).to.be.true; + expect(ampSelector.children[2].hasAttribute('selected')).to.be.true; - args = {'decrementPos': 2}; + args = {'delta': 2}; impl.executeAction( {method: 'selectUp', args, satisfiesTrust: () => true}); - expect(ampSelector.children[1].hasAttribute('selected')).to.be.false; + expect(ampSelector.children[2].hasAttribute('selected')).to.be.false; expect(ampSelector.children[0].hasAttribute('selected')).to.be.true; }); diff --git a/spec/amp-actions-and-events.md b/spec/amp-actions-and-events.md index 6d5ae48457f6..f7f78bbafa9b 100644 --- a/spec/amp-actions-and-events.md +++ b/spec/amp-actions-and-events.md @@ -369,12 +369,12 @@ event.response Description - selectUp(incrementPos=INTEGER) - Changes the selected element to the currentPos-incrementPos. If no increment value is specified, the default is 1. + selectUp(delta=INTEGER) + Changes the selected element to the currentPos - `delta`. The default `delta` is set to 1. selectDown(decrementPos=INTEGER) - Changes the selected element to the currentPos+decrementPos. If no decrement value is specified, the default is -1. + Changes the selected element to the currentPos + `delta`. The default `delta` is set to -1. From 33f521e001e88046ee7af81170fdb26bb0d6954b Mon Sep 17 00:00:00 2001 From: Naina Raisinghani Date: Wed, 21 Feb 2018 14:05:43 +1100 Subject: [PATCH 05/11] Cater for large negative numbers and add tests to check for that too --- extensions/amp-selector/0.1/amp-selector.js | 8 ++++- .../0.1/test/test-amp-selector.js | 32 +++++++++++++++++++ spec/amp-actions-and-events.md | 6 ++-- 3 files changed, 42 insertions(+), 4 deletions(-) diff --git a/extensions/amp-selector/0.1/amp-selector.js b/extensions/amp-selector/0.1/amp-selector.js index cedd8c16f2ec..31c456563bff 100644 --- a/extensions/amp-selector/0.1/amp-selector.js +++ b/extensions/amp-selector/0.1/amp-selector.js @@ -341,7 +341,13 @@ export class AmpSelector extends AMP.BaseElement { // past the beginning or end. const previousIndex = this.options_.indexOf(this.selectedOptions_[0]); const index = previousIndex + delta; - const normalizedIndex = index % this.options_.length; + const normalizedIndex = index > 0 ? + index % this.options_.length : + ((index % this.options_.length) + + this.options_.length) % this.options_.length; + + user().assert(normalizedIndex >= 0, + 'This should have been positive'); this.setSelection_(this.options_[normalizedIndex]); this.clearSelection_(this.options_[previousIndex]); diff --git a/extensions/amp-selector/0.1/test/test-amp-selector.js b/extensions/amp-selector/0.1/test/test-amp-selector.js index ff2b79d7a33d..02695f40254e 100644 --- a/extensions/amp-selector/0.1/test/test-amp-selector.js +++ b/extensions/amp-selector/0.1/test/test-amp-selector.js @@ -709,6 +709,38 @@ describes.realWin('amp-selector', { expect(ampSelector.children[0].hasAttribute('selected')).to.be.true; }); + it('should trigger `select` action when user uses ' + + '`selectUp`/`selectDown` action with user specified delta value ' + + '(test large values)', () => { + const ampSelector = getSelector({ + attributes: { + id: 'ampSelector', + }, + config: { + count: 5, + }, + }); + ampSelector.children[1].setAttribute('selected', ''); + ampSelector.build(); + const impl = ampSelector.implementation_; + + expect(ampSelector.hasAttribute('multiple')).to.be.false; + expect(ampSelector.children[1].hasAttribute('selected')).to.be.true; + + let args = {'delta': 1001}; + impl.executeAction( + {method: 'selectDown', args, satisfiesTrust: () => true}); + expect(ampSelector.children[1].hasAttribute('selected')).to.be.false; + expect(ampSelector.children[2].hasAttribute('selected')).to.be.true; + + args = {'delta': 1001}; + impl.executeAction( + {method: 'selectUp', args, satisfiesTrust: () => true}); + expect(ampSelector.children[2].hasAttribute('selected')).to.be.false; + expect(ampSelector.children[1].hasAttribute('selected')).to.be.true; + }); + + describe('keyboard-select-mode', () => { it('should have `none` mode by default', () => { diff --git a/spec/amp-actions-and-events.md b/spec/amp-actions-and-events.md index f7f78bbafa9b..52b3e5c4c702 100644 --- a/spec/amp-actions-and-events.md +++ b/spec/amp-actions-and-events.md @@ -370,11 +370,11 @@ event.response selectUp(delta=INTEGER) - Changes the selected element to the currentPos - `delta`. The default `delta` is set to 1. + Moves the selection up by the value of `delta`. The default `delta` is set to 1. - selectDown(decrementPos=INTEGER) - Changes the selected element to the currentPos + `delta`. The default `delta` is set to -1. + selectDown(delta=INTEGER) + Moves the selection down by the value of `delta`. The default `delta` is set to -1. From bfd6e114168fdc1a165b23539838b58f8e76b691 Mon Sep 17 00:00:00 2001 From: Naina Raisinghani Date: Wed, 21 Feb 2018 16:33:45 +1100 Subject: [PATCH 06/11] Rewrite code and tests to be cleaner --- extensions/amp-selector/0.1/amp-selector.js | 38 ++++----- .../0.1/test/test-amp-selector.js | 78 +++++++++++++++++++ 2 files changed, 92 insertions(+), 24 deletions(-) diff --git a/extensions/amp-selector/0.1/amp-selector.js b/extensions/amp-selector/0.1/amp-selector.js index a735d3cf4173..afdbccedcbb8 100644 --- a/extensions/amp-selector/0.1/amp-selector.js +++ b/extensions/amp-selector/0.1/amp-selector.js @@ -129,12 +129,10 @@ export class AmpSelector extends AMP.BaseElement { this.registerAction('toggle', invocation => { const args = invocation.args; if (args && args['index'] !== undefined) { - if (args['value'] !== undefined) { - this.toggle_(args['index'], args['value']); - } else { - this.toggle_(args['index'], + this.toggle_(args['index'], + args['value'] !== undefined ? + args['value'] : !this.options_[args['index']].hasAttribute('selected')); - } } }, ActionTrust.LOW); } @@ -352,27 +350,19 @@ export class AmpSelector extends AMP.BaseElement { // Change the selection to the next element in the specified direction. // The selection should loop around if the user attempts to go one // past the beginning or end. - const selectedIndex_ = this.options_.indexOf(this.selectedOptions_[0]); - - if (value == true) { - // If we are toggling the `selected` attribute to true: - // If selectedIndex_ == index then there is no change in the status. - - // If selectedIndex_ != index then the current selected element needs - // have the `selected` attribute removed and the element at position - // index needs to be selected. - if (selectedIndex_ != index) { + const indexCurrentStatus = this.options_[index].hasAttribute('selected'); + const indexFinalStatus = + !!(value !== undefined ? value : !indexCurrentStatus); + const selectedIndex = this.options_.indexOf(this.selectedOptions_[0]); + + // There is a change of the `selected` attribute for the element + if (indexFinalStatus !== indexCurrentStatus) { + if (selectedIndex !== index) { this.setSelection_(this.options_[index]); - this.clearSelection_(this.options_[selectedIndex_]); + this.clearSelection_(this.options_[selectedIndex]); + } else { + this.clearSelection_(this.options_[index]); } - } else { - // If we are toggling the `selected` attribute to false: - // If selectedIndex_ == index then the current selected element needs - // to have the `selected` attribute removed. - // If selectedIndex_ != index, then the current `selected` element - // can retain it's status but the element at the given index needs - // to have the `selected` attribute removed. - this.clearSelection_(this.options_[index]); } } diff --git a/extensions/amp-selector/0.1/test/test-amp-selector.js b/extensions/amp-selector/0.1/test/test-amp-selector.js index 16884f1aeadb..41c0b1742aa8 100644 --- a/extensions/amp-selector/0.1/test/test-amp-selector.js +++ b/extensions/amp-selector/0.1/test/test-amp-selector.js @@ -631,6 +631,84 @@ describes.realWin('amp-selector', { expect(setInputsSpy).calledTwice; }); + it('should trigger `toggle` action even when no `value` argument is' + + ' provided to the function', () => { + const ampSelector = getSelector({ + config: { + count: 5, + selectedCount: 1, + }, + }); + ampSelector.build(); + const impl = ampSelector.implementation_; + + expect(ampSelector.hasAttribute('multiple')).to.be.false; + expect(ampSelector.children[0].hasAttribute('selected')).to.be.true; + + // Test the case where the element to be `selected` and the currently + // selected element are different + let args = {'index': 2}; + impl.executeAction( + {method: 'toggle', args, satisfiesTrust: () => true}); + expect(ampSelector.children[0].hasAttribute('selected')).to.be.false; + expect(ampSelector.children[2].hasAttribute('selected')).to.be.true; + + // Test the case where the element to be `selected` and the currently + // selected element are the same + args = {'index': 2}; + impl.executeAction( + {method: 'toggle', args, satisfiesTrust: () => true}); + expect(ampSelector.children[2].hasAttribute('selected')).to.be.false; + }); + + it('should trigger `toggle` action even with specified `value`' + + ' argument', () => { + const ampSelector = getSelector({ + config: { + count: 5, + selectedCount: 1, + }, + }); + ampSelector.build(); + const impl = ampSelector.implementation_; + + expect(ampSelector.hasAttribute('multiple')).to.be.false; + expect(ampSelector.children[0].hasAttribute('selected')).to.be.true; + + // Test the case where the element to be `selected` and the currently + // selected element are different + let args = {'index': 2, 'value': true}; + impl.executeAction( + {method: 'toggle', args, satisfiesTrust: () => true}); + expect(ampSelector.children[0].hasAttribute('selected')).to.be.false; + expect(ampSelector.children[2].hasAttribute('selected')).to.be.true; + + // Test the case where the element to be `selected` and the currently + // selected element are the same + args = {'index': 2, 'value': true}; + impl.executeAction( + {method: 'toggle', args, satisfiesTrust: () => true}); + expect(ampSelector.children[2].hasAttribute('selected')).to.be.true; + + // Test the case where the element to be removed as `selected` and + // the currently selected element are the same + args = {'index': 2, 'value': false}; + impl.executeAction( + {method: 'toggle', args, satisfiesTrust: () => true}); + expect(ampSelector.children[2].hasAttribute('selected')).to.be.false; + + // Test the case where the element to be removed as `selected` + // is different from the currently `selected` element + ampSelector.children[0].setAttribute('selected', ''); + expect(ampSelector.children[0].hasAttribute('selected')).to.be.true; + + args = {'index': 2, 'value': false}; + impl.executeAction( + {method: 'toggle', args, satisfiesTrust: () => true}); + expect(ampSelector.children[0].hasAttribute('selected')).to.be.true; + expect(ampSelector.children[2].hasAttribute('selected')).to.be.false; + }); + it('should trigger `select` action when user selects an option', () => { const ampSelector = getSelector({ config: { From a848f219e07cb400e7b3c482ae8f9c87724dac61 Mon Sep 17 00:00:00 2001 From: Naina Raisinghani Date: Thu, 22 Feb 2018 10:10:29 +1100 Subject: [PATCH 07/11] Move mod functionality to helper and add tests too --- extensions/amp-selector/0.1/amp-selector.js | 10 +---- src/utils/math.js | 26 +++++++++++ test/functional/utils/test-math.js | 50 ++++++++++++++++++++- 3 files changed, 77 insertions(+), 9 deletions(-) diff --git a/extensions/amp-selector/0.1/amp-selector.js b/extensions/amp-selector/0.1/amp-selector.js index 31c456563bff..ade6f4a05898 100644 --- a/extensions/amp-selector/0.1/amp-selector.js +++ b/extensions/amp-selector/0.1/amp-selector.js @@ -21,7 +21,7 @@ import {Services} from '../../../src/services'; import {closestBySelector, isRTL, tryFocus} from '../../../src/dom'; import {createCustomEvent} from '../../../src/event-helper'; import {dev, user} from '../../../src/log'; - +import {mod} from '../../../src/utils/math'; const TAG = 'amp-selector'; /** @@ -341,13 +341,7 @@ export class AmpSelector extends AMP.BaseElement { // past the beginning or end. const previousIndex = this.options_.indexOf(this.selectedOptions_[0]); const index = previousIndex + delta; - const normalizedIndex = index > 0 ? - index % this.options_.length : - ((index % this.options_.length) + - this.options_.length) % this.options_.length; - - user().assert(normalizedIndex >= 0, - 'This should have been positive'); + const normalizedIndex = mod(index, this.options_.length); this.setSelection_(this.options_[normalizedIndex]); this.clearSelection_(this.options_[previousIndex]); diff --git a/src/utils/math.js b/src/utils/math.js index 286eb0f554b4..25154bf74a8a 100644 --- a/src/utils/math.js +++ b/src/utils/math.js @@ -53,6 +53,32 @@ export function mapRange(val, min1, max1, min2, max2) { return (val - min1) * (max2 - min2) / (max1 - min1) + min2; } +/** + * Computes the modulus of values `a` and `b`. + * + * This is needed because the % operator in JavaScript doesn't implement + * modulus behaviour as can be seen by the spec here: + * http://www.ecma-international.org/ecma-262/5.1/#sec-11.5.3. + * It instead is used to obtain the remainder of a division. + * This function uses the remainder (%) operator to determine the modulus. + * Derived from here: + * https://stackoverflow.com/questions/25726760/javascript-modular-arithmetic/47354356#47354356 + * + * @param {number} a + * @param {number} b + * @returns {number} returns the modulus of the two numbers. + * @example + * + * _.min(10, 5); + * // => 0 + * + * _.mod(-1, 5); + * // => 4 + */ +export function mod(a, b) { + return a > 0 && b > 0 ? a % b : ((a % b) + b) % b; +} + /** * Restricts a number to be in the given min/max range. * diff --git a/test/functional/utils/test-math.js b/test/functional/utils/test-math.js index 40c68a0090df..c1283fcd7851 100644 --- a/test/functional/utils/test-math.js +++ b/test/functional/utils/test-math.js @@ -14,7 +14,7 @@ * limitations under the License. */ -import {clamp, mapRange} from '../../../src/utils/math'; +import {clamp, mapRange, mod} from '../../../src/utils/math'; describes.sandboxed('utils/math', {}, () => { @@ -40,6 +40,54 @@ describes.sandboxed('utils/math', {}, () => { }); }); + describe('mod', () => { + it('a -> positive number, b -> positive number', () => { + expect(mod(0, 5)).to.equal(0); + expect(mod(1, 5)).to.equal(1); + expect(mod(2, 5)).to.equal(2); + expect(mod(3, 5)).to.equal(3); + expect(mod(4, 5)).to.equal(4); + expect(mod(5, 5)).to.equal(0); + expect(mod(6, 5)).to.equal(1); + expect(mod(7, 5)).to.equal(2); + expect(mod(1001, 5)).to.equal(1); + }); + + it('a -> negative number, b -> positive number', () => { + expect(mod(-1, 5)).to.equal(4); + expect(mod(-2, 5)).to.equal(3); + expect(mod(-3, 5)).to.equal(2); + expect(mod(-4, 5)).to.equal(1); + expect(mod(-5, 5)).to.equal(0); + expect(mod(-6, 5)).to.equal(4); + expect(mod(-7, 5)).to.equal(3); + expect(mod(-1001, 5)).to.equal(4); + }); + + it('a -> positive number, b -> negative number', () => { + expect(mod(0, -5)).to.equal(0); + expect(mod(1, -5)).to.equal(-4); + expect(mod(2, -5)).to.equal(-3); + expect(mod(3, -5)).to.equal(-2); + expect(mod(4, -5)).to.equal(-1); + expect(mod(5, -5)).to.equal(0); + expect(mod(6, -5)).to.equal(-4); + expect(mod(7, -5)).to.equal(-3); + expect(mod(1001, -5)).to.equal(-4); + }); + + it('a -> negative number, b -> negative number', () => { + expect(mod(-1, -5)).to.equal(-1); + expect(mod(-2, -5)).to.equal(-2); + expect(mod(-3, -5)).to.equal(-3); + expect(mod(-4, -5)).to.equal(-4); + expect(mod(-5, -5)).to.equal(0); + expect(mod(-6, -5)).to.equal(-1); + expect(mod(-7, -5)).to.equal(-2); + expect(mod(-1001, -5)).to.equal(-1); + }); + }); + describe('clamp', () => { it('should not clamp if within the range', () => { expect(clamp(0.5, 0, 1)).to.equal(0.5); From 861554f8bb1627d958391cfbc33737335665d2f0 Mon Sep 17 00:00:00 2001 From: Naina Raisinghani Date: Thu, 22 Feb 2018 11:15:46 +1100 Subject: [PATCH 08/11] Remove unnecessary attribute logic --- extensions/amp-selector/0.1/amp-selector.js | 27 ++++++++++----------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/extensions/amp-selector/0.1/amp-selector.js b/extensions/amp-selector/0.1/amp-selector.js index 42afaf4a11ba..306e2e6a6442 100644 --- a/extensions/amp-selector/0.1/amp-selector.js +++ b/extensions/amp-selector/0.1/amp-selector.js @@ -129,10 +129,7 @@ export class AmpSelector extends AMP.BaseElement { this.registerAction('toggle', invocation => { const args = invocation.args; if (args && args['index'] !== undefined) { - this.toggle_(args['index'], - args['value'] !== undefined ? - args['value'] : - !this.options_[args['index']].hasAttribute('selected')); + this.toggle_(args['index'], args['value']); } }, ActionTrust.LOW); } @@ -344,25 +341,27 @@ export class AmpSelector extends AMP.BaseElement { /** * Handles toggle action. * @param {number} index - * @param {boolean} value + * @param {boolean=} opt_value */ - toggle_(index, value) { + toggle_(index, opt_value) { // Change the selection to the next element in the specified direction. // The selection should loop around if the user attempts to go one // past the beginning or end. const indexCurrentStatus = this.options_[index].hasAttribute('selected'); const indexFinalStatus = - !!(value !== undefined ? value : !indexCurrentStatus); + opt_value !== undefined ? opt_value : !indexCurrentStatus; const selectedIndex = this.options_.indexOf(this.selectedOptions_[0]); + if (indexFinalStatus === indexCurrentStatus) { + return; + } + // There is a change of the `selected` attribute for the element - if (indexFinalStatus !== indexCurrentStatus) { - if (selectedIndex !== index) { - this.setSelection_(this.options_[index]); - this.clearSelection_(this.options_[selectedIndex]); - } else { - this.clearSelection_(this.options_[index]); - } + if (selectedIndex !== index) { + this.setSelection_(this.options_[index]); + this.clearSelection_(this.options_[selectedIndex]); + } else { + this.clearSelection_(this.options_[index]); } } From d043d7f7da4504879310ca152a8e22050d2c0167 Mon Sep 17 00:00:00 2001 From: Naina Raisinghani Date: Fri, 23 Feb 2018 11:33:24 +1100 Subject: [PATCH 09/11] Add assert to ensure index is > 0 --- extensions/amp-selector/0.1/amp-selector.js | 1 + 1 file changed, 1 insertion(+) diff --git a/extensions/amp-selector/0.1/amp-selector.js b/extensions/amp-selector/0.1/amp-selector.js index 306e2e6a6442..1e1a52c4ff4f 100644 --- a/extensions/amp-selector/0.1/amp-selector.js +++ b/extensions/amp-selector/0.1/amp-selector.js @@ -128,6 +128,7 @@ export class AmpSelector extends AMP.BaseElement { this.registerAction('toggle', invocation => { const args = invocation.args; + user().assert(args['index'] >= 0, '\'index\' must be greater than 0'); if (args && args['index'] !== undefined) { this.toggle_(args['index'], args['value']); } From c6c85cbb07ae70099e5abdba4a7d6bc711079d30 Mon Sep 17 00:00:00 2001 From: Naina Raisinghani Date: Sat, 24 Feb 2018 15:45:25 +1100 Subject: [PATCH 10/11] Add additional assert --- extensions/amp-selector/0.1/amp-selector.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/extensions/amp-selector/0.1/amp-selector.js b/extensions/amp-selector/0.1/amp-selector.js index 1e1a52c4ff4f..866fbb3c8aaf 100644 --- a/extensions/amp-selector/0.1/amp-selector.js +++ b/extensions/amp-selector/0.1/amp-selector.js @@ -129,6 +129,8 @@ export class AmpSelector extends AMP.BaseElement { this.registerAction('toggle', invocation => { const args = invocation.args; user().assert(args['index'] >= 0, '\'index\' must be greater than 0'); + user().assert(args['index'] < this.options_.length, '\'index\' must be less than the ' + + 'length of options in the '); if (args && args['index'] !== undefined) { this.toggle_(args['index'], args['value']); } From 0cb7e6b76f090f67d04d21967029148090312f91 Mon Sep 17 00:00:00 2001 From: nainar Date: Sat, 24 Feb 2018 22:53:41 +1100 Subject: [PATCH 11/11] Linter error --- extensions/amp-selector/0.1/amp-selector.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/extensions/amp-selector/0.1/amp-selector.js b/extensions/amp-selector/0.1/amp-selector.js index 866fbb3c8aaf..d5fbf03d2f64 100644 --- a/extensions/amp-selector/0.1/amp-selector.js +++ b/extensions/amp-selector/0.1/amp-selector.js @@ -129,8 +129,8 @@ export class AmpSelector extends AMP.BaseElement { this.registerAction('toggle', invocation => { const args = invocation.args; user().assert(args['index'] >= 0, '\'index\' must be greater than 0'); - user().assert(args['index'] < this.options_.length, '\'index\' must be less than the ' + - 'length of options in the '); + user().assert(args['index'] < this.options_.length, '\'index\' must be ' + + 'less than the length of options in the '); if (args && args['index'] !== undefined) { this.toggle_(args['index'], args['value']); }