Skip to content

Move CSS longhands outside of mako #19015

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

Closed
48 of 49 tasks
emilio opened this issue Oct 25, 2017 · 25 comments
Closed
48 of 49 tasks

Move CSS longhands outside of mako #19015

emilio opened this issue Oct 25, 2017 · 25 comments
Labels
A-content/css Interacting with CSS from web content (parsing, serializing, introspection) B-meta This issue tracks the status of multiple, related pieces of work E-more-complex Variable effort required; may require a mentor. Recommended solution is clearly described in the iss

Comments

@emilio
Copy link
Member

emilio commented Oct 25, 2017

We use the mako template system to autogenerate code.

Ideally, we'd like to use a manifest and use Rust to generate it instead. For that, we need to move all the remaining CSS properties (most of them have been moved already) outside of Mako. That is the same as saying: We should make all those use ${helpers.predefined_type(..)}.

This is mostly moving and cleaning up code in the same way as #19002 and #19005, but there are probably bugs along the way that may need fixing (like the calc() bug I added a test for in #19005).

If you want to help out with this, please comment on this issue or reach out on IRC (#servo in irc.mozilla.org) and we'll find one which is interesting to you.

Or, you can also file an issue and mention this one and I'll get it assigned to you.

Known properties still using mako:

FAQ

How do I get started? How can I make any sense of the code?

This code lives in compontents/style/properties/longhands. Note that some of the code is Firefox-specific (properties marked with products="gecko", so you may need to do ./mach build-geckolib or ./mach cargo-geckolib check in order to build and see).

Please ask here, in #servo, or open an issue like #19020 and say that you want to work on it.

Ask questions! I (or any of the other Servo members and contributors) are happy to help :)

What's the general setup?

Each CSS property has two different representations: The "specified" one (the one people write in the stylesheets), and the "computed" one. Sometimes they are the same, sometimes they're different.

So for example if you write width: 1em in a CSS style sheet, the "specified" value is 1em, but the "computed" value could be, for example, 10px, if the parent font-size is 10px.

So the way this is organized is roughly as follows: We have two rust modules inside compontents/style/values. The computed and the specified (there are others, but those shouldn't worry you).

The first argument for predefined_type is the CSS property name.

The second argument is the name of the type that becomes computed::$name or specified::$name, depending on the appropriate context. For example, in #19018, both are the same (computed::ScrollSnapType just re-exports specified::ScrollSnapType).

The third argument is the initial computed value of the property. The rest of the arguments are key-value pairs with various meanings, that we should probably preserve intact. If there are any questions about a particular argument feel free to ask though!

Which ones are easier? Which ones are harder?

In general, the level of difficulty relates with the amount that lies inside the mako section of that property.

For example, transform is huge, and is fairly non-trivial, while probably others like overflow-y are easy to refactor like #19018.

But again, please ask if you're unsure and we'll try to answer! :)

@emilio emilio added B-meta This issue tracks the status of multiple, related pieces of work A-content/css Interacting with CSS from web content (parsing, serializing, introspection) labels Oct 25, 2017
@Manishearth
Copy link
Member

Yes!

I'm currently doing transform stuff though I may not do the extra step of predefined typing it.

@jdm jdm added the E-more-complex Variable effort required; may require a mentor. Recommended solution is clearly described in the iss label Oct 25, 2017
@SimonSapin
Copy link
Member

We use the mako template system to autogenerate code.

Ideally, we'd like to use a manifest, and use Rust to generate it instead.

Before we go through every property and move code around, maybe we could talk a little bit about what design we want?

For example, maybe instead of the pattern of having modules that are expected to define a few items of certain kinds with certain names, we could rely on the hot new language feature of associated types. That kind of things.

@SimonSapin
Copy link
Member

After clarifying on IRC, it turns out that the changes I had in mind are mostly orthogonal with what this issue is about. So, carry on :)

bors-servo pushed a commit that referenced this issue Oct 26, 2017
style: Move -x-text-zoom outside of mako

