Bug 31593 – HTMLAnchorElement is inconsistent with its internal handling of the value returned for the href attribute.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Ben Murdoch]

During KURL::init(), trailing and leading spaces are removed from the URL. I think this step should also remove leading and trailing tabs, carriage returns and newlines.

Path and layout test to follow.

[Ben Murdoch]

Created attachment 43373 [details]
Proposed patch

Proposed patch

[Darin Adler]

Comment on attachment 43373 [details]
Proposed patch

There is insufficient context for this change. Does this affect websites? If so, how? What is the motivation for this change? Website incompatibilities? Behavior differences from other browsers?

This is too little test coverage. The new test covers only the newline character.

"I think this should change" is interesting, but not sufficient motivation for the change on its own.

[Ben Murdoch]

Hi Darin, apologies for the vagueness.

(In reply to comment #2)
> (From update of attachment 43373 [details])
> There is insufficient context for this change. Does this affect websites? If
> so, how? What is the motivation for this change? Website incompatibilities?
> Behavior differences from other browsers?

I discovered this issue while working on an Android browser bug. As I understand, currently it seems that if there is a leading newline in the href attribute of an <a> tag, that newline is only removed later by the deprecatedParseUrl() function (for example in HitTestResult::absoluteLinkURL() when the hit detection code is run). We do additional hit detection on Android to what is in WebCore, so I thought it made most sense to update KURLs behavior rather than making a call to a deprecated function in the Android code. As there is code to trim whitespace in KURL already, I thought it was sensible to augment that. I added trimming of tabs and carriage returns as I would consider them "whitespace" characters, too.

> 
> This is too little test coverage. The new test covers only the newline
> character.

I will fix this up and send a new patch if you are happy with the general idea of this patch.

Thanks, Ben

[Ben Murdoch]

+darin 

Sorry, forgot to add you on CC so you would know I replied :)

Thanks, Ben

[Darin Adler]

(In reply to comment #3)
> I discovered this issue while working on an Android browser bug. As I
> understand, currently it seems that if there is a leading newline in the href
> attribute of an <a> tag, that newline is only removed later by the
> deprecatedParseUrl() function (for example in HitTestResult::absoluteLinkURL()
> when the hit detection code is run). We do additional hit detection on Android
> to what is in WebCore, so I thought it made most sense to update KURLs behavior
> rather than making a call to a deprecated function in the Android code. As
> there is code to trim whitespace in KURL already, I thought it was sensible to
> augment that. I added trimming of tabs and carriage returns as I would consider
> them "whitespace" characters, too.

That's not the right way to fix this bug. To fix that bug should consistently use deprecatedParseUrl when constructing URLs from attribute values until we find a way to eliminate that function entirely.

Changing KURL would be OK to do if there are other arguments for doing so, but is not a good way to address that bug, since it addresses some differences between the code paths, but leaves other things different.

[Ben Murdoch]

(In reply to comment #5)

> That's not the right way to fix this bug. To fix that bug should consistently
> use deprecatedParseUrl when constructing URLs from attribute values until we
> find a way to eliminate that function entirely.
> 
> Changing KURL would be OK to do if there are other arguments for doing so, but
> is not a good way to address that bug, since it addresses some differences
> between the code paths, but leaves other things different.

OK, fair enough. In that case, would a patch to HTMLAnchorElement::href() to make it use deprecatedParseURL be acceptable (it currently does not)?

Thanks, Ben

[Darin Adler]

(In reply to comment #6)
> In that case, would a patch to HTMLAnchorElement::href() to
> make it use deprecatedParseURL be acceptable (it currently does not)?

Yes.

[Ben Murdoch]

Created attachment 43427 [details]
Alternative Patch.

Alternative patch as discussed above.

[Darin Adler]

You should retitle the bug to describe the problem seen by websites rather than your original thought on what to change to fix it.

[Darin Adler]

Comment on attachment 43427 [details]
Alternative Patch.

> +        When retrieving the href attribute of an A tag, we should run the attribute value through deprecatedParseURL until a good replacement for that function is found.

That's not really a bug title. It would be better to describe the problem and the real world symptom in both the bug and the change log.

This patch still seems too detached from what effect it's having on websites. There's not even a mention of what other browsers do or what HTML5 prescribes.

The key here is to do the same work in the href function that is done in HTMLAnchorElement::defaultEventHandler and HTMLAnchorElement::parseMappedAttribute. And I think that's what the comment should indicate. Consistency is the issue.

I would have liked to see more test cases about characters that should *not* be stripped, such as non-breaking spaces, \v, and the like.

r=me as is, although I'm not super-happy with how this is described in the change log

[Ben Murdoch]

Thanks for the review Darin.

I have updated the bug title and will edit the Changelogs on landing to match.

The HTML5 spec mandates that the href attribute must be a valid Web Address, which refers to the HTML5 Web Address spec and that states that when parsing a web address w, "Strip leading and trailing space characters from w." There doesn't appear to be a strict definition of "space characters", but running the layout test in Chrome, FF and IE it seems that they strip spaces, newlines, CR and tabs (as in they pass the test) and so this patch brings WebKit in line with that.

Thanks, Ben

[Ben Murdoch]

Comment on attachment 43427 [details]
Alternative Patch.

cq- so I can change the changelogs prior to landing.

[Darin Adler]

An even better way to handle this is would probably be to use [ReflectURL] in the IDL file and remove the HTMLAnchorElement::href() function entirely.

[Ben Murdoch]

(In reply to comment #13)
> An even better way to handle this is would probably be to use [ReflectURL] in
> the IDL file and remove the HTMLAnchorElement::href() function entirely.

Interesting, I wasn't aware of that. I will hold off landing and try it.

[Darin Adler]

(In reply to comment #14)
> Interesting, I wasn't aware of that. I will hold off landing and try it.

If any code is calling HTMLAnchorElement::href() directly rather than through DOM bindings, then we will still need to keep the function around. Generally, though, we want to synthesize these simple getters rather than hand-coding them. The one for URLs does indeed call deprecatedParseURL.

[Ben Murdoch]

(In reply to comment #15)
> If any code is calling HTMLAnchorElement::href() directly rather than through
> DOM bindings, then we will still need to keep the function around. Generally,
> though, we want to synthesize these simple getters rather than hand-coding
> them. The one for URLs does indeed call deprecatedParseURL.

OK. The IDL does already specify the ReflectURL property and the function is used elsewhere in the codebase (including the getters for other anchor properties such as hostname, which is how the layout test exposes the bug), so I think I'll land the patch as is (but update the Changelogs with the new bug title).

Thanks, Ben

[Ben Murdoch]

Landed as r51118.

[Ian 'Hixie' Hickson]

The term "space characters" is defined in the HTML5 spec.

[Darin Adler]

The following comment probably belongs in some other bug that's on the subject of our numerous whitespace-related functions:

We may be able to take advantage of HTML5 terminology and use the terms space characters and White_Space characters. Unfortunately, we normally do not use underscores in any of our identifiers, and since "skip whitespace" in HTML5 means "skip space characters", using "WhiteSpace" to mean Unicode White_Space characters might not work well for us.

Sadly, the isASCIISpace function in ASCIICType.h differs from both "space characters" or "White_Space characters". It includes U+000B, and HTML5 space characters does not. It is the subset of Unicode White_Space that fits in ASCII, so cannot be used if non-ASCII characters might be involved. It was named an designed based on the "C" locale version of the <ctype.h> isspace function.

We may find that isASCIISpace is never useful.

I'd love to clean this all up and unify based on a smaller set of characters. And make sure we have test cases.