Last Comment Bug 667518 - Don't trim white-space from <link> href attribute value
: Don't trim white-space from <link> href attribute value
Status: VERIFIED FIXED
[inbound]
: html5, regression
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla7
Assigned To: Mats Palmgren (:mats)
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks: 649134
  Show dependency treegraph
 
Reported: 2011-06-27 10:38 PDT by Mats Palmgren (:mats)
Modified: 2011-09-12 02:58 PDT (History)
8 users (show)
mats: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix (2.19 KB, patch)
2011-06-30 04:51 PDT, Mats Palmgren (:mats)
bzbarsky: review+
Details | Diff | Splinter Review

Description Mats Palmgren (:mats) 2011-06-27 10:38:27 PDT
Bug 649134 added trimming of white-space from <link> @href.

AFAICT, the "Editor's Response" in 
http://www.w3.org/Bugs/Public/show_bug.cgi?id=12508#c2
says we should not do that, and explicitly mentions that we should
load a non-empty white-space-only @href.
Comment 1 Boris Zbarsky [:bz] (still a bit busy) 2011-06-27 10:54:18 PDT
OK.  Want to fix?
Comment 2 Mats Palmgren (:mats) 2011-06-30 04:51:04 PDT
Created attachment 543101 [details] [diff] [review]
fix

Remove the whitespace Trim() from nsHTMLLinkElement::GetStyleSheetURL().

Not sure what to do about nsContentSink::ProcessLinkHeader() though.
Stripping WS there dates back to 1999 at least, so it seems too risky
to remove that at this point, although rfc 3986 does seem to suggest
it should NOT be stripped when quoted.  What do you think?

"Using <> angle brackets around each URI is especially recommended as
a delimiting style for a reference that contains embedded whitespace."
http://tools.ietf.org/html/rfc3986#appendix-C

http://tools.ietf.org/html/rfc5988#section-5
Comment 3 Henri Sivonen (:hsivonen) 2011-06-30 07:29:27 PDT
(In reply to comment #2)
> Not sure what to do about nsContentSink::ProcessLinkHeader() though.
> Stripping WS there dates back to 1999 at least, so it seems too risky
> to remove that at this point, although rfc 3986 does seem to suggest
> it should NOT be stripped when quoted.  What do you think?

I would guess it wouldn't be too risky to remove Link header support altogether (since the header isn't supported by all browsers), so it's probably safe to change space handling. Do we even know of any real-world uses of the Link header outside the sites of standards experts who work for browser vendors?
Comment 4 Boris Zbarsky [:bz] (still a bit busy) 2011-06-30 08:34:31 PDT
> contains embedded whitespace

I would read that as whitespace in the middle of the URI, not on the edges.  So I think trimming is fine.
Comment 5 Boris Zbarsky [:bz] (still a bit busy) 2011-06-30 08:35:12 PDT
Comment on attachment 543101 [details] [diff] [review]
fix

r=me
Comment 7 Marco Bonardo [::mak] 2011-07-01 09:06:43 PDT
backed out from inbound because the added test is permaorange on Android R2, please fix it or mark it as fail-if android on repush.
Comment 8 Mats Palmgren (:mats) 2011-07-02 14:55:22 PDT
Re-pushed with the test disabled for Android:
http://hg.mozilla.org/integration/mozilla-inbound/rev/f0d8fa54fecb
Comment 9 Marco Bonardo [::mak] 2011-07-04 05:01:05 PDT
http://hg.mozilla.org/mozilla-central/rev/f0d8fa54fecb
Comment 10 Ioana (away) 2011-09-12 02:58:23 PDT
Verified fixed on:
Mozilla/5.0 (Windows NT 5.1; rv:7.0) Gecko/20100101 Firefox/7.0
Mozilla/5.0 (Windows NT 6.1; rv:7.0) Gecko/20100101 Firefox/7.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:7.0) Gecko/20100101 Firefox/7.0
Mozilla/5.0 (X11; Linux i686; rv:7.0) Gecko/20100101 Firefox/7.0

Verified by loading beta reftests in the browser: 649134-1, 649134-2, 649134-2-ref.
649134-2 is displayed the same way as 649134-2-ref (moz - blue, ie - block surrounded by a red line), which means the href white-spaces are not trimmed. If the white-spaces were trimmed, 649134-2 would have been displayed the same way as 649134-1 (style specifications would be ignored).

This fix was verified the same way on the latest aurora and central builds.

Note You need to log in before you can comment on or make changes to this bug.