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-family parsing-serialization is not idempotent with quotes #15059

Closed
upsuper opened this issue Jan 17, 2017 · 7 comments
Closed

font-family parsing-serialization is not idempotent with quotes #15059

upsuper opened this issue Jan 17, 2017 · 7 comments
Labels
A-content/css

Comments

@upsuper
Copy link
Member

@upsuper upsuper commented Jan 17, 2017

See the following code:

<!DOCTYPE html>
<p style='font-family: \"times new roman'></p>
<script>
alert(document.querySelector('p').style.fontFamily);
</script>

It shows "times new roman which is a different value from \"times new roman.
@upsuper
Copy link
Member Author

@upsuper upsuper commented Jan 17, 2017

In addition, values like \035 simple (which is valid) would be serialized (or parsed) to 5simple (which is invalid since identifier cannot start with number), which is wrong.
@upsuper
Copy link
Member Author

@upsuper upsuper commented Jan 17, 2017

This seems to be an issue probably need to fix in the css parser in conjuction with font-family parsing code. I suspect this may need some deeper change in the parser, though I'm not completely sure.

cc @SimonSapin
@upsuper
Copy link
Member Author

@upsuper upsuper commented Jan 17, 2017

Some additional cases:

  • simple\02cinitial should not be serialized to simple,initial
  • simple, \02cinitial should not be serialized to simple, ,initial
  • \@simple should not be serialized to @simple

We probably want to migrate Gecko's test_font_family_parsing.html to wpt.
@SimonSapin
Copy link
Member

@SimonSapin SimonSapin commented Jan 19, 2017

I think this is because the serializer for FontFamily fails to use CSS escaping. I’m preparing a fix.
@SimonSapin
Copy link
Member

@SimonSapin SimonSapin commented Jan 19, 2017

I think this is correct, but it still needs tests.

diff --git a/components/style/properties/longhand/font.mako.rs b/components/style/properties/longhand/font.mako.rs
index fed23f1..013b6be 100644
--- a/components/style/properties/longhand/font.mako.rs
+++ b/components/style/properties/longhand/font.mako.rs
@@ -19,7 +19,8 @@
     impl NoViewportPercentage for SpecifiedValue {}
 
     pub mod computed_value {
-        use std::fmt;
+        use cssparser::CssStringWriter;
+        use std::fmt::{self, Write};
         use Atom;
         use style_traits::ToCss;
         pub use self::FontFamily as SingleComputedValue;
@@ -72,7 +73,16 @@
 
         impl ToCss for FontFamily {
             fn to_css<W>(&self, dest: &mut W) -> fmt::Result where W: fmt::Write {
-                self.atom().with_str(|s| dest.write_str(s))
+                match *self {
+                    FontFamily::FamilyName(ref name) => {
+                        dest.write_char('"')?;
+                        write!(CssStringWriter::new(dest), "{}", name)?;
+                        dest.write_char('"')
+                    }
+
+                    // All generic values accepted by the parser are known to not require escaping.
+                    FontFamily::Generic(ref name) => write!(dest, "{}", name),
+                }
             }
         }
@upsuper
Copy link
Member Author

@upsuper upsuper commented Jan 20, 2017

To test this, you can run ./mach mochitest --disable-e10s layout/style/test/test_font_family_parsing.html with a stylo build, and see if there is still any failure.
@upsuper
Copy link
Member Author

@upsuper upsuper commented Jan 20, 2017

Note that in some cases, the failure may not really make much sense, e.g. \"times new roman and '"times new roman' seems to be identical, so it doesn't matter if we serialize one to the other.
SimonSapin added a commit that referenced this issue Jan 24, 2017
@SimonSapin
Properly escape font-family names in CSS serialization.
db59dd4 
Fix #15059.
SimonSapin added a commit that referenced this issue Jan 24, 2017
@SimonSapin
Properly escape font-family names in CSS serialization.
bf16836 
Fix #15059.
@SimonSapin SimonSapin mentioned this issue Jan 24, 2017
Merged
4 of 5 tasks complete
bors-servo added a commit that referenced this issue Jan 24, 2017
@bors-servo
Auto merge of #15183 - servo:font-family, r=<try>
Loading status checks…
b9d29cf 
Fix parsing and serialization of font-family

<!-- 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 fix #15059 (github issue number if applicable).

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

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo added a commit that referenced this issue Jan 25, 2017
@bors-servo
Auto merge of #15183 - servo:font-family, r=<try>
Loading status checks…
ad48bb1 
Fix parsing and serialization of font-family

<!-- 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 fix #15059 (github issue number if applicable).

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

<!-- 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/15183)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Jan 26, 2017
@bors-servo
Auto merge of #15183 - servo:font-family, r=nox
Loading status checks…
58f4804 
Fix parsing and serialization of font-family

<!-- 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 fix #15059 (github issue number if applicable).

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

<!-- 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/15183)
<!-- 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
Projects
None yet
Milestone
No milestone
Linked pull requests

Successfully merging a pull request may close this issue.

Fix parsing and serialization of font-family
2 participants
@SimonSapin @upsuper
You can’t perform that action at this time.