From 55168f85305a8c75832a977e398d40dc14207a3d Mon Sep 17 00:00:00 2001 From: Caroline Liu Date: Tue, 12 Feb 2019 10:32:35 -0500 Subject: [PATCH 1/6] Check for undefined parameter in clearSelection_() --- extensions/amp-selector/0.1/amp-selector.js | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/extensions/amp-selector/0.1/amp-selector.js b/extensions/amp-selector/0.1/amp-selector.js index e837ad73d164..525f612e5f97 100644 --- a/extensions/amp-selector/0.1/amp-selector.js +++ b/extensions/amp-selector/0.1/amp-selector.js @@ -539,11 +539,13 @@ export class AmpSelector extends AMP.BaseElement { * @private */ clearSelection_(element) { - element.removeAttribute('selected'); - element.setAttribute('aria-selected', 'false'); - const selIndex = this.selectedElements_.indexOf(element); - if (selIndex !== -1) { - this.selectedElements_.splice(selIndex, 1); + if (element !== undefined) { + element.removeAttribute('selected'); + element.setAttribute('aria-selected', 'false'); + const selIndex = this.selectedElements_.indexOf(element); + if (selIndex !== -1) { + this.selectedElements_.splice(selIndex, 1); + } } } From fa0e71a4fb4d67c84eb6ed2b32bd96aad69e5bec Mon Sep 17 00:00:00 2001 From: Caroline Liu Date: Tue, 12 Feb 2019 10:33:08 -0500 Subject: [PATCH 2/6] Create example file to test changes locally. --- examples/selector.amp.html | 60 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 60 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..7f9a2a8678ee --- /dev/null +++ b/examples/selector.amp.html @@ -0,0 +1,60 @@ + + + + + AMP Selector Demo + + + + + + + + +

Selector Examples

+

Single Select

+ +
    +
  • Option 1
    • +
  • Option 2
    • +
  • Option 3
    • +
  • Option 4
    • +
+ + +

Multi Select

+ +
    +
  • Option 1
    • +
  • Option 2
    • +
  • Option 3
    • +
  • Option 4
    • +
+ + +

Actions

+ +
    +
  • Option 1
    • +
  • Option 2
    • +
  • Option 3
    • +
  • Option 4
    • +
  • Option 5
    • +
  • Option 6
    • +
+ + + + + + + + + + From 007a5b9a8898857ce45d887ce3127a581aa566eb Mon Sep 17 00:00:00 2001 From: Caroline Liu Date: Tue, 12 Feb 2019 10:40:07 -0500 Subject: [PATCH 3/6] Reorganize elements in --- examples/selector.amp.html | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/examples/selector.amp.html b/examples/selector.amp.html index 7f9a2a8678ee..84a07ef95cd7 100644 --- a/examples/selector.amp.html +++ b/examples/selector.amp.html @@ -2,12 +2,11 @@ + AMP Selector Demo - - From b896a0e48d91f1d0ee71a585f2012ffcc3f5a564 Mon Sep 17 00:00:00 2001 From: Caroline Liu Date: Tue, 12 Feb 2019 12:15:16 -0500 Subject: [PATCH 4/6] Remove guard from clearSelection() --- extensions/amp-selector/0.1/amp-selector.js | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/extensions/amp-selector/0.1/amp-selector.js b/extensions/amp-selector/0.1/amp-selector.js index 525f612e5f97..e837ad73d164 100644 --- a/extensions/amp-selector/0.1/amp-selector.js +++ b/extensions/amp-selector/0.1/amp-selector.js @@ -539,13 +539,11 @@ export class AmpSelector extends AMP.BaseElement { * @private */ clearSelection_(element) { - if (element !== undefined) { - element.removeAttribute('selected'); - element.setAttribute('aria-selected', 'false'); - const selIndex = this.selectedElements_.indexOf(element); - if (selIndex !== -1) { - this.selectedElements_.splice(selIndex, 1); - } + element.removeAttribute('selected'); + element.setAttribute('aria-selected', 'false'); + const selIndex = this.selectedElements_.indexOf(element); + if (selIndex !== -1) { + this.selectedElements_.splice(selIndex, 1); } } From 60146fd3b14c14fa37c8d218d8d6e8e3a234c0d2 Mon Sep 17 00:00:00 2001 From: Caroline Liu Date: Tue, 12 Feb 2019 12:20:02 -0500 Subject: [PATCH 5/6] Add undefined check to toggle() before calling clearSelection() --- extensions/amp-selector/0.1/amp-selector.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/extensions/amp-selector/0.1/amp-selector.js b/extensions/amp-selector/0.1/amp-selector.js index e837ad73d164..f8f27e7321da 100644 --- a/extensions/amp-selector/0.1/amp-selector.js +++ b/extensions/amp-selector/0.1/amp-selector.js @@ -395,7 +395,10 @@ export class AmpSelector extends AMP.BaseElement { // There is a change of the `selected` attribute for the element if (selectedIndex !== index) { this.setSelection_(el); - this.clearSelection_(this.elements_[selectedIndex]); + const selectedEl = this.elements_[selectedIndex]; + if (selectedEl) { + this.clearSelection_(selectedEl); + } } else { this.clearSelection_(el); } From 8fa348701f27cfdfde77631478d12a5e60b1ab8e Mon Sep 17 00:00:00 2001 From: Caroline Liu Date: Tue, 12 Feb 2019 12:20:26 -0500 Subject: [PATCH 6/6] Add undefined check to select() before calling clearSelection() --- extensions/amp-selector/0.1/amp-selector.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/extensions/amp-selector/0.1/amp-selector.js b/extensions/amp-selector/0.1/amp-selector.js index f8f27e7321da..82b3a25266d1 100644 --- a/extensions/amp-selector/0.1/amp-selector.js +++ b/extensions/amp-selector/0.1/amp-selector.js @@ -437,7 +437,10 @@ export class AmpSelector extends AMP.BaseElement { const normalizedIndex = mod(index, this.elements_.length); this.setSelection_(this.elements_[normalizedIndex]); - this.clearSelection_(this.elements_[previousIndex]); + const previousEl = this.elements_[previousIndex]; + if (previousEl) { + this.clearSelection_(previousEl); + } } /**