search

ocsigen / tyxml

Build valid HTML and SVG documents
https://ocsigen.org/tyxml/
Other
163 stars 57 forks source link

Adding support for `fill-opacity` #325

Open Mbodin opened 7 months ago

Mbodin commented 7 months ago

I was in need of the fill-opacity attribute, so I did these changes. I haven't changed the parser yet, but I'm not sure where to start.

The attribute fill-opacity takes the same kind of argument than offset (a number or a percentage), but they mean different things: I think that it would not make sense to reuse the type offset (being either a number or a percentage), so I introduced a number_or_percentage type. Maybe we should instead declare several types for offset and fill_opacity?

Drup commented 7 months ago

we do tend to try to use semantics names for the various possible inputs, but that's to be evaluated ton a case by case basis. Remind me what are the semantics in the offset and the opacity cases ?

Mbodin commented 7 months ago

Right. Then it's better to define different types: I'll revert the renaming to offset and create a new type ☺

I think that offset relates to patterns: it moves the start of the pattern (for instance on dashed lines). If provided a percentage, then it's along the line for that percentage (and it can be negative or greater than 100%), if a number, it represents a distance (I don't know why one can't provide a unit there).

For fill-opacity, the percentage must be between 0% and 100% (it's just the opacity), and the number must be between 0 and 1. So it's not only used to represent different things (distance vs alpha), but also has different bounds.

I'm going to change this ☺

Drup commented 7 months ago

Given your description ... maybe we could just mandate a float between 0 and 1 for the opacity, actually. The two representations are redundant.

Mbodin commented 7 months ago

The attribute fill-opacity itself can be seen as redundant with style='fill-opacity:__;', but it is useful as least for parsing. Is it fine to parse both percentages and numbers, but only using number internally?

Drup commented 7 months ago

There are three things: 1) The API, 2) The representation 3) The parsing

Parsing should be complete (in this case, accepting both form), API should be convenient (In this case, avoid 15 layers of data format), and the internal representation should yield valid HTML5 (in this case, whatever works).

Mbodin commented 7 months ago

Great! So let's write it such that we are able to write both percentages and numbers, but only storing numbers internally, and only printing out numbers ☺

I changed things in reflect.ml to be able to use my parser. I'm really not used to this kind of things (it feels like writing Ltac definitions in Coq). I hope that I did not do anything too wrong.

By the way, I noticed that there is a fill_rule parser defined in syntax/attribute_value.ml which is unused in syntax/reflect/reflect.ml. Is it normal?

Drup commented 7 months ago

I changed things in reflect.ml to be able to use my parser. I'm really not used to this kind of things (it feels like writing Ltac definitions in Coq). I hope that I did not do anything too wrong.

Yeah, it's ... not very structured. It works surprisingly well in practice :D

By the way, I noticed that there is a fill_rule parser defined in syntax/attribute_value.ml which is unused in syntax/reflect/reflect.ml. Is it normal?

.... not sure. Probably not!

Mbodin commented 7 months ago

Sorry for the amount of commits: it took me some time to understand how everything works.

I'm starting to appreciate the point that I reached regarding the opacity attributes. What do you think?