Sub-PR for #19015

r? emilio

---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #19020 .

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19030)
<!-- Reviewable:end -->
bors-servo pushed a commit that referenced this issue Oct 27, 2017
style: Move table -x-span outisde of mako

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes are apart of #19015 (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [X] These changes do not require tests because refactoring.

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19036)
<!-- Reviewable:end -->
bors-servo pushed a commit that referenced this issue Oct 28, 2017
style: Move text-overflow outside of mako

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes are apart of #19015 (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [X] These changes do not require tests because refactoring.

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19044)
<!-- Reviewable:end -->
bors-servo pushed a commit that referenced this issue Oct 28, 2017
style: Move text-overflow outside of mako

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes are apart of #19015 (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [X] These changes do not require tests because refactoring.

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19044)
<!-- Reviewable:end -->
bors-servo pushed a commit that referenced this issue Oct 28, 2017
style: Move text-overflow outside of mako

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes are apart of #19015 (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [X] These changes do not require tests because refactoring.

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19044)
<!-- Reviewable:end -->
bors-servo pushed a commit that referenced this issue Oct 29, 2017
style: Move text-overflow outside of mako

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes are apart of #19015 (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [X] These changes do not require tests because refactoring.

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19044)
<!-- Reviewable:end -->
bors-servo pushed a commit that referenced this issue Oct 30, 2017
…naltinova

style: Move font -moz-script-min-size outside of mako

This is a sub-PR for #19015
r? @emilio

---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #19034 (github issue number if applicable).
- [X] These changes do not require tests because _____

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19051)
<!-- Reviewable:end -->
bors-servo pushed a commit that referenced this issue Oct 30, 2017
style: Move text-overflow outside of mako

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes are apart of #19015 (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [X] These changes do not require tests because refactoring.

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19044)
<!-- Reviewable:end -->
bors-servo pushed a commit that referenced this issue Oct 30, 2017
…naltinova

style: Move font -moz-script-min-size outside of mako

This is a sub-PR for #19015
r? emilio

---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #19034 (github issue number if applicable).
- [X] These changes do not require tests because _____

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19051)
<!-- Reviewable:end -->
bors-servo pushed a commit that referenced this issue Nov 1, 2017
style: Move font-weight outside of mako

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes are apart of #19015 (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [X] These changes do not require tests because refactoring

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19086)
<!-- Reviewable:end -->
@Manishearth
Copy link
Member

We should also split up gecko.mako.rs into gecko.rs + gecko.mako.rs or something. All the manual impls go in gecko.rs. You can have multiple impl blocks for a type so that's fine.

@Manishearth
Copy link
Member

Ideally, when debugging code it would be nice to never/rarely have to look at an autogenerated file and map its contents back to the "real" source.

@chansuke
Copy link
Contributor

chansuke commented Nov 3, 2017

@emilio Hi.I want to try this issue:)

@emilio
Copy link
Member Author

emilio commented Nov 3, 2017

Hi @chansuke! This issue spans a few multiple sub-issues, are you interested in any of the list that don't have a PR or issue open already? If you don't want to choose one yourself, just let me know and I can choose one that is easy and assign it to you.

Thanks a lot!

bors-servo pushed a commit that referenced this issue Jan 14, 2018
style: Remove -servo-display-for-hypothetical-box from longhand

This is a sub-PR of #19015
r? emilio

For the `fn set_original_display` inside `properties.mako.rs`, I removed `is_item_or_root` first to see how the tests result is. If it's needed, I'll add it back.

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #19697
- [x] These changes do not require tests

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19709)
<!-- Reviewable:end -->
@jonleighton
Copy link
Contributor

I'll work on image-orientation: #19765

bors-servo pushed a commit that referenced this issue Jan 14, 2018
style: Remove -servo-display-for-hypothetical-box from longhand

This is a sub-PR of #19015
r? emilio

