Bug 33725 – .selectorText and .cssText on CSSRule don't escape '.' and ':'

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: CSS

[David Kilzer (:ddkilzer)]

Created attachment 46690 [details]
Test case

* SUMMARY
According to the HTML4.01 Spec, an ID attribute _can_ contain the '.' and ':' characters. <http://www.w3.org/TR/html401/struct/global.html#h-7.5.2>
Example: <li id="item1.1">

They would be matched in CSS by #item1\.1 { color: red; }

However CSSRule.selectorText and CSSRule.cssText would return this string unescaped, so that one cannot disambiguate between:
<div id="hello.Text"> and <div id="hello" class="Text">

* STEPS TO REPRODUCE
1. Open attached test case.

* RESULTS
Test case fails.

* REGRESSION
Unknown, but probably not a regression.

[David Kilzer (:ddkilzer)]

<rdar://problem/6348333>

[Yuzo Fujishima]

Created attachment 55680 [details]
Fix for Bug 33725 - .selectorText and .cssText on CSSRule don't escape '.' and ':'

[Early Warning System Bot]

Attachment 55680 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/2183130

[Yuzo Fujishima]

Created attachment 55684 [details]
Fix for Bug 33725 - .selectorText and .cssText on CSSRule don't escape '.' and ':'

[Yuzo Fujishima]

Fixed Qt compilation error.

[Shinichiro Hamaji]

Comment on attachment 55684 [details]
Fix for Bug 33725 - .selectorText and .cssText on CSSRule don't escape '.' and ':'

WebCore/css/CSSSelector.cpp:585
 +              str += String(static_cast<const String&>(cs->m_value)).replace('.', "\\.").replace(':', "\\:");
I'm guessing cs->m_value.string().replace('.', "\\.").replace(':', "\\:") works. Could you check please?

It's a bit sad we don't have a convenient way to replace '.' and ':' at once, but maybe it's OK to land this code because IIRC selectorText() is rarely called.

[Yuzo Fujishima]

Hi,

Thank you for the review.

I am not sure if it is safe to change cs->m_value.
'.' and ':' should not be escaped everywhere else.

That's why I created a copy and replace the characters there.

[Shinichiro Hamaji]

> I am not sure if it is safe to change cs->m_value.
> '.' and ':' should not be escaped everywhere else.
> 
> That's why I created a copy and replace the characters there.

Ah sorry, I've forgot to copy the String. How about

String(cs->m_value.string()).replace('.', "\\.").replace(':', "\\:")

? I still slightly prefer this to static_cast.

[Yuzo Fujishima]

Created attachment 56045 [details]
Fix for Bug 33725 - .selectorText and .cssText on CSSRule don't escape '.' and ':'

[Yuzo Fujishima]

(In reply to comment #8)
> > I am not sure if it is safe to change cs->m_value.
> > '.' and ':' should not be escaped everywhere else.
> > 
> > That's why I created a copy and replace the characters there.
> 
> Ah sorry, I've forgot to copy the String. How about
> 
> String(cs->m_value.string()).replace('.', "\\.").replace(':', "\\:")
> 
> ? I still slightly prefer this to static_cast.

Agreed. :) Done.

[Darin Adler]

Comment on attachment 56045 [details]
Fix for Bug 33725 - .selectorText and .cssText on CSSRule don't escape '.' and ':'

Are these really the only two characters that need to be escaped? What about "\"? Doesn't that have to be escaped so it serializes as "\\"?

[Darin Adler]

Comment on attachment 56045 [details]
Fix for Bug 33725 - .selectorText and .cssText on CSSRule don't escape '.' and ':'

This is only scratching the surface of the characters that might need quoting. All other non-identifier characters need to be escaped too. This needs logic something like what we have in the quoteCSSString function in CSSParser.cpp. I think I'd call it escapeCSSIdentifierIfNeeded and put it in CSSParser.cpp alongside quoteCSSString and quoteCSSStringIfNeeded.

[Yuzo Fujishima]

http://dev.w3.org/csswg/cssom/#serializing-selectors defines how to serialize selectors.

*** This bug has been marked as a duplicate of bug 34529 ***