Escape list prefix, suffix and bullet chars in the CSS rules #550

peitschie · 2014-04-17T09:09:23Z

Various browser cope differently with unescaped characters in a CSS content block, though all browsers agree it is a bad thing. WebKit will throw an error, Chrome & FF will "guess" where the rule ends (and usually get it wrong).

This patch does 2 things to fix this:

Consistently use double-quotes to quote strings
Escape backspace and double-quote chars in the CSS-related content

The vast amount of testing code is primarily creating a string tokenizer able to parse out the displayed text across Chrome/WebKit + FF. As there is no simple way to get the "rendered text" from a pseudo-element, a lot of code is required to compute this. There are tests included for the hand-rolled tokenizer as well to ensure proper function.

No tests have been added for OdfCanvas, as the existing list logic in here is destined (sooner rather than later) to be merged into the newer list CSS translation class.

2 Additional tweaks included in this patch

Remove some try-catch blocks that immediately rethrow the caught exception
Remove unused webodf/tests/test_odt directory

kogmbh-ci · 2014-04-28T07:51:22Z

Build succeeded.
Refer to this link for build results: http://ci.kogmbh.com/jenkins/job/WebODF-PullReq/1542/

kossebau · 2014-04-29T16:10:44Z

webodf/tests/visual-tests.odt

@@ -0,0 +1,674 @@
+<?xml version="1.0" encoding="UTF-8"?>


visual-tests.odt -> visual-tests.fodt, as fodt is the usual suffix for flat ODT files

peitschie · 2014-04-30T05:48:47Z

I've also dropped commit 4045c79 (fixing the tests in FF) in preference to the simpler fix in PR #564.

kogmbh-ci · 2014-04-30T05:53:45Z

Build succeeded.
Refer to this link for build results: http://ci.kogmbh.com/jenkins/job/WebODF-PullReq/1548/

kossebau · 2014-04-30T10:50:45Z

webodf/lib/odf/OdfCanvas.js

+            // Content needs to be on a new line if it contains slashes due to a bug in older versions of webkit
+            // E.g., the one used in the qt runtime tests - https://bugs.webkit.org/show_bug.cgi?id=35010
+            content = '"' + escapeCSSString(prefix) + '"\n';
+        }

        if (stylemap.hasOwnProperty(style)) {
            content += " counter(list, " + stylemap[style] + ")";
        } else if (style) {
            content += "'" + style + "';";


"'" -> '"' here as well, no?

These are unimportant. The specs allow both double and single quoted strings.. the main reason I changed the others to double-quoted was so I only had to escape a single type of quote. I couldn't think of any harm caused by leaving this alone.

Sure. But in the PR text you say "Consistently use double-quotes to quote strings" :) So it is rather "More consistenly" ? Also is not consistent with the copy in ListStylesToCss.
Hm. How quick will #565 be done?

Hmm. Good points. Am I still allowed to land this PR if it is only "more" consistent? ^_^.

#565 is @satsuper's primary goal. The code is ready, and he's just fighting with creating some decent unit tests for it. So I expect it to land within the next week or two (assuming you have the spare bandwidth to review of course!).

This directory has never been used, even within the commit that added it

These effectively act as slightly expensive no-ops. There is no need for them to be present.

Various browser cope differently with unescaped characters in a CSS content block, though all browsers agree it is a bad thing. WebKit will throw an error, Chrome & FF will "guess" where the rule ends (and usually get it wrong). This patch does 2 things to fix this: - Consistently use double-quotes to quote strings - Escape backspace and double-quote chars in the CSS-related content The vast amount of testing code is primarily creating a string tokenizer able to parse out the displayed text across Chrome/WebKit + FF. As there is no simple way to get the "rendered text" from a pseudo-element, a lot of code is required to compute this. There are tests included for the hand-rolled tokenizer as well to ensure proper function. No tests have been added for OdfCanvas, as the existing list logic in here is destined (sooner rather than later) to be merged into the newer list CSS translation class.

Content needs to be on a new line if it contains slashes due to a bug in older versions of webkit. E.g., the one used in the qt runtime tests. See https://bugs.webkit.org/show_bug.cgi?id=35010 In addition, asking for the value of a content block that contains a counter returns a strange counter string such as "counter(-879878e564)". I can't find a bug report for this, but it only occurs inside the qt runtime tests.

kogmbh-ci · 2014-04-30T11:45:34Z

Build succeeded.
Refer to this link for build results: http://ci.kogmbh.com/jenkins/job/WebODF-PullReq/1550/

kossebau · 2014-04-30T12:37:32Z

webodf/tests/odf/ListStyleToCssTests.js

+     *
+     * 2. joining commas (i.e., commas between strings) are removed completely (some browsers separate content parts with commas)
+     * 3. no whitespace trimming or normalization is performed
+     * 4. replace counter(.*) with the text "counter(...)". It seems qtruntimetests fail as the counters don't survive


