868930 - Relative paths in manifest should be relative to manifest file

Status: RESOLVED FIXED

(L20n :: HTML Bindings, defect, P2)

Description

[Matthias Mullie]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:20.0) Gecko/20100101 Firefox/20.0
Build ID: 20130326150557

Steps to reproduce:

Load a manifest on multiple different directory levels.


Actual results:

Did not work because of the relative paths to the l20n resources.
The resources are being loaded relative to the document, not to the manifest file, which makes it quite impossible to use a manifest file unless you'll only use it for files in the same folder.


Example setup:

/l20n.json
    {
      "languages": [
        ...
      ],
      "resources": [
        "{{lang}}.l20n"
      ]
    }

/index.html
    <link rel="localization" type="application/json" href="l20n.json" />


/test/index.html
    <link rel="localization" type="application/json" href="../l20n.json" />

Both files point to the same manifest, which has resources at the same level. What you'd expect is that both files result in having the same l20n applied.
Instead, the path to the loaded resources will look like this:
for /index.html: /en.l20n
for /test/index.html: /test/en.l20n


Expected results:

Relative paths to resources in a manifest should be relative the manifest file, not to the document that includes the manifest.

What I would have preferred was for the resources to be loaded like this:
for /index.html: /en.l20n
for /test/index.html: /en.l20n

Much like how relative paths in @import or url() in CSS are relative to the CSS file (not to how the document that loads the CSS file), relative paths in manifest file should be relative to the manifest file.

--

I won't attach a patch since my local setup (running https://github.com/l20n/l20n.min.js/blob/master/l20n.js) is not really up-to-date with the current l20n master.
You could use pretty much the same code as Resource::relativeToSelf (with the manifest url being the self.id used there).
Only "problem" however is that it would also have to apply this to non-{{lang}} resources too, so it would either have to calculate the full path immediately (add linkResource as string), or defer adding that path using a .bind (add linkResource as function) and no longer have the template/uri difference currently used to differentiate for _none usage.

--

The below code serves only as an example of what I'd like to accomplish. It's based on old code.

    function loadManifest(url) {
      var deferred = new L20n.Promise();
      L20n.IO.load(url, true).then(
        function(text) {
          var manifest = JSON.parse(text);
          var langList = L20n.Intl.prioritizeLocales(manifest.languages);
          ctx.registerLocales.apply(this, langList);

          var resource = function(manifestUrl, lang) {
            var url = manifest.resources[0].replace("{{lang}}", lang);
            if (url[0] == '/') {
              return url;
            }
            var dirname = manifestUrl.split('/').slice(0, -1).join('/');
            if (dirname) {
              // strip the trailing slash if present
              if (dirname[dirname.length - 1] == '/') {
                dirname = dirname.slice(0, dirname.length - 1);
              }
              return dirname + '/' + url;
            } else {
              return './' + url;
            }
          };
          ctx.linkResource(resource.bind(this, url));

          deferred.fulfill();
        }
      );
      return deferred;
    }

Comment 1

[Staś Małolepszy :stas]

Good catch.  Let's fix this in 1.0 beta.

Comment 2

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: patch

I decided to go different route than relativeToSelf because there is no risk of manifest url to be empty here.

Comment 3

[Staś Małolepszy :stas]

Comment on attachment 758763 [details] [diff] [review]
patch

Review of attachment 758763 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, but please see the comments below.

(In reply to Matthias Mullie from comment #0)
> Only "problem" however is that it would also have to apply this to
> non-{{lang}} resources too, so it would either have to calculate the full
> path immediately (add linkResource as string), or defer adding that path
> using a .bind (add linkResource as function) and no longer have the
> template/uri difference currently used to differentiate for _none usage.

This seems to have been mitigated by the fix from bug 868723:

https://github.com/l20n/l20n.js/commit/69a3885323600b3282667503387a22f66fb55e19

::: bindings/l20n/html.js
@@ +101,5 @@
>      });
>    }
>  
> +  function relativeToManifest(manifestUrl, url) {
> +    if (url[0] == '/') {

Do we want to allow absolute URLs with a protocol and a host in the manifest?

@@ +107,5 @@
> +    }
> +    var dirs = manifestUrl.split('/')
> +                          .slice(0, -1)
> +                          .concat(url.split('/'))
> +                          .filter(function(elem){return elem !== '.';});

Can you please format the filter function into three lines?

@@ +110,5 @@
> +                          .concat(url.split('/'))
> +                          .filter(function(elem){return elem !== '.';});
> +
> +    if (dirs[0] !== '' &&
> +        dirs[0] !== '..') {

Move this to a single line, please.

Comment 4

[Zibi Braniecki [:zbraniecki][:gandalf]]

(In reply to Staś Małolepszy :stas from comment #3)
> ::: bindings/l20n/html.js
> @@ +101,5 @@
> >      });
> >    }
> >  
> > +  function relativeToManifest(manifestUrl, url) {
> > +    if (url[0] == '/') {
> 
> Do we want to allow absolute URLs with a protocol and a host in the manifest?

sameOriginPolicy etc. I wouldn't worry about it for now.

Comment 5

[Zibi Braniecki [:zbraniecki][:gandalf]]

https://github.com/l20n/l20n.js/commit/b48534ef89a29b1d886845b29f63deadbc5093b1