#5505 closed bug (fixed)

Opened November 12, 2009 07:24PM UTC

Closed December 14, 2009 09:25PM UTC

hasClass doesn't work with carriage return or tab

Reported by:	batiste	Owned by:
Priority:	major	Milestone:	1.4
Component:	core	Version:	1.4a1
Keywords:		Cc:	yoan.blanc@gmail.com
Blocked by:		Blocking:

Description

className attribute in a element could contain carriage return or tab. jQuery hasClass doesn't work properly with them.

Here is a complete fix, with some doc, and tests integrated to the suite:

http://github.com/batiste/jquery

The performance impact should remain small because I just added a variable declaration and 2 replace call.

Attachments (1)

hasClass.patch (2.6 KB) - added by batiste November 16, 2009 08:58AM UTC.

Change History (15)

Changed November 12, 2009 10:40PM UTC by batiste comment:1

This bug is the same as :

http://dev.jquery.com/ticket/4761

It happens on every browser, no just IE.

Changed November 13, 2009 01:11AM UTC by pbcomm comment:2

This is not a bug. According to standards class names should not contain tabs or carriage returns.

Changed November 13, 2009 09:32AM UTC by batiste comment:3

pbcomm: Reference please?

I have made my homework :

http://www.w3.org/TR/html401/struct/global.html#h-7.5.2

And try to validate that :

<!DOCTYPE html>

<html>

<body>

<p class="toto

tata">

Changed November 13, 2009 09:46AM UTC by greut comment:4

http://www.w3.org/TR/html4/sgml/dtd.html#coreattrs

  class       CDATA          #IMPLIED  -- space-separated list of classes --

I usually use /\\bclassName\\b/ to check it's presence but it might be slower than an much simpler indexOf like it done at the moment. \\b handles a class attributes containing tabs or carriage returns.

Changed November 13, 2009 09:49AM UTC by batiste comment:5

This is perfectly valid, working HTML. Css apply on these classes. jQuery hasClass implementation is incorrect. Please fix it.

<!DOCTYPE html> 
<html> 
<body>
<p class="toto
tata">

Changed November 13, 2009 10:00AM UTC by batiste comment:6

A more precise explanation, className is a cdata type. What the reference tells bout cdata type:

http://www.w3.org/TR/html4/types.html#type-cdata

CDATA is a sequence of characters 
from the document character set and may 
include character entities. User agents 
should interpret attribute values as follows:

 1. Replace character entities with characters,
 2. Ignore line feeds,
 3. Replace each carriage return or tab with a single space.

The User agent interpret the attribute value like that. And jQuery need to do the same thing. The attribute stay unchanged by the browser so jQuery has to do the same work too.

Changed November 13, 2009 10:54AM UTC by batiste comment:7

Add a test for dotted class name :

http://github.com/batiste/jquery/commit/ca1c88e3884ef3d234f2d6a935aa7390f540a7e7

Changed November 13, 2009 11:40AM UTC by batiste comment:8

Fix removeClass : http://github.com/batiste/jquery/commit/4284da32c746622737ff96a6ed2ba7a24fe1fbf1

Changed November 14, 2009 03:40PM UTC by dmethvin comment:9

See also #3680; browsers may trim leading/trailing space in the className CDATA before it gets to jQuery.

Changed November 14, 2009 03:58PM UTC by dmethvin comment:10

I couldn't find an explicit W3C reference to newlines in class attributes, other than the one I mentioned in #3680. The section batiste referenced says, "For some HTML 4 attributes with CDATA attribute values, the specification imposes further constraints on the set of legal values for the attribute that may not be expressed by the DTD."

In the section greut references, it says "Multiple class names must be separated by white space characters." Since CR and LF are white space it sounds like they should be allowed in className.

Changed November 15, 2009 11:23AM UTC by batiste comment:11

The few modifications I have done fix all the mentionned bugs.

If applied #5505 #3680 and #4761 could be fixed completly.

Please let me know what is the process for pushing my github branch into the trunk.

Cheers,

Batiste

Changed November 16, 2009 09:03AM UTC by batiste comment:12

I said that my fix will help with #3680 but it's not true because I only fixed hasClass and removeClass.

I have not looked at all if there was a similar issue with selectors.

Changed November 24, 2009 03:29PM UTC by batiste comment:13

My patch is actually weird but seems proper to me...

Would be probably better to fix Sizzle?

For example something like that :

$('<div class="foo" />').hasClass('.foo');

Is seems to work with 1.3.2, but is debatable if it should work or not... This seems more correct:

$('<div class="foo" />').hasClass('foo');

Changed December 14, 2009 09:50AM UTC by batiste comment:14

Link to the latest commit:

http://github.com/batiste/jquery/commit/e379815c095cc971a37f751e2b7f73b4b09c694b

Changed December 14, 2009 09:25PM UTC by john comment:15

component:	unfilled → core
resolution:	→ fixed
status:	new → closed
version:	1.3.2 → 1.4a1

Landed these fixes in two commits:

http://github.com/jquery/jquery/commit/649024909d376032e6e9c41f209182d584e51043

http://github.com/jeresig/sizzle/commit/40a9507edae124fdce8fa8760592df1870ec27be

Thanks for the patches and test cases!

Side navigation