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

amp-selector: "select" event should have variable trust #24431

Merged
merged 5 commits into from Oct 24, 2019
Merged
Show file tree
Hide file tree
Changes from 3 commits
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
38 changes: 21 additions & 17 deletions extensions/amp-selector/0.1/amp-selector.js
Expand Up @@ -122,35 +122,35 @@ export class AmpSelector extends AMP.BaseElement {
this.registerAction(
'selectUp',
invocation => {
const {args} = invocation;
const {args, trust} = invocation;
const delta = args && args['delta'] !== undefined ? -args['delta'] : -1;
this.select_(delta);
this.select_(delta, trust);
},
ActionTrust.LOW
);

this.registerAction(
'selectDown',
invocation => {
const {args} = invocation;
const {args, trust} = invocation;
const delta = args && args['delta'] !== undefined ? args['delta'] : 1;
this.select_(delta);
this.select_(delta, trust);
},
ActionTrust.LOW
);

this.registerAction(
'toggle',
invocation => {
const {args} = invocation;
const {args, trust} = invocation;
userAssert(args['index'] >= 0, "'index' must be greater than 0");
userAssert(
args['index'] < this.elements_.length,
"'index' must " +
'be less than the length of options in the <amp-selector>'
);
if (args && args['index'] !== undefined) {
return this.toggle_(args['index'], args['value']);
return this.toggle_(args['index'], args['value'], trust);
} else {
return Promise.reject("'index' must be specified");
}
Expand Down Expand Up @@ -363,7 +363,8 @@ export class AmpSelector extends AMP.BaseElement {
}
// Newly picked option should always have focus.
this.updateFocus_(el);
this.fireSelectEvent_(el);
// User gesture trigger is "high" trust.
this.fireSelectEvent_(el, ActionTrust.HIGH);
});
}

Expand Down Expand Up @@ -399,18 +400,18 @@ export class AmpSelector extends AMP.BaseElement {
/**
* Handles toggle action.
* @param {number} index
* @param {boolean=} opt_value
* @param {boolean|undefined} value
* @param {!ActionTrust} trust
* @return {!Promise}
* @private
*/
toggle_(index, opt_value) {
toggle_(index, value, trust) {
// 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 el = this.elements_[index];
const indexCurrentStatus = el.hasAttribute('selected');
const indexFinalStatus =
opt_value !== undefined ? opt_value : !indexCurrentStatus;
const indexFinalStatus = value !== undefined ? value : !indexCurrentStatus;
const selectedIndex = this.elements_.indexOf(this.selectedElements_[0]);

if (indexFinalStatus === indexCurrentStatus) {
Expand All @@ -428,8 +429,8 @@ export class AmpSelector extends AMP.BaseElement {
} else {
this.clearSelection_(el);
}

this.fireSelectEvent_(el);
// Propagate the trust of the originating action.
this.fireSelectEvent_(el, trust);
});
}

Expand All @@ -438,9 +439,10 @@ export class AmpSelector extends AMP.BaseElement {
* 'targetOption' - option value of the selected or deselected element.
* 'selectedOptions' - array of option values of selected elements.
* @param {!Element} el The element that was selected or deslected.
* @param {!ActionTrust} trust
* @private
*/
fireSelectEvent_(el) {
fireSelectEvent_(el, trust) {
const name = 'select';
const selectEvent = createCustomEvent(
this.win,
Expand All @@ -450,15 +452,16 @@ export class AmpSelector extends AMP.BaseElement {
'selectedOptions': this.selectedOptions_(),
})
);
this.action_.trigger(this.element, name, selectEvent, ActionTrust.HIGH);
this.action_.trigger(this.element, name, selectEvent, trust);
}

/**
* Handles selectUp events.
* @param {number} delta
* @param {!ActionTrust} trust
* @private
*/
select_(delta) {
select_(delta, trust) {
// 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.
Expand All @@ -479,7 +482,8 @@ export class AmpSelector extends AMP.BaseElement {
}

this.setInputs_();
this.fireSelectEvent_(el);
// Propagate the trust of the source action.
this.fireSelectEvent_(el, trust);
}

/**
Expand Down
24 changes: 23 additions & 1 deletion extensions/amp-selector/0.1/test/test-amp-selector.js
Expand Up @@ -15,6 +15,7 @@
*/

import '../amp-selector';
import {ActionTrust} from '../../../../src/action-constants';
import {AmpEvents} from '../../../../src/amp-events';
import {Keys} from '../../../../src/utils/key-codes';

Expand Down Expand Up @@ -845,6 +846,9 @@ describes.realWin(
expect(event).to.have.property('detail');
expect(event.detail).to.have.property('targetOption', '3');
expect(event.detail).to.have.deep.property('selectedOptions', ['3']);

const trust = triggerSpy.firstCall.args[3];
expect(trust).to.equal(ActionTrust.HIGH);
});

it('should trigger "select" event when an item is toggled', async () => {
Expand All @@ -864,6 +868,7 @@ describes.realWin(
method: 'toggle',
args,
satisfiesTrust: () => true,
trust: 789,
});

expect(triggerSpy).to.be.calledOnce;
Expand All @@ -873,6 +878,9 @@ describes.realWin(
expect(event).to.have.property('detail');
expect(event.detail).to.have.property('targetOption', '3');
expect(event.detail).to.have.deep.property('selectedOptions', ['3']);

const trust = triggerSpy.firstCall.args[3];
expect(trust).to.equal(789);
});

it('should trigger "select" event for multiple selections', function*() {
Expand Down Expand Up @@ -906,6 +914,9 @@ describes.realWin(
'1',
'2',
]);

const trust = triggerSpy.firstCall.args[3];
expect(trust).to.equal(ActionTrust.HIGH);
});

it(
Expand All @@ -931,6 +942,7 @@ describes.realWin(
impl.executeAction({
method: 'selectDown',
satisfiesTrust: () => true,
trust: 789,
});
expect(ampSelector.children[0].hasAttribute('selected')).to.be.false;
expect(ampSelector.children[1].hasAttribute('selected')).to.be.true;
Expand All @@ -942,6 +954,9 @@ describes.realWin(
expect(event).to.have.property('detail');
expect(event.detail).to.have.property('targetOption', '1');
expect(event.detail).to.have.deep.property('selectedOptions', ['1']);

const trust = triggerSpy.firstCall.args[3];
expect(trust).to.equal(789);
}
);

Expand All @@ -965,7 +980,11 @@ describes.realWin(
expect(ampSelector.hasAttribute('multiple')).to.be.false;
expect(ampSelector.children[0].hasAttribute('selected')).to.be.true;

impl.executeAction({method: 'selectUp', satisfiesTrust: () => true});
impl.executeAction({
method: 'selectUp',
satisfiesTrust: () => true,
trust: 789,
});

expect(ampSelector.children[0].hasAttribute('selected')).to.be.false;
expect(ampSelector.children[5].hasAttribute('selected')).to.be.true;
Expand All @@ -977,6 +996,9 @@ describes.realWin(
expect(event).to.have.property('detail');
expect(event.detail).to.have.property('targetOption', '5');
expect(event.detail).to.have.deep.property('selectedOptions', ['5']);

const trust = triggerSpy.firstCall.args[3];
expect(trust).to.equal(789);
}
);

Expand Down