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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

馃悰amp-autocomplete: Fix submit-on-enter bug #21800

Merged
merged 4 commits into from Apr 23, 2019
Merged

馃悰amp-autocomplete: Fix submit-on-enter bug #21800

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1b622f5
Fix submit-on-enter bug
caroqliu Apr 10, 2019
0401d63
Refactor re/setting active element index
caroqliu Apr 10, 2019
72ec9f5
Fix tests
caroqliu Apr 10, 2019
16df5cf
Update documentation for 'submit-on-enter'
caroqliu Apr 10, 2019
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
17 changes: 9 additions & 8 deletions extensions/amp-autocomplete/0.1/amp-autocomplete.js
View file
Open in desktop
Expand Up @@ -358,6 +358,7 @@ export class AmpAutocomplete extends AMP.BaseElement {
*/
renderResults_(filteredData, container) {
let renderPromise = Promise.resolve();
this.resetActiveElement_();
if (this.templateElement_) {
renderPromise = this.templates_.renderTemplateArray(this.templateElement_,
filteredData).then(renderedChildren => {
Expand Down Expand Up @@ -453,7 +454,6 @@ export class AmpAutocomplete extends AMP.BaseElement {
return this.mutateElement(() => {
if (!display) {
this.resetActiveElement_();
this.activeIndex_ = -1;
}
this.toggleResults_(display);
});
Expand Down Expand Up @@ -524,12 +524,13 @@ export class AmpAutocomplete extends AMP.BaseElement {
}
const keyUpWhenNoneActive = this.activeIndex_ === -1 && delta < 0;
const index = keyUpWhenNoneActive ? delta : this.activeIndex_ + delta;
this.activeIndex_ = mod(index, this.container_.children.length);
const newActiveElement = this.container_.children[this.activeIndex_];
const activeIndex = mod(index, this.container_.children.length);
const newActiveElement = this.container_.children[activeIndex];
this.inputElement_.value = newActiveElement.getAttribute('value');
return this.mutateElement(() => {
this.resetActiveElement_();
newActiveElement.classList.add('i-amphtml-autocomplete-item-active');
this.activeIndex_ = activeIndex;
this.activeElement_ = newActiveElement;
});
}
Expand All @@ -541,11 +542,10 @@ export class AmpAutocomplete extends AMP.BaseElement {
displayUserInput_() {
this.inputElement_.value = this.userInput_;
this.resetActiveElement_();
this.activeIndex_ = -1;
}

/**
* Resets the activeElement_ and removes its 'active' class.
* Resets the activeIndex_, activeElement_ and removes its 'active' class.
* @private
*/
resetActiveElement_() {
Expand All @@ -555,6 +555,7 @@ export class AmpAutocomplete extends AMP.BaseElement {
this.activeElement_.classList.toggle(
'i-amphtml-autocomplete-item-active', false);
this.activeElement_ = null;
this.activeIndex_ = -1;
}

/**
Expand Down Expand Up @@ -590,10 +591,10 @@ export class AmpAutocomplete extends AMP.BaseElement {
}
return this.updateActiveItem_(-1);
case Keys.ENTER:
if (this.resultsShowing_() && !this.submitOnEnter_) {
event.preventDefault();
}
if (this.activeElement_) {
if (!this.submitOnEnter_) {
event.preventDefault();
}
return this.mutateElement(() => {
this.selectItem_(this.activeElement_);
this.resetActiveElement_();
Expand Down
11 changes: 7 additions & 4 deletions extensions/amp-autocomplete/0.1/test/test-amp-autocomplete.js
View file
Open in desktop
Expand Up @@ -368,6 +368,7 @@ describes.realWin('amp-autocomplete unit tests', {
it('should call selectItem_ and resetActiveElement_ as expected', () => {
return layoutAndSetSpies().then(() => {
impl.activeElement_ = impl.createElementFromItem_('abc');
sandbox.stub(impl, 'resultsShowing_').returns(true);
return impl.keyDownHandler_(event);
}).then(() => {
expect(impl.inputElement_.value).to.equal('abc');
Expand Down Expand Up @@ -407,21 +408,22 @@ describes.realWin('amp-autocomplete unit tests', {
selectItemSpy = sandbox.spy(impl, 'selectItem_');
resetSpy = sandbox.spy(impl, 'resetActiveElement_');
clearAllSpy = sandbox.spy(impl, 'clearAllItems_');
sandbox.stub(impl, 'resultsShowing_').returns(true);
return impl.keyDownHandler_(event);
}).then(() => {
expect(impl.inputElement_.value).to.equal('');
expect(selectItemSpy).not.to.have.been.called;
expect(clearAllSpy).not.to.have.been.called;
expect(resetSpy).not.to.have.been.called;
expect(eventPreventSpy).not.to.have.been.called;
expect(eventPreventSpy).to.have.been.calledOnce;
impl.activeElement_ = impl.createElementFromItem_('abc');
return impl.keyDownHandler_(event);
}).then(() => {
expect(impl.inputElement_.value).to.equal('abc');
expect(selectItemSpy).to.have.been.calledOnce;
expect(clearAllSpy).to.have.been.calledOnce;
expect(resetSpy).to.have.been.calledOnce;
expect(eventPreventSpy).to.have.been.calledOnce;
expect(eventPreventSpy).to.have.been.calledTwice;
expect(impl.submitOnEnter_).to.be.false;
impl.submitOnEnter_ = true;
impl.activeElement_ = impl.createElementFromItem_('abc');
Expand All @@ -431,7 +433,7 @@ describes.realWin('amp-autocomplete unit tests', {
expect(selectItemSpy).to.have.been.calledTwice;
expect(clearAllSpy).to.have.been.calledTwice;
expect(resetSpy).to.have.been.calledTwice;
expect(eventPreventSpy).to.have.been.calledOnce;
expect(eventPreventSpy).to.have.been.calledTwice;
expect(impl.submitOnEnter_).to.be.true;
});
});
Expand All @@ -446,12 +448,13 @@ describes.realWin('amp-autocomplete unit tests', {
return impl.renderResults_(impl.sourceData_, impl.container_);
}).then(() => {
expect(impl.container_.children.length).to.equal(3);
expect(resetSpy).to.have.been.calledOnce;
impl.toggleResults_(true);
expect(impl.resultsShowing_()).to.be.true;
return impl.keyDownHandler_(event);
}).then(() => {
expect(displayInputSpy).to.have.been.calledOnce;
expect(resetSpy).to.have.been.calledOnce;
expect(resetSpy).to.have.been.calledTwice;
expect(toggleResultsSpy).to.have.been.calledWith(false);
expect(impl.resultsShowing_()).to.be.false;
});
Expand Down
3 changes: 2 additions & 1 deletion extensions/amp-autocomplete/amp-autocomplete.md
View file
Open in desktop
Expand Up @@ -111,7 +111,8 @@ Example:
<tr>
<td width="40%"><strong>submit-on-enter [optional]</strong></td>
<td>
The enter key is primarily used for selecting suggestions in autocomplete, so it shouldn鈥檛 also submit the form unless the developer explicitly sets it to do so (for search fields/one field forms, et cetera). Defaults to false.
The enter key is primarily used for selecting suggestions in autocomplete, so it shouldn鈥檛 also submit the form unless the developer explicitly sets it to do so (for search fields/one field forms, et cetera).
The user flow is as follows: If <code>submit-on-enter</code> is <code>true</code>, pressing <code>Enter</code> will select any currently active item and engage in default behavior, including submitting the form if applicable. If <code>submit-on-enter</code> is <code>false</code>, pressing <code>Enter</code> <em>while suggestions are displaying</em> will select any currently active item only and prevent any other default behavior. If suggestions are not displaying, autocomplete allows default behavior. <strong>Defaults to false.</strong>
</td>
</tr>
</table>
Expand Down