For the `fn set_original_display` inside `properties.mako.rs`, I removed `is_item_or_root` first to see how the tests result is. If it's needed, I'll add it back.

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #19697
- [x] These changes do not require tests

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19709)
<!-- Reviewable:end -->
bors-servo pushed a commit that referenced this issue Jan 15, 2018
…o, r=emilio

style: Move -moz-context-properties outside of mako.

<!-- Please describe your changes on the following line: -->
Sub-PR of #19015

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #19742  (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [x] These changes do not require tests

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19749)
<!-- Reviewable:end -->
bors-servo pushed a commit that referenced this issue Jan 20, 2018
style: Move cursor property out of mako

<!-- Please describe your changes on the following line: -->
Sub-PR of #19015

r? emilio

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach build-geckolib` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #19775 (github issue number if applicable).

<!-- Either: -->
- [x] These changes do not require tests

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19798)
<!-- Reviewable:end -->
bors-servo pushed a commit that referenced this issue Jan 20, 2018
style: Move cursor property out of mako

<!-- Please describe your changes on the following line: -->
Sub-PR of #19015

r? emilio

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach build-geckolib` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #19775 (github issue number if applicable).

<!-- Either: -->
- [x] These changes do not require tests

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19798)
<!-- Reviewable:end -->
bors-servo pushed a commit that referenced this issue Jan 20, 2018
style: Move cursor property out of mako

<!-- Please describe your changes on the following line: -->
Sub-PR of #19015

r? emilio

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach build-geckolib` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #19775 (github issue number if applicable).

<!-- Either: -->
- [x] These changes do not require tests

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19798)
<!-- Reviewable:end -->
bors-servo pushed a commit that referenced this issue Feb 1, 2018
style: moved css longhand counter-reset out of mako

<!-- Please describe your changes on the following line: -->
This is a sub-PR of #19015
Code does not yet compile with `build-geckolib`.
r? emilio

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #19387

<!-- Either: -->
- [x] These changes do not require tests

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19529)
<!-- Reviewable:end -->
@donovanglover
Copy link

Hello I'd like to work on text-emphasis-style, thanks.

@jdm
Copy link
Member

jdm commented Feb 3, 2018

@gloverdonovan Go ahead and file an issue for it; you're welcome to work on it!

bors-servo pushed a commit that referenced this issue Mar 10, 2018
…psuper

Extract text emphasis style

<!-- Please describe your changes on the following line: -->
Extracted the text-emphasis-style property out of the inherited_text.mako.rs.
This is a part of #19015

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] `./mach cargo-geckolib check` does not report any errors
- [X] These changes fix #19940 (github issue number if applicable).

<!-- Either: -->
- [X] These changes do not require tests because it's a refactoring

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20252)
<!-- Reviewable:end -->
@atouchet atouchet changed the title [meta] Move CSS longhands outside of mako. Move CSS longhands outside of mako. Jun 23, 2018
@servo servo deleted a comment Jul 7, 2018
@servo servo deleted a comment Jul 7, 2018
@kleinph
Copy link
Contributor

kleinph commented Jul 8, 2018

I would like to take float (and clear if the first one is successful). I already created issue #21130 (and a PR) for that.

bors-servo pushed a commit that referenced this issue Jul 10, 2018
style: Move `clear` CSS property outside mako

This is part of issue #19015.

- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #21155

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21156)
<!-- Reviewable:end -->
@atouchet atouchet changed the title Move CSS longhands outside of mako. Move CSS longhands outside of mako Jul 2, 2020
@atouchet
Copy link
Contributor

atouchet commented Jul 2, 2020

@emilio @Manishearth is this done now?

@jdm
Copy link
Member

jdm commented Jul 6, 2020

I'm going to go ahead and claim it's done. We can file specific issues for anything remaining.

@jdm jdm closed this as completed Jul 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-content/css Interacting with CSS from web content (parsing, serializing, introspection) B-meta This issue tracks the status of multiple, related pieces of work E-more-complex Variable effort required; may require a mentor. Recommended solution is clearly described in the iss
Projects
None yet
Development

No branches or pull requests

15 participants