Bug Tracker

Modify ↓

Ticket #5505 (closed bug: fixed)

Opened 4 years ago

Last modified 4 years ago

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@…
Blocking: Blocked by:

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

hasClass.patch Download (2.6 KB) - added by batiste 4 years ago.

Change History

comment:1 Changed 4 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 4 years ago by pbcomm

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

comment:3 Changed 4 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 4 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 4 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 4 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 4 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 4 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 4 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 4 years ago by batiste

comment:12 Changed 4 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 4 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 4 years ago by john

  • Status changed from new to closed
  • Resolution set to fixed
  • Version changed from 1.3.2 to 1.4a1
  • Component changed from unfilled to core

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!

Please follow the  bug reporting guidlines and use  jsFiddle when providing test cases and demonstrations instead of pasting the code in the ticket.

View ↑

Add a comment

Modify Ticket

Action
as closed
Author


E-mail address and user name can be saved in the Preferences.

Attachments ↑
 
Note: See TracTickets for help on using tickets.