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

font-family:serif doesn't override a font-family property #50

Closed hrj closed 8 years ago

hrj commented 8 years ago

CSS:

html { font-family: 'Ahem' }
p { font-family: serif }

HTML:

<html>
<p>Test</p>
</html>

The p element doesn't get font-family:serif.

radkovo commented 8 years ago

I mean this is related to my recent comment to #45 . According to my tests, the value obtained using the getProperty() call is correct (i.e. you obtain SERIF for the p element which is a keyword). The getValue() call returns the inherited value but it shouldn't be taken into account because getProperty() does not return list_values for the paragraph.

hrj commented 8 years ago

According to my tests, the value obtained using the getProperty() call is correct (i.e. you obtain SERIF for the p element which is a keyword).

Ok.

The getValue() call returns the inherited value but it shouldn't be taken into account because getProperty() does not return list_values for the paragraph.

Is there a reason why it returns the inherited value? In our current code, for historical reasons, we are trying the getValue() call first and then the getProperty() call.

I can perfectly understand if you don't want to change the behaviour for any reason (like performance). However, if it makes sense to change it to return nullthen it would save us time.

Alternatively, when the getProperty() call returns SERIF, and we call .toString() on it, could it return the keyword as a string value? Currently, it seems to return an empty string.

radkovo commented 8 years ago

Actually, the toString() call on the returned property value (e.g. SERIF) should return the correct string value (i.e. "serif") for the keywords and should return an empty string for the values that require the getValue() call. At least it seems to work in CSSBox, see this code. Doesn't it work like this for you?

Anyway, I will take a look on removing the inherited value as well if it is not a performance problem.

hrj commented 8 years ago

Actually, the toString() call on the returned property value (e.g. SERIF) should return the correct string value (i.e. "serif") for the keywords and should return an empty string for the values that require the getValue() call. At least it seems to work in CSSBox, see this code. Doesn't it work like this for you?

Ah, you are right; it works as you describe. I had understood some things wrong.

So, why does getProperty() return a non-null result for those cases when getValue() needs to be called? The call toString() and isEmpty() are additional overheads. A null check would be more efficient.

Anyway, I will take a look on removing the inherited value as well if it is not a performance problem.

Great. Meanwhile, I am running all the CSS tests with getProperty().toString().isEmpty() technique; just to check if it works.

radkovo commented 8 years ago

So, why does getProperty() return a non-null result for those cases when getValue() needs to be called?

It's because the returned property value may be important for further processing the value. E.g. for the width property, the getProperty() call may return Width.length or Width.percentage or Width.AUTO. For both the lengths and percentages, getValue() must be called but the obtained value has a different meaning (we obtain TermLength or TermPercentage object from getValue()).

The call toString() and isEmpty() are additional overheads. A null check would be more efficient.

I could efficiently implement a function like boolean valueProvided() that would return true when the getValue() would return a value. This would be more efficient than the toString().isEmpty() call. Would this be a solution?

hrj commented 8 years ago

I could efficiently implement a function like boolean valueProvided() that would return true when the getValue() would return a value. This would be more efficient than the toString().isEmpty() call. Would this be a solution?

Yes, that would be awesome!

An alternate / addition that would be nice to have: NodeData.getAsString(propertyName) which could return the string representation equivalent to:

let property = nodeData.getProperty(name);
let propertyString = property == null ? null : property.toString();
if (propertyString == null || propertyString.isEmpty()) {
  let value = nodeData.getValue(name);
  return value == null ? null : value.toString();
} else
  return propertyString;
}

I was testing with the above pseudo-code in gngr, and it worked fine, but having a direct getAsString() method would be much more efficient (even more efficient than using valueProvided()).

hrj commented 8 years ago

(I updated the pseudo-code for clarity and correctness)

radkovo commented 8 years ago

Finally, it has not been difficult to make the getValue() call always return null when no value is provided. That means, the way you originally proposed may be used now.

Additionally, I have added the getAsString(name, includeInherited) function you proposed that should be even more efficient because it directly uses some internal structures of the NodeData implementation.

hrj commented 8 years ago

@radkovo Thanks! I have tested the getAsString function with our code and it works. I added some comments about the implementation. Were you planning to optimise the implementation later?

hrj commented 8 years ago

I am closing this as all major concerns are addressed.

Thanks again, @radkovo