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.
<rdar://problem/6348333>
Created attachment 55680 [details] Fix for Bug 33725 - .selectorText and .cssText on CSSRule don't escape '.' and ':'
Attachment 55680 [details] did not build on qt: Build output: http://webkit-commit-queue.appspot.com/results/2183130
Created attachment 55684 [details] Fix for Bug 33725 - .selectorText and .cssText on CSSRule don't escape '.' and ':'
Fixed Qt compilation error.
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.
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.
> 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.
Created attachment 56045 [details] Fix for Bug 33725 - .selectorText and .cssText on CSSRule don't escape '.' and ':'
(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.
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 "\\"?
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.
http://dev.w3.org/csswg/cssom/#serializing-selectors defines how to serialize selectors. *** This bug has been marked as a duplicate of bug 34529 ***