Skip to content

/ servo

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

Jump to bottom

`font` shorthand should be serialized to empty when some of its subproperties have non-default value #15036

Closed
upsuper opened this issue Jan 16, 2017 · 9 comments
Closed

`font` shorthand should be serialized to empty when some of its subproperties have non-default value #15036

upsuper opened this issue Jan 16, 2017 · 9 comments
Labels
A-content/css C-assigned
Milestone
Stylo: Style syst...

Comments

@upsuper
Copy link
Member

@upsuper upsuper commented Jan 16, 2017

There are many subproperties of font shorthand which cannot present in font shorthand (see #15033). When any of those properties have non-default value, font shorthand should be serialized to empty string.
@upsuper upsuper mentioned this issue Feb 6, 2017
Closed
@jdm
Copy link
Member

@jdm jdm commented Feb 15, 2017

@zploskey What about this one? It requires modifying the implementation of to_css_declared in components/style/properties/shorthand/font.mako.rs to check if the longhand properties from #15033 are non-default values and change the serialization behaviour in that case. This will require a unit test in tests/unit/style/properties/serialization.rs which can be run with ./mach test-unit -p style.
@zploskey
Copy link
Contributor

@zploskey zploskey commented Feb 15, 2017

@jdm That sounds doable. I'll get started. Thanks!
@jdm jdm added the C-assigned label Feb 15, 2017
@upsuper upsuper mentioned this issue Feb 15, 2017
Merged
4 of 5 tasks complete
@zploskey
Copy link
Contributor

@zploskey zploskey commented Feb 16, 2017

I've written a test and enabled the commented out test for the font module that was already there, with a couple of changes. I think I've made the changes necessary to resolve this issue, but when I run mach test-unit -p style none of the code that is enabled for specific products in the mako template seems to be built or executed. I actually just put a panic in one of the product-specific parts and it never ran. I thought all products were supposed to be enabled in test? Are they not for some reason? Anyone else have this issue?

The tests won't be able to pass until I figure out why this is happening. Once I get that part sorted out I'll make any necessary refinements and send a pull request.
@jdm
Copy link
Member

@jdm jdm commented Feb 16, 2017

@Manishearth Any idea what's going on?
@Manishearth
Copy link
Member

@Manishearth Manishearth commented Feb 16, 2017

The % if product == "gecko": will not work in testing mode. I don't think there's a way around this.

Properties defined for specific products are enabled in tests. The conditional ifs are not.
@zploskey
Copy link
Contributor

@zploskey zploskey commented Feb 16, 2017

I guess I need to come up with an appropriate way to check if a struct has a field or not. Open to suggestions. I had been using % if product == "gecko", etc.
@Manishearth
Copy link
Member

@Manishearth Manishearth commented Feb 16, 2017

No, you should use if product == gecko. It just means that your tests won't be able to cover that code perfectly. We have tests in gecko for this (but they're harder to run right now)
@zploskey
Copy link
Contributor

@zploskey zploskey commented Feb 16, 2017

In that case, I'm not sure how to test this issue at all, but I'll go ahead and open a work-in-progress pull request and let you have a look.
@Manishearth
Copy link
Member

@Manishearth Manishearth commented Feb 16, 2017

Sure. I might try to make the shorthand code more testable later.
zploskey added a commit to zploskey/servo that referenced this issue Feb 16, 2017
@zploskey
serialize font: to empty on non-default subprops
3d5f50d 
Fixes font shorthand serialization so that it serializes to "" when
non-default subproperties are defined. These subproperties are those
defined in servo#15033.

Adds tests: -
should_serialize_to_empty_if_there_are_nondefault_subproperties -
font_should_serialize_all_available_properties

The first test passes by default but has not been tested in a way that
enables product = "gecko" or "none". Might be worth trying to configure
that to work in the unit tests.

The second of test was previously commented and underwent some minor
cleanup.

Bugfix: servo#15036
zploskey added a commit to zploskey/servo that referenced this issue Feb 16, 2017
@zploskey
serialize font: to empty on non-default subprops
cd5353f 
Fixes font shorthand serialization so that it serializes to "" when
non-default subproperties are defined. These subproperties are those
defined in servo#15033.

Adds tests: -
should_serialize_to_empty_if_there_are_nondefault_subproperties -
font_should_serialize_all_available_properties

The first test passes by default but has not been tested in a way that
enables product = "gecko" or "none". Might be worth trying to configure
that to work in the unit tests.

The second of test was previously commented and underwent some minor
cleanup.

Bugfix: servo#15036
@zploskey zploskey mentioned this issue Feb 16, 2017
Merged
4 of 4 tasks complete
zploskey added a commit to zploskey/servo that referenced this issue Feb 22, 2017
@zploskey
serialize font to "" on non-default subproperties
2112c97 
Fixes font shorthand serialization so that it serializes to "" when
non-default subproperties are defined. Subproperties which cause the
font shorthand to serialize to an empty string are:
- font-size-adjust
- font-kerning
- font-variant-caps
- font-variant-position
- font-language-override

Fixes servo#15036.
zploskey added a commit to zploskey/servo that referenced this issue Feb 22, 2017
@zploskey
serialize font to "" on non-default subproperties
4433841 
Fixes font shorthand serialization so that it serializes to "" when
non-default subproperties are defined. Subproperties which cause the
font shorthand to serialize to an empty string are:
- font-size-adjust
- font-kerning
- font-variant-caps
- font-variant-position
- font-language-override

Fixes servo#15036.
zploskey added a commit to zploskey/servo that referenced this issue Mar 2, 2017
@zploskey
serialize font to Ok(()) on nondefault subprops
3a5ce44 
Fixes font shorthand serialization so that it serializes to Ok(()) when
non-default subproperties are defined. Subproperties which cause the
font shorthand to serialize to an empty string are:
- font-size-adjust
- font-kerning
- font-variant-caps
- font-variant-position
- font-language-override

Fixes servo#15036.
zploskey added a commit to zploskey/servo that referenced this issue Mar 13, 2017
@zploskey
serialize font to Ok(()) on nondefault subprops
a734d0a 
Fixes font shorthand serialization so that it serializes to Ok(()) when
non-default subproperties are defined. Subproperties which cause the
font shorthand to serialize to an empty string are:
- font-size-adjust
- font-kerning
- font-variant-caps
- font-variant-position
- font-language-override

Fixes servo#15036.
bors-servo added a commit that referenced this issue Mar 13, 2017
@bors-servo
Auto merge of #15604 - zploskey:reset_font_shorthand_serialization, r…
Loading status checks…
2b4a364 
…=SimonSapin

serialize font: to empty on non-default subprops

Fixes font shorthand serialization so that it serializes to "" when
non-default subproperties are defined. These subproperties are those
defined in #15033.

Adds tests:
- should_serialize_to_empty_if_there_are_nondefault_subproperties
- font_should_serialize_all_available_properties

The first test passes, but has not been tested in a way that
enables product = "gecko" or "none". Might be worth trying to configure
that to work in the unit tests.

The second test was previously commented out and underwent some minor
cleanup to make it run.

---
<!-- 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 #15036

<!-- Either: -->
- [X] There are tests for these changes

<!-- 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/15604)
<!-- Reviewable:end -->
zploskey added a commit to zploskey/servo that referenced this issue Mar 14, 2017
@zploskey
serialize font to "" on nondefault subproperties
639651e 
Fixes font shorthand serialization so that it serializes to "" when
non-default subproperties are defined. Subproperties which cause the
font shorthand to serialize to "" are:
- font-size-adjust
- font-kerning
- font-variant-caps
- font-variant-position
- font-language-override

Fixes servo#15036.

This change also enables the above subproperties during testing.
bors-servo added a commit that referenced this issue Mar 16, 2017
@bors-servo
Auto merge of #15604 - zploskey:reset_font_shorthand_serialization, r…
Loading status checks…
2fa3efa 
…=SimonSapin

serialize font: to empty on non-default subprops

Fixes font shorthand serialization so that it serializes to "" when non-default subproperties are defined. These subproperties are those defined in #15033.

Adds tests:
- font_should_serialize_to_empty_if_there_are_nondefault_subproperties
- font_should_serialize_all_available_properties

The second test was previously commented out and underwent some cleanup to make it run.

---
<!-- 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 #15036

<!-- Either: -->
- [X] There are tests for these changes

<!-- 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/15604)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Mar 19, 2017
@bors-servo
Auto merge of #15604 - zploskey:reset_font_shorthand_serialization, r…
Loading status checks…
0fe2a0e 
…=SimonSapin

serialize font: to empty on non-default subprops

Fixes font shorthand serialization so that it serializes to "" when non-default subproperties are defined. These subproperties are those defined in #15033.

Adds tests:
- font_should_serialize_to_empty_if_there_are_nondefault_subproperties
- font_should_serialize_all_available_properties

The second test was previously commented out and underwent some cleanup to make it run.

---
<!-- 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 #15036

<!-- Either: -->
- [X] There are tests for these changes

<!-- 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/15604)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Mar 19, 2017
@bors-servo
Auto merge of #15604 - zploskey:reset_font_shorthand_serialization, r…
Loading status checks…
fbd561b 
…=SimonSapin

serialize font: to empty on non-default subprops

Fixes font shorthand serialization so that it serializes to "" when non-default subproperties are defined. These subproperties are those defined in #15033.

Adds tests:
- font_should_serialize_to_empty_if_there_are_nondefault_subproperties
- font_should_serialize_all_available_properties

The second test was previously commented out and underwent some cleanup to make it run.

---
<!-- 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 #15036

<!-- Either: -->
- [X] There are tests for these changes

<!-- 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/15604)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-content/css C-assigned
Projects
None yet
Milestone
Stylo: Style syst...
Linked pull requests

Successfully merging a pull request may close this issue.

serialize font: to empty on non-default subprops
4 participants
@jdm @upsuper @zploskey @Manishearth
You can’t perform that action at this time.