Bug Tracker

Opened 11 years ago

Closed 11 years ago

#5505 closed bug (fixed)

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@…
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 11 years ago.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 11 years ago by batiste

This bug is the same as :

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

It happens on every browser, no just IE.

comment:2 Changed 11 years ago by pbcomm

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

comment:3 Changed 11 years ago by batiste

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">

comment:4 Changed 11 years ago by greut

​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.

comment:5 Changed 11 years ago by batiste

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">

comment:6 Changed 11 years ago by batiste

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.

comment:9 Changed 11 years ago by dmethvin

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

comment:10 Changed 11 years ago by dmethvin

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.

comment:11 Changed 11 years ago by batiste

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 11 years ago by batiste

Attachment: hasClass.patch​ added

comment:12 Changed 11 years ago by batiste

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.

comment:13 Changed 11 years ago by batiste

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');

comment:15 Changed 11 years ago by john

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!

Note: See TracTickets for help on using tickets.