Curious: Which version of Qt and which platform? Here for me (Qt 4.8.5 on OpenSuse) qtruntimetests passes if I go to variant with original counter content. It is just dropping the , between identifier and list-style-type (e.g. counter(list, decimal) becomes counter(list decimal)), which BTW also Firefox (28.0) and Chromium (33.0.1750.152) (both OpenSUSE) do.

http://www.w3.org/TR/CSS21/syndata.html#counter actually specifies counter(<identifier>, <'list-style-type'>), strange that all drop the ,.

The version used on the build server, and one I had on OSX (not sure the exact version). You can see the logs on the CI here: http://ci.kogmbh.com/jenkins/job/WebODF-PullReq/1499/consoleFull by searching for: counter(

(Just for simplicity)

<span>Running <span style='color:blue;cursor:pointer' onclick='runTest("ListStyleToCssTests","numberedListPrefixes")'>numberedListPrefixes</span></span> t.actualContent should be [a b] counter(list, decimal) []. Was [a b] counter(2.33059e-310) []. t.actualContent should be ['] counter(list, decimal) []. Was ['] counter(2.33059e-310) []. t.actualContent should be ["] counter(list, decimal) []. Was ["] counter(2.33059e-310) [].

It's my own hacked-up parsing logic doing that :). I made it do the proper tokenizing in 2c0f8a7, and then "unfixed" it in c9aadd8 as it re-simplified the logic.

Ah, CI might only have QtWebKit of Qt 4.8.5 (due to being self-compiled there and not from packages if I saw correcty), and not the newer https://gitorious.org/webkit/qtwebkit-23 (like most Linux distris now do). And same for the OSX version. And our build requirements do not check fo that. I see.

Hm, if those fixes are only done due to a pretty old webkit in normal Qt installations, which is nowhere else used in what we support, I propose to require the newer QtWebKit 2.3...
But that can/should be done in a separate PR.

It's my own hacked-up parsing logic doing that :).

"that" is what? That the , disappears? Cannot yet see where that would be done...

Yes, I was meaning it's normalizeCSSContent stripping the comma out. Check the difference in this function between commit 2c0f8a7 and c9aadd8 for hints. Long story short, the first commit treats a ( as the start of a string region.

Ah, in the test only. Hm, why do I continue to ignore the nice description in the API dox there :(
Okay.

kossebau · 2014-04-30T13:23:30Z

In general seems fine and improves things without regression, good work, so can be merged. Please ship :)

Escape list prefix, suffix and bullet chars in the CSS rules

peitschie · 2014-04-30T13:29:19Z

As per usual, thanks for the careful oversight :)

kossebau reviewed Apr 29, 2014
View reviewed changes

kossebau reviewed Apr 30, 2014
View reviewed changes

peitschie added 6 commits April 30, 2014 21:15

Remove unused webodf/tests/test_odt directory

130bedb

This directory has never been used, even within the commit that added it

Remove try-catch blocks that immediately rethrow the caught exception

c35b8c4

These effectively act as slightly expensive no-ops. There is no need for them to be present.

Log additional information if a stylesheet rule fails to insert

bdec312

Add test document demonstrating list escape character issues

0df2943

kossebau reviewed Apr 30, 2014
View reviewed changes

peitschie added a commit that referenced this pull request Apr 30, 2014

Merge pull request #550 from peitschie/css-injection

70031cf

Escape list prefix, suffix and bullet chars in the CSS rules

peitschie merged commit 70031cf into webodf:master Apr 30, 2014

peitschie deleted the css-injection branch April 30, 2014 13:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Escape list prefix, suffix and bullet chars in the CSS rules #550

Escape list prefix, suffix and bullet chars in the CSS rules #550

peitschie commented Apr 17, 2014

kogmbh-ci commented Apr 28, 2014

kossebau Apr 29, 2014

peitschie Apr 30, 2014

peitschie commented Apr 30, 2014

kogmbh-ci commented Apr 30, 2014

kossebau Apr 30, 2014

peitschie Apr 30, 2014

kossebau Apr 30, 2014

peitschie Apr 30, 2014

kogmbh-ci commented Apr 30, 2014

kossebau Apr 30, 2014

kossebau Apr 30, 2014

peitschie Apr 30, 2014

peitschie Apr 30, 2014

peitschie Apr 30, 2014

kossebau Apr 30, 2014

kossebau Apr 30, 2014

kossebau Apr 30, 2014

peitschie Apr 30, 2014

kossebau Apr 30, 2014

kossebau commented Apr 30, 2014

peitschie commented Apr 30, 2014

Escape list prefix, suffix and bullet chars in the CSS rules #550

Escape list prefix, suffix and bullet chars in the CSS rules #550

Conversation

peitschie commented Apr 17, 2014

kogmbh-ci commented Apr 28, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peitschie commented Apr 30, 2014

kogmbh-ci commented Apr 30, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kogmbh-ci commented Apr 30, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kossebau commented Apr 30, 2014

peitschie commented Apr 30, 2014