Skip to content
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

Support presentational hints #352

Merged
merged 10 commits into from Sep 1, 2016
Merged

Support presentational hints #352

merged 10 commits into from Sep 1, 2016

Conversation

liZe
Copy link
Member

@liZe liZe commented Aug 29, 2016

See #253.

This feature has been implemented in a clean way.

Random thoughts:

  • I've added this feature because it's useful for many W3C tests.
  • The diff is big but really simple.
  • Many hints are given in CSS in the spec, they could (should?) be added in another stylesheet in WP with priority set to 0 (instead of code).
  • Merging find_style_attributes and find_presetational_hints is possible, it would avoid going through the whole tree twice.
  • The code is not tested, but the test suites available on test.weasyprint.org pass without a crash.
  • Calling check_style_attribute with constant strings is probably useless.
  • This features makes the tests last ~5% more for me, and it's probably much worst on real cases. Should we allow the users to enable/disable it with an option?
  • Some rules have not been implemented, mainly because they would be much easier to handle with CSS than with code (one more reason to use an extra stylesheet).
  • Some parsing errors are logged, some aren't (who cares?).

Comments are welcome, as usual. I won't merge it right now, I'd like (at least):

  • a new option to activate this feature (and keep it deactivated by default),
  • a stylesheet for rules described in CSS in the spec,
  • tests.

@liZe liZe added the feature New feature that should be supported label Aug 29, 2016
@liZe liZe added this to the v0.32 milestone Aug 29, 2016
@liZe
Copy link
Member Author

liZe commented Sep 1, 2016

This PR is ready to be merged. I'm happy with it because:

  • it's optional and doesn't impact performances by default,
  • its code is clean 💐, with CSS rules when possible and Python code when it's not,
  • it's tested and quite reliable,
  • it's implementing almost all the rules defined in the spec,
  • it's really useful for the W3C tests.

The main problems remaining are not that important:

  • CSS rules' attributes values are case-sensitive (we have to implement CSS Selectors Level 4 to fix this),
  • some values should raise errors instead of being accepted or silently rejected.

@SimonSapin I think that you don't care about presentational hints, but you can look at the code if you want 😄.

@liZe liZe merged commit 3fc6e33 into master Sep 1, 2016
@liZe liZe deleted the presentationalhints branch September 1, 2016 21:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature that should be supported
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant