ampproject / amphtml

The AMP web component framework.
https://amp.dev
Apache License 2.0
14.89k stars 3.89k forks source link

amp-script: Support location.href (don't return the blob URL) #26791

Open betosg opened 4 years ago

betosg commented 4 years ago

I am using AMP-SCRIPT. I want to get the current URL with parameter, but that what I get:

blob:http://localhost:8080/bcd8ce8b-4c8f-4ab0-8d4e-471d7be9af3f

That's what I want:

http://localhost:8080/?foo=bar

The HTML code:

<!DOCTYPE html>
<html ⚡>
    <head>
        <meta charset="utf-8" />
        <title>AMP-SCRIPT</title>
        <meta
            name="viewport"
            content="width=device-width,minimum-scale=1,initial-scale=1"
            />      
        <script type="application/ld+json">
            {
            "@context": "http://schema.org",
            "@type": "NewsArticle",
            "headline": "Article headline",
            "image": ["thumbnail1.jpg"],
            "datePublished": "2015-02-05T08:00:00+08:00"
            }
        </script>
        <script 
            async 
            custom-element="amp-script" 
            src="https://cdn.ampproject.org/v0/amp-script-0.1.js"
        ></script>            
        <style amp-boilerplate>
            body {
                -webkit-animation: -amp-start 8s steps(1, end) 0s 1 normal both;
                -moz-animation: -amp-start 8s steps(1, end) 0s 1 normal both;
                -ms-animation: -amp-start 8s steps(1, end) 0s 1 normal both;
                animation: -amp-start 8s steps(1, end) 0s 1 normal both;
            }
            @-webkit-keyframes -amp-start {
                from {
                    visibility: hidden;
                }
                to {
                    visibility: visible;
                }
            }
            @-moz-keyframes -amp-start {
                from {
                    visibility: hidden;
                }
                to {
                    visibility: visible;
                }
            }
            @-ms-keyframes -amp-start {
                from {
                    visibility: hidden;
                }
                to {
                    visibility: visible;
                }
            }
            @-o-keyframes -amp-start {
                from {
                    visibility: hidden;
                }
                to {
                    visibility: visible;
                }
            }
            @keyframes -amp-start {
                from {
                    visibility: hidden;
                }
                to {
                    visibility: visible;
                }
            }
        </style>
        <noscript
        ><style amp-boilerplate>
            body {
                -webkit-animation: none;
                -moz-animation: none;
                -ms-animation: none;
                animation: none;
            }
        </style></noscript
        >
        <script async src="https://cdn.ampproject.org/v0.js"></script>
    </head>
    <body>
    <amp-script layout="container" src="/javascript.js" sandbox="allow-forms">
        <button>Hello amp-script!</button>
        <span></span>
    </amp-script>
</body>
</html>

javascript.js

const btn = document.querySelector('button');
btn.addEventListener('click', () => {
  document.body.textContent = location.href;
});
morsssss commented 4 years ago

Looks like something that should be remedied!

betosg commented 4 years ago

Any predictions to correct this problem?

morsssss commented 4 years ago

@kristoferbaxter did give it a P2!

It might also be a simple way to start contributing... to submit your own PR.

betosg commented 4 years ago

@choumx Any news?

morsssss commented 4 years ago

@betosg, I know that @choumx and @kristoferbaxter really want to work on changes like this one, and I also know that they're both super busy. It's really an excellent opportunity to contribute to this project!

samouri commented 4 years ago

This does seem like it should be an easy fix. I'll take a stab at it either tomorrow or early next week.

CarlosCPC commented 4 years ago

Hi I have the same problem trying to get url query parameter from location.href inside amp-script Could you please help me to know if it is possible?

samouri commented 4 years ago

Could you please help me to know if it is possible?

@CarlosCPC: this is currently not possible.


Less of an easy fix than I initially thought. The worker location object is WAI in terms of how Workers usually behave. We could overwrite the global location as well as provide Document.location and keep them up to date with the main thread.

CarlosCPC commented 4 years ago

Thank you @samouri

To get a query parameter, any workaround could help me a lot, Could you please explain more about provide Document.location and keep them up to date with the main thread?

morsssss commented 4 years ago

@CarlosCPC , I think @samouri is talking about what WorkerDOM would need to do to make location available to the Worker - and thus to <amp-script>.

If you want to work to make this possible in <amp-script>, check out the WorkerDom repo. We highly encourage thoughtful contributions!

CarlosCPC commented 4 years ago

Thank you. I will try.

betosg commented 4 years ago

@samouri @CarlosCPC Were you able to solve this problem?

samouri commented 4 years ago

@betosg: No. Progress made on this issue will continue to be captured in this thread (unless moved to worker-dom in the future, but that will be called out here as well).

morsssss commented 4 years ago

Also, a similar request just appeared on Stack Overflow.

morsssss commented 4 years ago

Here's a case from the amp.dev team: https://github.com/ampproject/amp.dev/pull/4320

danieltxok commented 4 years ago

+1 on having this feature. Conditional (e.g. taking keyword from URL parameter) rendering of the CTA message in a page is something very useful that even has been proved to increase conversions.

rafaelmuttoni commented 3 years ago

I'm having this issue too. I'm trying to get the current URL with amp-script to pass it as query params in my amp-iframe src. Any other way to do it maybe? Opened a request at Stack Overflow with further information.

morsssss commented 3 years ago

@rafaelmuttoni I don't think there's a way at present. I answered your Stack Overflow question... I hope that helps at least a little!

In the meantime, thanks for commenting here; it helps us prioritize this work. Or let us know if you have time to contribute the code yourself. You'd be an AMP hero!

sdenaro commented 3 years ago

Has this been reviewed or discussed recently? I have an analytics use case that would be solved easily with a document.href being available

TusharTevetia-Hear commented 2 years ago

@morsssss Is there any progress made on this issue? I am trying to get the URL query params in my amp-script but I am getting empty string (window.location.search). Can I get the URL query params in any other way via amp-script?

morsssss commented 2 years ago

Unfortunately, I'm no longer active in the AMP community. I've actually moved to a different job :)

I wonder if you could get this info from the QUERY_PARAM variable described here, and pass that along to the amp-script?

sdenaro commented 2 years ago

@morsssss I haven't touched amp in a while but query_param didn't work for us we needed to inspect a url and extract something we could then pass to analytics. We wouldn't know for sure what params it may of may not include. This was the sharpest of edge cases.

stale[bot] commented 12 months ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

kietvt commented 5 months ago

I can't get current url inside script in AMP html also (see attached photo) amp-html-error

sayamalvi commented 5 months ago

One walkaround I came across which solved my use case is getting the query parameter in an inline javascript and storing it in local storage, then getting that parameter in amp-script. It is working for my use case as of now. Screenshot 2024-04-06 160608