Skip to content

Add "line-height" #422

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 24, 2020
Merged

Conversation

federicomenaquintero
Copy link
Contributor

This is in CSS3, which is kind of required for SVG2, which I'm slowly getting at in librsvg 😃

@jdm
Copy link
Member

jdm commented Jun 24, 2020

@bors-servo r+

@bors-servo
Copy link
Contributor

📌 Commit d086eef has been approved by jdm

@bors-servo
Copy link
Contributor

⌛ Testing commit d086eef with merge 0e6b972...

@bors-servo
Copy link
Contributor

☀️ Test successful - checks-travis
Approved by: jdm
Pushing 0e6b972 to master...

@bors-servo bors-servo merged commit 0e6b972 into servo:master Jun 24, 2020
@federicomenaquintero
Copy link
Contributor Author

That was fast, thank you!

@SimonSapin
Copy link
Member

I don’t find line-height in https://svgwg.org/svg2-draft/eltindex.html or https://svgwg.org/svg2-draft/attindex.html, so it doesn’t like it’s an element name or attribute name which is what html5ever uses the LocalName type for. Servo uses https://docs.rs/string_cache/0.8.0/string_cache/#with-compile-time-atoms to define a separate Atom type for CSS property names, with the names of all supported properties as static atoms. I recommend librsvg does the same, so a PR like this one isn’t needed every time you add support for a new property.

@federicomenaquintero
Copy link
Contributor Author

Oh, line-height is indeed a property name, not an attribute name, from https://www.w3.org/TR/CSS22/visudet.html#propdef-line-height

It wasn't clear to me if the names in markup5ever are in fact attribute names that happen to correspond to properties.

Am I doing something wrong here:

  • Librsvg stores all attribute names and CSS property names as QualName, because the code more or less evolved to that, and because I thought that attributes which have the same name as properties may as well use the same representation for their name. Should property names live elsewhere / am I wrongly using QualName because it seemed like a convenient way to have a namespace/name?
  • (Note that librsvg does not implement CSS namespaces yet...)
  • If I define my own string cache for CSS property names, won't there be awkward conversions between QualName, which expects type LocalName = Atom<LocalNameStaticSet>, and my own string cache? (If the answer above was "QualName is not meant for CSS property names", then I guess this question is moot :)

@SimonSapin
Copy link
Member

CSS properties do not have namespaces. CSS @namespace rules affect selectors, not properties, and is relevant to the namespace of element types and attributes names being matched.

SVG has a number of presentation attributes that map to CSS properties of corresponding names, but not all SVG attributes are "presentation" and not all CSS properties relevant to SVG have a corresponding attribute. (Parsing the value of those attributes is also slightly different from parsing the value of the corresponding property in a stylesheet or style attribute context.)

gnomesysadmins pushed a commit to GNOME/librsvg that referenced this pull request Jul 1, 2020
For now this requires a special case in the make_properties! macro.
The markup5ever crate does not support "line-height" out of the box
yet; see servo/html5ever#422.

So, we add a special case for property names that are not supported
yet in markup5ever.  Ugly but it works.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants