Bug 33725 - .selectorText and .cssText on CSSRule don't escape '.' and ':'
Summary: .selectorText and .cssText on CSSRule don't escape '.' and ':'
Status: RESOLVED DUPLICATE of bug 34529
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Yuzo Fujishima
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2010-01-15 10:58 PST by David Kilzer (:ddkilzer)
Modified: 2010-05-17 22:35 PDT (History)
5 users (show)

See Also:


Attachments
Test case (447 bytes, text/html)
2010-01-15 10:58 PST, David Kilzer (:ddkilzer)
no flags Details
Fix for Bug 33725 - .selectorText and .cssText on CSSRule don't escape '.' and ':' (3.82 KB, patch)
2010-05-11 02:17 PDT, Yuzo Fujishima
no flags Details | Formatted Diff | Diff
Fix for Bug 33725 - .selectorText and .cssText on CSSRule don't escape '.' and ':' (3.85 KB, patch)
2010-05-11 03:12 PDT, Yuzo Fujishima
no flags Details | Formatted Diff | Diff
Fix for Bug 33725 - .selectorText and .cssText on CSSRule don't escape '.' and ':' (3.86 KB, patch)
2010-05-13 19:36 PDT, Yuzo Fujishima
darin: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Kilzer (:ddkilzer) 2010-01-15 10:58:28 PST
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.
Comment 1 David Kilzer (:ddkilzer) 2010-01-15 10:58:46 PST
<rdar://problem/6348333>
Comment 2 Yuzo Fujishima 2010-05-11 02:17:59 PDT
Created attachment 55680 [details]
Fix for Bug 33725 - .selectorText and .cssText on CSSRule don't escape '.' and ':'
Comment 3 Early Warning System Bot 2010-05-11 02:23:24 PDT
Attachment 55680 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/2183130
Comment 4 Yuzo Fujishima 2010-05-11 03:12:59 PDT
Created attachment 55684 [details]
Fix for Bug 33725 - .selectorText and .cssText on CSSRule don't escape '.' and ':'
Comment 5 Yuzo Fujishima 2010-05-11 21:34:49 PDT
Fixed Qt compilation error.
Comment 6 Shinichiro Hamaji 2010-05-13 02:30:10 PDT
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.
Comment 7 Yuzo Fujishima 2010-05-13 03:29:19 PDT
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.
Comment 8 Shinichiro Hamaji 2010-05-13 04:13:13 PDT
> 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.
Comment 9 Yuzo Fujishima 2010-05-13 19:36:55 PDT
Created attachment 56045 [details]
Fix for Bug 33725 - .selectorText and .cssText on CSSRule don't escape '.' and ':'
Comment 10 Yuzo Fujishima 2010-05-13 19:38:28 PDT
(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 11 Darin Adler 2010-05-13 20:33:38 PDT
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 12 Darin Adler 2010-05-13 20:39:32 PDT
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.
Comment 13 Yuzo Fujishima 2010-05-17 22:35:34 PDT
http://dev.w3.org/csswg/cssom/#serializing-selectors defines how to serialize selectors.

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