1197967 - output-parser.js rewrites url()

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Tom Tromey :tromey]

I noticed that output-parser.js can rewrite url()s.
In particular _appendURL adds quotes, even if there were none
before.

Comment 1

[Tom Tromey :tromey]

Attachment: preserve spelling of urls in output-parser.js

Comment 2

[Patrick Brosset <:pbro>]

Comment on attachment 8651953 [details] [diff] [review]
preserve spelling of urls in output-parser.js

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

::: browser/devtools/shared/test/browser_outputparser.js
@@ +135,5 @@
>  
>    target.innerHTML = "";
>  }
> +
> +function testParseURL(doc, parser) {

Can you add an info in here, so it's easy to follow where the test is when reading logs:

info("Testing that URL parsing preserve authored quotes");

@@ +143,5 @@
> +      urlClass: "test-urlclass"
> +    });
> +
> +    let target = doc.querySelector("div");
> +    ok(target, "captain, we have the div");

This part of the test isn't really about testing that the test div exists. Could you remove this assertion?

@@ +146,5 @@
> +    let target = doc.querySelector("div");
> +    ok(target, "captain, we have the div");
> +    target.appendChild(frag);
> +
> +    is(target.innerHTML, url, "URL correctly parsed");

And make sure the comment in this assertion is unique so it's easy to know which of the steps failed when reading logs:

is(target.innerHTML, url, "URL with " + quote + " quotes correctly parsed");

::: toolkit/devtools/output-parser.js
@@ +426,5 @@
>        this._appendNode("a", {
>          target: "_blank",
>          class: options.urlClass,
>          href: href
> +      }, matches[2]);

With this regexp (and doing some tests locally), I think the closing quote is going to be part of group 2 and therefore part of the <a> link.

url("test.png");
will result in
<text>url("</text><a>test.png"</a><text>)</text>

The closing quote should be part of group 3 instead.

Comment 3

[Tom Tromey :tromey]

Attachment: preserve spelling of urls in output-parser.js

That test case sure was bogus.  Here is a better one.  This patch also
fixes the problem you noticed in the regexp; plus some I found.

I considered changing CSSLexer to emit the sort of information we need
about URL tokens.  However, we'd still want to break the token apart
like this, so in the end I decided a regexp was ok, essentially
because we know that the token is already valid.

Comment 4

[Patrick Brosset <:pbro>]

Comment on attachment 8652952 [details] [diff] [review]
preserve spelling of urls in output-parser.js

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

::: toolkit/devtools/output-parser.js
@@ +414,5 @@
>      */
>    _appendURL: function(match, url, options={}) {
>      if (options.urlClass) {
> +      let [, leader, , body, trailer] =
> +        /^(url\([ \t\r\n\f]*(["']?))(.*?)(\2[ \t\r\n\f]*\))$/i.exec(match);

Can you add a comment before this crazy-looking regex explaining what it's about and summarizing what you said about using a regex in your last comment?

Comment 5

[Tom Tromey :tromey]

I belatedly realized that this doesn't handle bad_url tokens.
That makes everything much hairier :(

I'm considering three choices.

One is to just wrap the entire url() in the <a>.
This is simple and won't affect functionality.

Another is to punt on bad_url.

The final complicated one is to re-lex the bad_url token text,
then use the facility in bug 1196896 to get the implied EOF
characters to turn it into a good url.

Comment 6

[Tom Tromey :tromey]

Attachment: preserve spelling of urls in output-parser.js

Using a new lexer was easy to write and gets the correct results.  But
one might reasonably believe it is too complicated compared to some of
the simpler approaches suggested earlier.

It's worth noting that the CSS lexer doesn't in fact yield bad_url
tokens for the new test cases.  So, in one of the alternate plans, the
approach would have to be to look for regexp failure rather than
bad_url.

Comment 7

[Patrick Brosset <:pbro>]

Comment on attachment 8653673 [details] [diff] [review]
preserve spelling of urls in output-parser.js

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

Looks good to me. Just a suggestion, but R+ing this anyway.

::: toolkit/devtools/output-parser.js
@@ +416,3 @@
>      if (options.urlClass) {
> +      // Maybe the URL is incomplete.  Re-lex it and add any needed
> +      // termination characters so that our regexp will work.

This approach looks good to me, not too complex. I'd be nice if we could move this to a helper function (like sanitizeURL or something).
Of course this means that this function should also return eofChars.

@@ +433,5 @@
> +      let [, leader, , body, trailer] =
> +        /^(url\([ \t\r\n\f]*(["']?))(.*?)(\2[ \t\r\n\f]*\))$/i.exec(match);
> +      // If the URL needed some completing characters, remove them
> +      // here so that the result bears a close relationship to the
> +      // authored text.

I wonder if it'd be ok to keep the URL complete here? We wouldn't strictly show style as authored, but it would remove some code complexity and this use case is probably rare.

Comment 8

[Tom Tromey :tromey]

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #7)

> I wonder if it'd be ok to keep the URL complete here? We wouldn't strictly
> show style as authored, but it would remove some code complexity and this
> use case is probably rare.

I'll make the change.  I don't think it can hurt anything.

Comment 9

[Tom Tromey :tromey]

Attachment: preserve spelling of urls in output-parser.js

Updated per review comments and changes in bug 1196896.

Comment 10

[Tom Tromey :tromey]

Comment on attachment 8655638 [details] [diff] [review]
preserve spelling of urls in output-parser.js

Carrying over r+

Comment 11

[Tom Tromey :tromey]

Attachment: preserve spelling of urls in output-parser.js

Updated for requested changes in bug 1196896.

Comment 12

[Tom Tromey :tromey]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=62cb9f5a9950

Comment 13

[Carsten Book [:Tomcat]]

Hi, i think this may need a rebase for fx-team  because this failed to apply:

adding 1197967 to series file
renamed 1197967 -> Bug-1197967---preserve-spelling-of-urls-in-output-.patch
applying Bug-1197967---preserve-spelling-of-urls-in-output-.patch
unable to find 'browser/devtools/shared/test/browser_outputparser.js' for patching
3 out of 3 hunks FAILED -- saving rejects to file browser/devtools/shared/test/browser_outputparser.js.rej
unable to find 'toolkit/devtools/output-parser.js' for patching
1 out of 1 hunks FAILED -- saving rejects to file toolkit/devtools/output-parser.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and refresh Bug-1197967---preserve-spelling-of-urls-in-output-.patch

Comment 14

[Tom Tromey :tromey]

Attachment: preserve spelling of urls in output-parser.js

Rebased.

Comment 15

[Tom Tromey :tromey]

Comment on attachment 8666694 [details] [diff] [review]
preserve spelling of urls in output-parser.js

Oh, sorry about that.
I tested this rebased version but forgot to attach it to the bug.

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/ef5e10b8d3f0

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ef5e10b8d3f0