-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Make additional properties be reset by the 'font' shorthand. #15305
Conversation
Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @glennw (or someone else) soon. |
(And I did now get the Servo build to work, after installing a bunch of packages, and realizing I needed to "unset MOZCONFIG" to get my Gecko mozconfig out of the way for building JS.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bors-servo delegate+
r=me with nit
(you can bors-servo r=Manishearth
this to approve for landing)
I suspect this will break in ./mach test-unit -p style
due to the funky conditional compilation we do in unit testing mode (product is still servo but we include all properties). If it does, the fix would be to do something like
<%
extra_reset_props = ["font-size-adjust", "font-kerning", ....]
enabled_reset_props = [prop for prop in extra_reset_props if prop in data.longhands_by_name]
%>
above the shorthand decl, and using ${enabled_reset_props}.join(" ")
and
%for prop in enabled_reset_props:
${to_rust_ident(prop)}: None,
%endfor
@@ -73,6 +78,17 @@ | |||
font_size: size, | |||
line_height: line_height, | |||
font_family: Some(font_family::SpecifiedValue(family)) | |||
% if product == "gecko": | |||
, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: just keep this comma above. trailing commas are ok and often encouraged in rust
@bors-servo delegate+ hm, homu doesn't pick up review comments |
✌️ @dbaron can now approve this pull request |
A number of properties are supposed to be reset to initial values by the 'font' shorthand. This does so for all such properties that Servo currently supports (conditional on when they're supported). Fixes servo#15033.
|
bb83612
to
7a65a47
Compare
@bors-servo r=Manishearth,emilio Thanks David :) |
📌 Commit 7a65a47 has been approved by |
⌛ Testing commit 7a65a47 with merge ef81384... |
Make additional properties be reset by the 'font' shorthand. A number of properties are supposed to be reset to initial values by the 'font' shorthand. This does so for all such properties that Servo currently supports (conditional on when they're supported). Fixes #15033. It's not clear to me that the use of <code>None</code> is correct for resetting to initial values, but it seems to match what at least some other shorthands do (I think I was looking at the code for border), and it appears to fix the tests as expected. I haven't probed into it more than that. /cc @Manishearth --- <!-- 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 (I haven't quite gotten the servo build to work, but stylo build works) - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #15033 (github issue number if applicable). <!-- Either: --> - [X] There are tests for these changes - [ ] These changes do not require tests because _____ This has test coverage when running stylo. In particular, it fixes a bunch of sets of mochitest failures in layout/style/test/test_value_storage.html, with the set for each value of the 'font' shorthand listed in property_database.js looking like: ``` TEST-PASS | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' TEST-PASS | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-style') TEST-PASS | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-variant') TEST-PASS | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-weight') TEST-PASS | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-size') TEST-PASS | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'line-height') TEST-PASS | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-family') TEST-PASS | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-stretch') -TEST-UNEXPECTED-FAIL | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-size-adjust') - didn't expect "", but got it +TEST-PASS | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-size-adjust') TEST-UNEXPECTED-FAIL | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-feature-settings') - didn't expect "", but got it TEST-UNEXPECTED-FAIL | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-language-override') - didn't expect "", but got it -TEST-UNEXPECTED-FAIL | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-kerning') - didn't expect "", but got it +TEST-PASS | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-kerning') TEST-UNEXPECTED-FAIL | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-synthesis') - didn't expect "", but got it TEST-UNEXPECTED-FAIL | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-variant-alternates') - didn't expect "", but got it -TEST-UNEXPECTED-FAIL | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-variant-caps') - didn't expect "", but got it +TEST-PASS | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-variant-caps') TEST-UNEXPECTED-FAIL | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-variant-east-asian') - didn't expect "", but got it TEST-UNEXPECTED-FAIL | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-variant-ligatures') - didn't expect "", but got it TEST-UNEXPECTED-FAIL | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-variant-numeric') - didn't expect "", but got it -TEST-UNEXPECTED-FAIL | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-variant-position') - didn't expect "", but got it +TEST-PASS | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-variant-position') ``` I'm not sure if separate Servo test coverage is needed. <!-- 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/15305) <!-- Reviewable:end -->
💔 Test failed - mac-rel-wpt2 |
@bors-servo try retry looks like a new intermittent? cc @creativcoder @jdm
|
Make additional properties be reset by the 'font' shorthand. A number of properties are supposed to be reset to initial values by the 'font' shorthand. This does so for all such properties that Servo currently supports (conditional on when they're supported). Fixes #15033. It's not clear to me that the use of <code>None</code> is correct for resetting to initial values, but it seems to match what at least some other shorthands do (I think I was looking at the code for border), and it appears to fix the tests as expected. I haven't probed into it more than that. /cc @Manishearth --- <!-- 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 (I haven't quite gotten the servo build to work, but stylo build works) - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #15033 (github issue number if applicable). <!-- Either: --> - [X] There are tests for these changes - [ ] These changes do not require tests because _____ This has test coverage when running stylo. In particular, it fixes a bunch of sets of mochitest failures in layout/style/test/test_value_storage.html, with the set for each value of the 'font' shorthand listed in property_database.js looking like: ``` TEST-PASS | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' TEST-PASS | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-style') TEST-PASS | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-variant') TEST-PASS | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-weight') TEST-PASS | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-size') TEST-PASS | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'line-height') TEST-PASS | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-family') TEST-PASS | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-stretch') -TEST-UNEXPECTED-FAIL | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-size-adjust') - didn't expect "", but got it +TEST-PASS | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-size-adjust') TEST-UNEXPECTED-FAIL | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-feature-settings') - didn't expect "", but got it TEST-UNEXPECTED-FAIL | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-language-override') - didn't expect "", but got it -TEST-UNEXPECTED-FAIL | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-kerning') - didn't expect "", but got it +TEST-PASS | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-kerning') TEST-UNEXPECTED-FAIL | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-synthesis') - didn't expect "", but got it TEST-UNEXPECTED-FAIL | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-variant-alternates') - didn't expect "", but got it -TEST-UNEXPECTED-FAIL | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-variant-caps') - didn't expect "", but got it +TEST-PASS | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-variant-caps') TEST-UNEXPECTED-FAIL | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-variant-east-asian') - didn't expect "", but got it TEST-UNEXPECTED-FAIL | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-variant-ligatures') - didn't expect "", but got it TEST-UNEXPECTED-FAIL | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-variant-numeric') - didn't expect "", but got it -TEST-UNEXPECTED-FAIL | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-variant-position') - didn't expect "", but got it +TEST-PASS | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-variant-position') ``` I'm not sure if separate Servo test coverage is needed. <!-- 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/15305) <!-- Reviewable:end -->
☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-gnu-dev, windows-msvc-dev |
Make additional properties be reset by the 'font' shorthand. A number of properties are supposed to be reset to initial values by the 'font' shorthand. This does so for all such properties that Servo currently supports (conditional on when they're supported). Fixes #15033. It's not clear to me that the use of <code>None</code> is correct for resetting to initial values, but it seems to match what at least some other shorthands do (I think I was looking at the code for border), and it appears to fix the tests as expected. I haven't probed into it more than that. /cc @Manishearth --- <!-- 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 (I haven't quite gotten the servo build to work, but stylo build works) - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #15033 (github issue number if applicable). <!-- Either: --> - [X] There are tests for these changes - [ ] These changes do not require tests because _____ This has test coverage when running stylo. In particular, it fixes a bunch of sets of mochitest failures in layout/style/test/test_value_storage.html, with the set for each value of the 'font' shorthand listed in property_database.js looking like: ``` TEST-PASS | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' TEST-PASS | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-style') TEST-PASS | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-variant') TEST-PASS | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-weight') TEST-PASS | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-size') TEST-PASS | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'line-height') TEST-PASS | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-family') TEST-PASS | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-stretch') -TEST-UNEXPECTED-FAIL | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-size-adjust') - didn't expect "", but got it +TEST-PASS | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-size-adjust') TEST-UNEXPECTED-FAIL | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-feature-settings') - didn't expect "", but got it TEST-UNEXPECTED-FAIL | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-language-override') - didn't expect "", but got it -TEST-UNEXPECTED-FAIL | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-kerning') - didn't expect "", but got it +TEST-PASS | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-kerning') TEST-UNEXPECTED-FAIL | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-synthesis') - didn't expect "", but got it TEST-UNEXPECTED-FAIL | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-variant-alternates') - didn't expect "", but got it -TEST-UNEXPECTED-FAIL | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-variant-caps') - didn't expect "", but got it +TEST-PASS | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-variant-caps') TEST-UNEXPECTED-FAIL | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-variant-east-asian') - didn't expect "", but got it TEST-UNEXPECTED-FAIL | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-variant-ligatures') - didn't expect "", but got it TEST-UNEXPECTED-FAIL | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-variant-numeric') - didn't expect "", but got it -TEST-UNEXPECTED-FAIL | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-variant-position') - didn't expect "", but got it +TEST-PASS | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-variant-position') ``` I'm not sure if separate Servo test coverage is needed. <!-- 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/15305) <!-- Reviewable:end -->
💔 Test failed - linux-rel-wpt |
OK, that's a webrender intermittent @bors-servo try- r+ retry |
💡 This pull request was already approved, no need to approve it again.
|
📌 Commit 7a65a47 has been approved by |
Make additional properties be reset by the 'font' shorthand. A number of properties are supposed to be reset to initial values by the 'font' shorthand. This does so for all such properties that Servo currently supports (conditional on when they're supported). Fixes #15033. It's not clear to me that the use of <code>None</code> is correct for resetting to initial values, but it seems to match what at least some other shorthands do (I think I was looking at the code for border), and it appears to fix the tests as expected. I haven't probed into it more than that. /cc @Manishearth --- <!-- 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 (I haven't quite gotten the servo build to work, but stylo build works) - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #15033 (github issue number if applicable). <!-- Either: --> - [X] There are tests for these changes - [ ] These changes do not require tests because _____ This has test coverage when running stylo. In particular, it fixes a bunch of sets of mochitest failures in layout/style/test/test_value_storage.html, with the set for each value of the 'font' shorthand listed in property_database.js looking like: ``` TEST-PASS | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' TEST-PASS | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-style') TEST-PASS | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-variant') TEST-PASS | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-weight') TEST-PASS | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-size') TEST-PASS | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'line-height') TEST-PASS | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-family') TEST-PASS | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-stretch') -TEST-UNEXPECTED-FAIL | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-size-adjust') - didn't expect "", but got it +TEST-PASS | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-size-adjust') TEST-UNEXPECTED-FAIL | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-feature-settings') - didn't expect "", but got it TEST-UNEXPECTED-FAIL | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-language-override') - didn't expect "", but got it -TEST-UNEXPECTED-FAIL | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-kerning') - didn't expect "", but got it +TEST-PASS | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-kerning') TEST-UNEXPECTED-FAIL | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-synthesis') - didn't expect "", but got it TEST-UNEXPECTED-FAIL | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-variant-alternates') - didn't expect "", but got it -TEST-UNEXPECTED-FAIL | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-variant-caps') - didn't expect "", but got it +TEST-PASS | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-variant-caps') TEST-UNEXPECTED-FAIL | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-variant-east-asian') - didn't expect "", but got it TEST-UNEXPECTED-FAIL | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-variant-ligatures') - didn't expect "", but got it TEST-UNEXPECTED-FAIL | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-variant-numeric') - didn't expect "", but got it -TEST-UNEXPECTED-FAIL | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-variant-position') - didn't expect "", but got it +TEST-PASS | layout/style/test/test_value_storage.html | setting 'medium serif' on 'font' (for 'font-variant-position') ``` I'm not sure if separate Servo test coverage is needed. <!-- 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/15305) <!-- Reviewable:end -->
☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-gnu-dev, windows-msvc-dev |
A number of properties are supposed to be reset to initial values by the
'font' shorthand. This does so for all such properties that Servo
currently supports (conditional on when they're supported).
Fixes #15033.
It's not clear to me that the use of
None
is correct for resetting to initial values, but it seems to match what at least some other shorthands do (I think I was looking at the code for border), and it appears to fix the tests as expected. I haven't probed into it more than that./cc @Manishearth
./mach build -d
does not report any errors (I haven't quite gotten the servo build to work, but stylo build works)./mach test-tidy
does not report any errorsfont
shorthand should resetfont-size-adjust
,font-kerning
,font-language-override
etc. even if those longhands cannot be set via the longhand #15033 (github issue number if applicable).This has test coverage when running stylo. In particular, it fixes a bunch of sets of mochitest failures in layout/style/test/test_value_storage.html, with the set for each value of the 'font' shorthand listed in property_database.js looking like:
I'm not sure if separate Servo test coverage is needed.
This change is