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

Initial values don't work #16

Closed hrj closed 7 years ago

hrj commented 9 years ago

I am not sure if the bug title is correct, but this test doesn't work:

http://test.csswg.org/suites/css2.1/20110323/html4/shand-border-000.htm

(the border color is not green).

The border property is defined with a short hand:

border: medium solid;

It looks like the border-color should be inferred as initial and then get assigned to the color of the element, which is set to green.

It is possible that we are not calling the analyser correctly in gngr, but I thought I would check with you first, since it seemed more probable in this case that it's an issue in jStyleParser.

Note that there are five more tests in similar vein here: http://test.csswg.org/suites/css2.1/20110323/html4/chapter-1.html

.. but they work fine. Only the border one fails.

radkovo commented 9 years ago

When the color is not specified the getProperty('background-color') call should return BackgroundColor.taken and getValue() should return null. jStyleParser does not automatically replace the missing value by the colo property value sine this would probably require an additional post-processing step after evaluating the complete DOM. However, I mean the client application can easily use the appropriate color when the background-color value is not defined. This works correctly in CSSBox, you may take a look at the corresponding code.

hrj commented 9 years ago

Thanks for the response; and yes it makes sense to do it separately if it requires another pass at the DOM.

Just wondering aloud; does it make sense to do this in the NodeData.concretize() call? The idea seems to be similar and will probably not require another DOM pass.

radkovo commented 9 years ago

That's a good commen, this seems to be possible. However, we would have to represent this information somehow, e.g. by extending the CSSProperty definition with the possibility of specifying the default value as a link to another property. This is generally possible but I am not sure if it is worth implementing such a mechanism just for this single case that can be easily solved in the application. Are you aware about any other similar case?

hrj commented 9 years ago

I have not come across another similar case yet. I guess we could start by checking all properties that can have initial value. I will try to enlist them, but may take time since I am not very familiar with it. If you can help find the list or any other help, please let me know.

hrj commented 9 years ago

There's another similar issue:

If the border width is not specified, it gets a value of initial. And this should be computed to 0 if the border-style is none, else, it should be medium.

So, there is atleast one other place where the computed value is going missing.

@radkovo It would be great if you could take a decision about this. Should jStyleParser compute such properties as part of concretize call, or should it be left to the client?

radkovo commented 9 years ago

I am sorry for the delay again. I have been thinking about a systematic solution. After reading the specs, it seems to me that the best solution would be to add support for the new CSS3 keywords, mainly initial, unset and currentColor. It seems that something has been already done for this but I have to review the current state. I'll try to take a look at that asap. As the temporary solution, I'd recommend to solve the mentioned few situations in the client application but the final solution should be provided by jStyleParser because it's a part of CSS3 specs.

hrj commented 9 years ago

Thanks for response @radkovo

We have started working around this in gngr for now. Looking forward to a permanent solution.

radkovo commented 7 years ago

After a long time (I am sorry for that), I have made some experiments with the support of initial, unset and currentColor keywords. You may find the result in the cascade3 branch.

Unfortunately, I had to break the backward compatibility because in CSS3, the transparent is treated as an ordinary color instead of a special keyword. That's why I removed the Transparent terms from CSSProperty and I extended the TermColor interface to provide the isTransparent() method and getKeyword() which is usable for testing the currentColor value. Finally, the NodeData interface provides an additional getColorValue() method that does the same as getValue(TermColor.class, ...) but it evaluates the currentColor value as well.

This modification should solve the situations when some of the above mentioned keywords is explicitly used or the border CSS property is used that implicitly defines border-color. However, it does not cover the situation when border-color is not defined at all (e.g. only border-style is defined); in this case, getValue() for border-color still returns null and it is the responsibility of the client software to use the initial value instead. This behavior is consistent across all the properties.

hrj commented 7 years ago

Thanks for the update.

However, it does not cover the situation when border-color is not defined at all (e.g. only border-style is defined); in this case, getValue() for border-color still returns null and it is the responsibility of the client software to use the initial value instead. This behavior is consistent across all the properties.

Will this be implemented as part of concretize() or would have to be implemented completely in client code? I have not been following the specs closely, so I am not sure if it makes sense, but putting it in concretize() would avoid code duplication in the clients.

radkovo commented 7 years ago

I still don't like the idea of putting it in concretize() too much; I find it strange to make this only for the single property (border-color) and it would be highly inefficient to concretize the values of all undefined properties by their initial values: it would fill the style map by a great number of default values for every DOM node and most of these values would not be used anyway.

So I have another proposal: we could generalize the getColorValue() method proposed above to someting like getSpecifiedValue() (according to the CSS3 terminology). This would work the same way as getValue() but the undefined properties would be replaced by their initial values and special keywords such as currentColor would be resolved as well. This solution would save space and time during style computation and would avoid code duplication in the clients as well. What do you think?

hrj commented 7 years ago

The concept of getSpecifiedValue() sounds fine. :+1:

radkovo commented 7 years ago

Ok, I have added the getSpecifiedProperty() and getSpecifiedValue() methods and the corresponding tests. It seems to work as expected.