Bug 121592 – Fixed img src URLS with multiple spaces

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yoav Weiss]

Fixed img src URLS with multiple spaces

[Yoav Weiss]

Created attachment 212039 [details]
Patch

[Romain Perier]

This is better to put the context of the issue in the bug itself instead of the changelog, imho.


Context:
simplifyWhiteSpace seems to be no longer required for the src attribute. As described by Yoav, the method might remove spaces in the image name (so truncate it).

Remark:
As the test output is dumped as text, it's useless to add an "expected png" :)

Dean, that is you who added simplifyWhiteSpace when you landed my first patch, right ? what do you think ?

[Yoav Weiss]

Romain,

You're absolutely right regarding the context. I should have added a comment.

As you said, current simplifyWhiteSpace step prevents URLs with multiple spaces from loading, and doesn't seem to be necessary (removing it doesn't break any tests, or any functionality that I'm aware of).

The reason I added the PNG is that my test is based on measuring the layout dimensions of the loaded PNG. Any PNG would do, but the test needs something there.

[Alexey Proskuryakov]

Comment on attachment 212039 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=212039&action=review

Some drive by comments below. I don't now enough about srcset to review.

> Source/WebCore/html/parser/HTMLParserIdioms.cpp:408
> +        image.imageURL = decodeURLEscapeSequences(srcAttribute);

Is decodeURLEscapeSequences needed? Generally, relative URLs from HTML attributes should not be touched in any way other than completeURL().

> LayoutTests/ChangeLog:12
> +        * fast/loader/resources/image  space.png: Added.

It woud be nice if the image wasn't pink/red. Having any red in test results implies a failure, see e.g. <http://www.w3.org/Style/CSS/Test/guidelines.html>.

> LayoutTests/fast/loader/image-src-multiple-space.html:8
> +    if (window.testRunner) {
> +        testRunner.dumpAsText();
> +    }
> +

This is unnecessary and should be removed, js-test-pre.js calls dumpAsText().

[Yoav Weiss]

(In reply to comment #4)
> (From update of attachment 212039 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=212039&action=review
> 
> Some drive by comments below. I don't now enough about srcset to review.
> 
> > Source/WebCore/html/parser/HTMLParserIdioms.cpp:408
> > +        image.imageURL = decodeURLEscapeSequences(srcAttribute);
> 
> Is decodeURLEscapeSequences needed? Generally, relative URLs from HTML attributes should not be touched in any way other than completeURL().

It is not needed, and causes harm as seen at https://bugs.webkit.org/show_bug.cgi?id=121609
If I'd seen it earlier, I would have merged the 2 into a single issue.
I can still extend this one to include both patches if necessary.

> 
> > LayoutTests/ChangeLog:12
> > +        * fast/loader/resources/image  space.png: Added.
> 
> It woud be nice if the image wasn't pink/red. Having any red in test results implies a failure, see e.g. <http://www.w3.org/Style/CSS/Test/guidelines.html>.

I wasn't aware of that. I'll modify it.
> 
> > LayoutTests/fast/loader/image-src-multiple-space.html:8
> > +    if (window.testRunner) {
> > +        testRunner.dumpAsText();
> > +    }
> > +
> 
> This is unnecessary and should be removed, js-test-pre.js calls dumpAsText().
OK. Will do.

[Alexey Proskuryakov]

> I can still extend this one to include both patches if necessary.

I think that separate patches would be better.

[Romain Perier]

Please don't touch to decodeURLEscapeSequences, a data uri scheme might contain escaped characters, this is the spec. See my comments about that in the following bug https://bugs.webkit.org/show_bug.cgi?id=119423

[Yoav Weiss]

(In reply to comment #7)
> Please don't touch to decodeURLEscapeSequences, a data uri scheme might contain escaped characters, this is the spec. See my comments about that in the following bug https://bugs.webkit.org/show_bug.cgi?id=119423

As you can see in https://bugs.webkit.org/show_bug.cgi?id=121609 , decodeURLEscapeSequences is currently breaking existing content of the form of <img src="foo%3Fbar.gif">
OTOH, you have encoded data URIs, which aren't supported in <img src> in neither Gecko nor Blink (and weren't supported in WebKit before the srcset patch), for which there's no existing content you may break.

If you believe encoded data URIs should be supported, they should be supported for all URLs, not only for img, and not with the price of breaking existing content.

[Romain Perier]

So why does the example at the end of the bug https://bugs.webkit.org/show_bug.cgi?id=119423 work with chrome 29 ?

[Yoav Weiss]

(In reply to comment #9)
> So why does the example at the end of the bug https://bugs.webkit.org/show_bug.cgi?id=119423 work with chrome 29 ?

As far as I understand it, the reason data URIs needed encoding after the introduction of srcset was to encode the comma that follows their type (since the algorithm split the attribute on commas). That kind of encoding was not supported for src before that, and is not supported in Chrome/Firefox.

In any case, this discussion doesn't belong to this bug, but to https://bugs.webkit.org/show_bug.cgi?id=121609

[Benjamin Poulain]

(In reply to comment #10)
> (In reply to comment #9)
> > So why does the example at the end of the bug https://bugs.webkit.org/show_bug.cgi?id=119423 work with chrome 29 ?
> 
> As far as I understand it, the reason data URIs needed encoding after the introduction of srcset was to encode the comma that follows their type (since the algorithm split the attribute on commas). That kind of encoding was not supported for src before that, and is not supported in Chrome/Firefox.
> 
> In any case, this discussion doesn't belong to this bug, but to https://bugs.webkit.org/show_bug.cgi?id=121609

It was an odd place to decode the input anyway. We can decode data URL later in the stack.

Can you please update the test and rebaseline after #121609?

[Yoav Weiss]

Created attachment 212133 [details]
Patch

[Yoav Weiss]

> 
> Can you please update the test and rebaseline after #121609?

Updated and rebased

[Yoav Weiss]

Created attachment 212167 [details]
Patch

[Yoav Weiss]

(In reply to comment #14)
> Created an attachment (id=212167) [details]
> Patch

Rebased again.

[WebKit Commit Bot]

Comment on attachment 212167 [details]
Patch

Clearing flags on attachment: 212167

Committed r156186: <http://trac.webkit.org/changeset/156186>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.