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-script overzealously terminates workers #26960

Closed
mdmower opened this issue Feb 25, 2020 · 6 comments · Fixed by #27108
Closed

amp-script overzealously terminates workers #26960

mdmower opened this issue Feb 25, 2020 · 6 comments · Fixed by #27108

Comments

@mdmower
Copy link
Contributor

mdmower commented Feb 25, 2020

What's the issue?

If an amp-script contains actions that require a user gesture to complete, and too long of a delay occurs between user gestures, the worker is terminated.

How do we reproduce the issue?

A minimal example is available at ampdemo.cmphys.com/worker-death/amp-script-worker-death.html.

  1. Type a character in the input box
  2. Wait for at least 5 seconds
  3. Press tab to defocus the input

Console errors report the worker has been terminated:

[amp-script] amp-script[script="echo-script"].js was terminated due to illegal mutation.
[amp-script] Dropped 1x "ATTRIBUTES" mutation(s); user gesture is required with [layout=container].
[amp-script] Dropped 2x "CHILD_LIST" mutation(s); user gesture is required with [layout=container].

What browsers are affected?

Tested on Chrome 80: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/80.0.3987.122 Safari/537.36

Which AMP version is affected?

2002200031230

@morsssss
Copy link
Contributor

Oh, that's an interesting case.

Have you tried this with an with layout != 'container' and with height and width set?

@samouri
Copy link
Member

samouri commented Feb 25, 2020

Thanks for finding this! Definitely a bug. Seems like the input events are counting as user gesture but the change event isn't. The specific breaking case is:

When the element loses focus after its value was changed, but not commited (e.g., after editing the value of <textarea> or ).

We'll need to either count dom blur or all change events as user gesture.

@morsssss, this issue (and all terminations) only affect layout=container cases.

@armanfeyzi
Copy link

I have an issue like @mdmower sample!
In my case, I want to loop over some element and remove/add attributes but I get below error:

[amp-script] Dropped 7x "ATTRIBUTES" mutation(s); user gesture is required with [layout=container].

A sample of this error was available at https://behnazar.com/%DA%A9%D8%B1%D9%85-%D9%88%DB%8C%D8%AA%D8%A7%D9%85%DB%8C%D9%86-%D8%B3%DB%8C/?amp=b1

Here is the JS script I wrote:

(function(){

    console.log(1);
    
    var navElements = document.getElementsByClassName('product_widget_content');

    console.log(navElements);
    
    navElements.forEach(function(element){

        element.getElementsByClassName("off-wrp")[0].classList.remove("hidden");
        
    });
    

})();

What's is a problem? and how I can fix that?

@kristoferbaxter
Copy link
Contributor

The following snippet is at issue.

navElements.forEach(function(element){
  element.getElementsByClassName("off-wrp")[0].classList.remove("hidden");
});

To modify the classList of these elements, you'll need the user to perform an interaction, then change the values in response to the interaction.

@armanfeyzi
Copy link

@kristoferbaxter Thanks bro
I try with Click event and now script working.
But I want to 100% load my script after load content! actually, I want to load the latest product price by ajax request.
When replace click event with touchstart or touchmove even I got the same error in the console
I think it's a bad idea to tell the user "For view product price you should click this button" or something like this.

@samouri
Copy link
Member

samouri commented Mar 11, 2020

@armanfeyzi: the only way to let amp-script perform updates without a user interaction is if you make its sized fix. For examples you would need to set layout=fixed and then explicitly specify width and height.

Most publishers have been able to accomplish much of their ajax needs via amp-list though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants