Opened 8 years ago

Closed 7 years ago

Last modified 7 years ago

#13323 closed defect (wontfix)

dojo.(add|remove|toggle|has)Class doesn't handle newline delimiters

Reported by: Andrew Muraco Owned by: Eugene Lazutkin
Priority: high Milestone: tbd
Component: HTML Version: 1.6.1
Keywords: Cc:
Blocked By: Blocking:

Description (last modified by bill)

The className string could have newline or other non-printable characters that delimit the classes applied, but these functions only work if the classes are delimited by a space.

Code for removeClass (dojo.js/line 7041): cls = cls.replace(" " + classStr[i] + " ", " ");

Example A className on the element of "foo\n bar" gets recognized by the browser as the classes "foo" and "bar", but doing dojo.removeClass(e,"foo"); will not remove "foo" from the class list.

Possible fixes:

  1. Use Regex to do the replacement (use the \s character class).
  2. When the dom is initially rendered or loaded, all classNames get normalized by removing non-space whitespace characters.

Both fixes are likely to carry a performance penalty.

Change History (9)

comment:1 Changed 8 years ago by Andrew Muraco

To clarify, I'm using Chrome 12.

comment:2 Changed 8 years ago by bill

Component: GeneralHTML
Description: modified (diff)
Owner: set to Eugene Lazutkin

Or of course (3) don't support that. I don't know why the initial DOM would have newlines (or tab characters) in the class names for DOMNodes.

comment:3 Changed 8 years ago by Andrew Muraco

That's the obvious work around, but why couldn't a DOM node have newlines or tabs?

In-line JSP syntax sometimes makes it easier to do something like this:

<div class="<c:if test="true">class1</c:if>
            <c:if test="true">class2</c:if>"></div>

Why isn't that valid, at least Chrome will recognize class1 and class2 properly, so why shouldn't dojo?

comment:4 Changed 8 years ago by Douglas Hays

The w3c spec says that newlines and tabs are valid delimiters

comment:5 Changed 8 years ago by Douglas Hays

dijit might be able to minify the problem by tweaking _WidgetBase to intercept newlines/tabs and converting them to spaces using replace(/\s+/g,' ') for class attributes being copied from HTML source, assuming that programmatic use cases will use spaces only.

comment:6 in reply to:  5 Changed 8 years ago by Eugene Lazutkin

Owner: changed from Eugene Lazutkin to bill

Replying to doughays:

dijit might be able to minify the problem by tweaking _WidgetBase to intercept newlines/tabs and converting them to spaces using replace(/\s+/g,' ') for class attributes being copied from HTML source, assuming that programmatic use cases will use spaces only.

A suspect that the option proposed by Doug is the least invasive one. All sure proof remedies in dom/class module I can imagine carry a significant penalty, or at least appear to be complex. OTOH, it would be nice to support the spec.

Let's study the problem more thoroughly, run some benchmarks, but for now let's use the fix in Dijit code as proposed by Doug.

comment:7 Changed 8 years ago by bill

Owner: changed from bill to Eugene Lazutkin

I really don't see what this has to do with dijit. In the example given above:

<div class="<c:if test="true">class1</c:if>
            <c:if test="true">class2</c:if>"></div>

It's not even using dijit, and dijit might not even be loaded on the page.

So, a better workaround to this issue/limitation, short of not putting newlines in your class strings to begin with, would be to adjust the node classes on page load, like:

dojo.ready(function(){
   dojo.query("*").forEach(function(node){
      node.className = node.className.replace(/\s/g, " ");
   });
):

Anyway, Eugene, giving this back to you but you can feel free to close if you find that fixing the limitation is not worth the performance degradation.

comment:8 in reply to:  7 Changed 7 years ago by Eugene Lazutkin

Resolution: wontfix
Status: newclosed

Replying to bill:

So, a better workaround to this issue/limitation, short of not putting newlines in your class strings to begin with, would be to adjust the node classes on page load, like:

dojo.ready(function(){
   dojo.query("*").forEach(function(node){
      node.className = node.className.replace(/\s/g, " ");
   });
):

Anyway, Eugene, giving this back to you but you can feel free to close if you find that fixing the limitation is not worth the performance degradation.

Dojo is in business since 2004, yet this is the only complaint about us not supporting newlines and tabs as class delimiters. To me it means that while the case is totally valid, it is extremely rare. Other toolkits basically do what we do.

Providing that there is a simple workaround (see above), and in order to avoid run-time penalties in most I decided to close the ticket without modifying the base.

PS: We actually do not support many other things. For example, we do not support removing duplicated class names on the same node:

<div class="abc hello abc"></div>

Some browsers remove the duplication, some don't. But dojo.removeClass() removes the first occurrence only. Of course, we can add a loop for removing possibly duplicated class names, but it would hurt the overall performance just to cater to an extremely rare case.

I remember that somebody actually complained about this behavior of dojo.removeClass() once telling that duplicated classes were generated by some server-side framework, but apparently it was easy enough to fix the problem at its source rather than to add more code to Dojo Base.

comment:9 Changed 7 years ago by Andrew Muraco

I 100% agree that its not worth fixing given the implications, and usually server side code should be fixed. But its always good to document these corner cases ( particularly in the wiki for dojo/ dom-class) so users dont have to hunt down such a minor inconsistency.

Note: See TracTickets for help on using tickets.