search

radkovo / jStyleParser

jStyleParser is a CSS parser written in Java. It has its own application interface that is designed to allow an efficient CSS processing in Java and mapping the values to the Java data types. It parses CSS 2.1 style sheets into structures that can be efficiently assigned to DOM elements. It is intended be the primary CSS parser for the CSSBox library. While handling errors, it is user agent conforming according to the CSS specification.
http://cssbox.sourceforge.net/jstyleparser/
GNU Lesser General Public License v3.0
92 stars 49 forks source link

Multiple classes for an element are in some cases improperly parsed #11

Closed gurevi closed 9 years ago

gurevi commented 9 years ago

jStyleParser is a quite useful tool and am very happy that it exists. Have been playing with it and learning a lot. Thanks!

That said, I did notice an interesting and potentially significant bug.

It seems in some cases, jStyleParser evaluates CSS classes based on the order they are in and can miss if an element has multiple classes that modify the same property.

I researched it a bit and found that the order of the classes should not matter, rather it should go by which style rule comes later in the .css file.

For instance, let’s say you have <div class ="red blue">. The red CSS style has {color:red;} and the blue has {color: blue;}. As is, the div will always be parsed as being red because it came first.

But the correct interpretation is, I believe, that it should be blue if the blue style rule comes later in the css.

This can be particularly significant with widths as shown below in a test case I came up with:

HTML file 

<html lang="en"> 
<head>
<link rel="stylesheet" type="text/css"  href="/testing/test.css">
</head>
<body>
<div class = "coltypea coltypeb"> Lorem ipsum <p class = "blue red"> test</p></div>
</body>
</html>

CSS File

.coltypea {width: 90%;}
.coltypeb {width: 10%;}
.red {font-size: 50px;}
.blue{font-size:12px;}

With this code, the potential bug can be seen in that both Firefox and Chrome interpret the width of the <div class = "coltypea coltypeb"> element to be 10% as that definition comes later in the CSS.

jStyleParser, however, seems to think that element has a width of 90%.

Not sure if it matters, but I am using TagSoup instead of NekoHTML to parse the HTML code.

hrj commented 9 years ago

@gurevi Good catch. Can confirm this with our parser + renderer as well. Will analyse this further by checking whether our DOM implementation is correct.

I have created a test-case here.

Will try to submit a unit-test later.

gurevi commented 9 years ago

@hrj Cool, thanks.

Am learning Java by experimenting with jStyleParser so fixing this is most likely a little beyond me (for now) but glad to be able to contribute.

hrj commented 9 years ago

Thinking a little more about this, I can't recollect any specification about mandatory ordering of rules in this case.

However, it might be desirable to follow other User-agents, to reduce breakage.

I will wait for @radkovo to chip-in before digging further.

radkovo commented 9 years ago

Hi, I mean @gurevi is right: the specification order is important and the latter specified value should win according to CSS specification: http://www.w3.org/TR/2011/REC-CSS2-20110607/cascade.html#cascading-order (item 4)

According to my tests, this bug does not occur in the latest release so it's probably caused by removing the priority strategy mechanism. But we can probably solve it without re-introducing the priority strategy, can we?

hrj commented 9 years ago

Oh, I completely mis-understood the issue then. I thought it was about the "order of classes" in the class attribute, rather than "order of rules" in the CSS because the title says: "Multiple classes for an element are in some cases improperly parsed".

I will analyse this and my priority related changes again.

radkovo commented 9 years ago

Yes, the order of classes in the class attribute should not be important. But according to my tests, it is important in current jStyleParser, e.g. class="red green" works differently from class="green red". Thank you for taking a look on this.

hrj commented 9 years ago

Ah ok, I understand the issue better now. I have simplified the test-case to illustrate the problem more explicitly:

<html> 
<head>
  <style>
    .red {font-size: 3em; color: red}
    .blue{font-size: 1em; color: blue}
  </style>
</head>
<body>
  <div>Lorem ipsum <p class = "red blue"> This should be blue and in same font size as lorem ipsum</p></div>
  <div>Lorem ipsum <p class = "blue red"> This should be blue and in same font size as lorem ipsum</p></div>
</body>
</html>

(Both the lines should render the same, but they are not the same with jStyleParser).

radkovo commented 9 years ago

Yes, that's exactly the issue.

radkovo commented 9 years ago

My experimental solution is in the ruleorder branch. It uses just a simple ordering in the Analyzer. @hrj Note that the order of style sheets passed to the Analyzer or DirectAnalyzer is now important as it corresponds to the CSS specification. However, they may be parsed in any order (in contrast to the original PriorityStrategy mechanism).

radkovo commented 9 years ago

I have pushed another re-implementation to a new ruleorder_tuples branch.

hrj commented 9 years ago

@radkovo we have shipped this in gngr v0.3 and everything seems to be working fine. I guess this issue can be closed.

radkovo commented 9 years ago

Great, thanks for info.

gurevi commented 9 years ago

Just tested this and the parser is now working in situations it hadn't before. Thanks!