✨ Amp Actions: AMP.goBack action should call history.back by samouri · Pull Request #26585 · ampproject/amphtml

samouri · 2020-01-31T23:41:13Z

Implements #5225

dreamofabear · 2020-02-04T20:37:42Z

Not a huge fan of this name. Maybe we can parameterize goBack? #bikeshed

dvoytenko · 2020-02-06T20:09:36Z

Not a huge fan of this name. Maybe we can parameterize goBack? #bikeshed

I mentioned this on the issue as well. But I believe in the current world, we could just extend goBack w/o any new parameters, as long as we have a user gesture.

samouri · 2020-02-06T22:46:14Z

Does that mean:

With user gesture: pop the same-doc history stack. If stack is empty call history.back
Without user gesture: only pop same-doc history stack

dvoytenko · 2020-02-07T20:49:56Z

With user gesture: pop the same-doc history stack. If stack is empty call history.back

Without user gesture: only pop same-doc history stack

From what I can see: a history stack modification with a user gesture is very unambiguous for within- and cross-doc navigation (i.e. history stack pop). I'd even constraint all of them. But if it's "too late", I believe it would be reasonable to allow cross-doc navigation iff the user gesture is present.

dreamofabear · 2020-02-11T16:55:54Z

I think goBack currently already requires a user gesture per ActionTrust.

amphtml/src/service/standard-actions-impl.js

Lines 145 to 149 in ca175d7

    
           handleAmpTarget_(invocation) { 
        
             // All global `AMP` actions require default trust. 
        
             if (!invocation.satisfiesTrust(ActionTrust.DEFAULT)) { 
        
               return null; 
        
             }

Would any existing pages rely on the fact that goBack doesn't pop window history? I'm having trouble thinking of a good use case. If not, I suppose we could just treat this as a bug fix and forge ahead (probably worth an I2S though).

dvoytenko · 2020-02-11T19:20:49Z

I think goBack currently already requires a user gesture per ActionTrust.

amphtml/src/service/standard-actions-impl.js

Lines 145 to 149 in ca175d7

handleAmpTarget_(invocation) {

// All global `AMP` actions require default trust.

if (!invocation.satisfiesTrust(ActionTrust.DEFAULT)) {

return null;

}

Would any existing pages rely on the fact that goBack doesn't pop window history? I'm having trouble thinking of a good use case. If not, I suppose we could just treat this as a bug fix and forge ahead (probably worth an I2S though).

Does ActionTrust.DEFAULT cover only active gestures? If so - we can proceed with it then. If we have any worries w.r.t. backward compatibility, we can parametrize.

samouri · 2020-02-11T21:58:10Z

Does ActionTrust.DEFAULT cover only active gestures? If so - we can proceed with it then. If we have any worries w.r.t. backward compatibility, we can parametrize.

Yes! At least according to action-constants.js:

amphtml/src/action-constants.js

Lines 56 to 63 in ca175d7

    
              * Events that are triggered nearly immediately (up to a few seconds) after 
        
              * a user gesture with strong intent (e.g. tap or swipe) are "default trust". 
        
              * 
        
              * Actions that can modify the page's visual state (e.g. content jumping) 
        
              * should require "default trust". This is the default required trust level 
        
              * for actions. 
        
              */ 
        
             DEFAULT: 2,

I do still see this .goBack function called in a couple of places though, so I'll need to parameterize it to make sure its only used in places with at least default trust.

samouri · 2020-02-14T20:51:38Z

I've made the updates, this should be ready for review

src/service/history-impl.js

src/service/standard-actions-impl.js

src/service/history-impl.js

src/service/standard-actions-impl.js

dreamofabear · 2020-07-14T23:09:52Z

src/service/standard-actions-impl.js

      case 'goBack':
-        Services.historyForDoc(this.ampdoc).goBack();
+        Services.historyForDoc(this.ampdoc).goBack(
+          /*canPerforWindowBack*/ !!args && args['navigate']


Is args['navigate'] a boolean or a string? If string, what if I pass navigate=false?

Great question! There is a full ParserTokenizer that will correctly discover the type as a boolean, so navigate=false works as expected.

I'm pretty sure the code that does this is:

amphtml/src/service/action-impl.js

Lines 1403 to 1407 in 805651a

// Boolean literal.

if (convertValues && (s == 'true' || s == 'false')) {

const value = s == 'true';

return {type: TokenType.LITERAL, value, index: newIndex};

}

I also verified this empirically since I don't understand the parser just yet

dreamofabear · 2020-07-14T23:12:13Z

src/service/history-impl.js

    return this.enque_(() => {
-      if (this.stackIndex_ <= 0) {
-        // Nothing left to pop.
+      if (this.stackIndex_ <= 0 && !canPerformWindowBack) {


For HistoryBindingVirtual, do viewers face-plant if they receive a negative stackIndex param?

The SRP Viewer won't face-plant. I believe it'll just drop the request as invalid

dreamofabear · 2020-07-14T23:16:39Z

Oh and remember to update documentation. :)

…#26585) * history-impl: parameterize goBack * fixed test name * always use the binding * fix tests * improve doc * back to AMP.windowBack instead. * restore canPerformWindowBack * also test noarg * update to parameterized func * checktypes, use quoted prop * will updates * keep enthusiastic boolean conversion, constrain to ==true

googlebot added the cla: yes label Jan 31, 2020

samouri marked this pull request as ready for review February 1, 2020 00:00

samouri requested a review from dreamofabear February 1, 2020 00:00

samouri self-assigned this Feb 1, 2020

samouri force-pushed the goback-samaurijack branch 2 times, most recently from 4b95ad5 to 966135b Compare February 3, 2020 21:48

dreamofabear requested a review from dvoytenko February 4, 2020 20:36

samouri force-pushed the goback-samaurijack branch from 966135b to e33090c Compare February 11, 2020 22:10

samouri changed the title ~~✨ Amp Actions: implement AMP.windowBack~~ ✨ Amp Actions: AMP.goBack action should call history.back() Feb 11, 2020

samouri changed the title ~~✨ Amp Actions: AMP.goBack action should call history.back()~~ ✨ Amp Actions: AMP.goBack action should call history.back Feb 11, 2020

samouri force-pushed the goback-samaurijack branch from 5c2c25f to 78f802e Compare February 14, 2020 16:28

dreamofabear reviewed Feb 24, 2020

View reviewed changes

src/service/history-impl.js Outdated Show resolved Hide resolved

src/service/history-impl.js Outdated Show resolved Hide resolved

samouri mentioned this pull request Mar 30, 2020

wg-runtime Status Update 2020-03-30 ampproject/wg-runtime#31

Closed

samouri force-pushed the goback-samaurijack branch from 78f802e to f03cb04 Compare June 8, 2020 21:31

samouri added the WG: runtime label Jun 8, 2020

samouri mentioned this pull request Jun 8, 2020

I2S: AMP.goBack to perform cross-document navigation #28751

Closed

samouri force-pushed the goback-samaurijack branch from cb063cd to 0c9138c Compare July 7, 2020 18:32

dreamofabear reviewed Jul 8, 2020

View reviewed changes

src/service/standard-actions-impl.js Outdated Show resolved Hide resolved

samouri force-pushed the goback-samaurijack branch from 1e75aec to 756cc60 Compare July 13, 2020 16:08

samouri added 3 commits July 14, 2020 16:48

history-impl: parameterize goBack

609881a

fixed test name

33c3c14

always use the binding

e9909f6

samouri added 7 commits July 14, 2020 16:48

fix tests

1884240

improve doc

07e00ad

back to AMP.windowBack instead.

95c955c

restore canPerformWindowBack

b7c8ce1

also test noarg

36d4fba

update to parameterized func

dbd5a7d

checktypes, use quoted prop

805651a

samouri force-pushed the goback-samaurijack branch from 756cc60 to 805651a Compare July 14, 2020 20:51

dreamofabear approved these changes Jul 14, 2020

View reviewed changes

samouri added 2 commits July 15, 2020 12:44

will updates

744e40d

keep enthusiastic boolean conversion, constrain to ==true

d90a30d

samouri merged commit 29dc250 into ampproject:master Jul 15, 2020

samouri deleted the goback-samaurijack branch July 15, 2020 19:15

dreamofabear mentioned this pull request Jul 20, 2020

wg-runtime Status Update 2020-07-20 ampproject/wg-runtime#39

Closed

ampprojectbot added the PR use: In Beta / Experimental label Jul 22, 2020

samouri mentioned this pull request Aug 2, 2021

Proper goback functionality in AMP #5225

Closed

	// Boolean literal.
	if (convertValues && (s == 'true' \|\| s == 'false')) {
	const value = s == 'true';
	return {type: TokenType.LITERAL, value, index: newIndex};
	}

Conversation

samouri commented Jan 31, 2020

Uh oh!

dreamofabear commented Feb 4, 2020

Uh oh!

dvoytenko commented Feb 6, 2020

Uh oh!

samouri commented Feb 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dvoytenko commented Feb 7, 2020

Uh oh!

dreamofabear commented Feb 11, 2020

Uh oh!

dvoytenko commented Feb 11, 2020

Uh oh!

samouri commented Feb 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

samouri commented Feb 14, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dreamofabear Jul 14, 2020

Choose a reason for hiding this comment

Uh oh!

samouri Jul 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dreamofabear Jul 14, 2020

Choose a reason for hiding this comment

Uh oh!

samouri Jul 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dreamofabear commented Jul 14, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

samouri commented Feb 6, 2020 •

edited

Loading

samouri commented Feb 11, 2020 •

edited

Loading

samouri Jul 15, 2020 •

edited

Loading

samouri Jul 15, 2020 •

edited

Loading