Add separate specified value for keyword font sizes #16016

Manishearth · 2017-03-18T03:24:21Z

In Gecko, these keywords compute to different values depending on the
font.

See https://bugzilla.mozilla.org/show_bug.cgi?id=1341775

This change is

highfive · 2017-03-18T03:24:24Z

Heads up! This PR modifies the following files:

@bholley: components/style/properties/longhand/font.mako.rs, components/style/values/specified/length.rs
@KiChjang: components/script/dom/element.rs, components/script/dom/htmlfontelement.rs
@fitzgen: components/script/dom/element.rs, components/script/dom/htmlfontelement.rs
@emilio: components/style/properties/longhand/font.mako.rs, ports/geckolib/glue.rs, components/style/values/specified/length.rs

highfive · 2017-03-18T03:24:26Z

Warning

These commits modify unsafe code. Please review it carefully!
These commits modify style and script code, but no tests are modified. Please consider adding a test!

Manishearth · 2017-03-18T03:24:33Z

r? @upsuper

wafflespeanut · 2017-03-18T18:16:21Z

components/style/properties/longhand/font.mako.rs

        }
    }

    #[derive(Debug, Clone, PartialEq)]
    #[cfg_attr(feature = "servo", derive(HeapSizeOf))]
-    pub struct SpecifiedValue(pub specified::LengthOrPercentage);
+    pub enum SpecifiedValue {


Could use Either?

No, the ToComputedValue and parse impls are special.

Oops. Never mind :)

bors-servo · 2017-03-19T22:11:47Z

☔ The latest upstream changes (presumably #16014) made this pull request unmergeable. Please resolve the merge conflicts.

upsuper

Please address the comments.

upsuper · 2017-03-20T05:29:15Z

components/script/dom/htmlfontelement.rs

@@ -77,8 +80,12 @@ impl VirtualMethods for HTMLFontElement {
            &local_name!("face") => AttrValue::from_atomic(value.into()),
            &local_name!("color") => AttrValue::from_legacy_color(value.into()),
            &local_name!("size") => {
-                let length = parse_length(&value);
-                AttrValue::Length(value.into(), length)
+                let size = parse_size(&value);


Could we have a function to share the code here and above? It seems that a function to parse DOMString into AttrValue can be split out.

upsuper · 2017-03-20T05:29:15Z

components/style/properties/longhand/font.mako.rs

+        } else {
+            let ret = match_ignore_ascii_case! {&*input.expect_ident()?,
+                "smaller" => FontRelativeLength::Em(0.85),
+                "larger" => FontRelativeLength::Em(1.2),


You probably should keep these two as keyword values rather than converting them promptly. CSSOM should get them as keywords given existing browsers' behavior.

upsuper · 2017-03-20T05:29:15Z

components/style/properties/longhand/font.mako.rs

+    }
+
+    impl SpecifiedValue {
+        pub fn from_html_size(size: u8) -> Self {


Probably worth a comment here referring to HTML spec's related section and mentioning

If value is greater than 7, let it be 7.
If value is less than 1, let it be 1.

upsuper · 2017-03-20T05:29:15Z

components/style/properties/longhand/font.mako.rs

+                Large => "large",
+                XLarge => "x-large",
+                XXLarge => "xx-large",
+                XXXLarge => "",


If this is not going to happen, should we unreachable!() rather than silently return an empty string?

Manishearth · 2017-03-20T06:00:02Z

Updated

upsuper · 2017-03-20T06:03:35Z

@bors-servo r+

bors-servo · 2017-03-20T06:03:36Z

📌 Commit fe8338c has been approved by upsuper

bors-servo · 2017-03-20T06:03:39Z

⌛ Testing commit fe8338c with merge 04dab4a...


        Auto merge of #16016 - Manishearth:graft-font-size, r=upsuper

Add separate specified value for keyword font sizes In Gecko, these keywords compute to different values depending on the font. See https://bugzilla.mozilla.org/show_bug.cgi?id=1341775  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16016)

bors-servo · 2017-03-20T06:15:02Z

💔 Test failed - linux-dev

Manishearth · 2017-03-20T06:21:28Z

@bors-servo r=upsuper

bors-servo · 2017-03-20T06:21:29Z

📌 Commit 4a7fbfd has been approved by upsuper

bors-servo · 2017-03-20T06:21:32Z

⌛ Testing commit 4a7fbfd with merge 6b2a9d8...


        Auto merge of #16016 - Manishearth:graft-font-size, r=upsuper

Add separate specified value for keyword font sizes In Gecko, these keywords compute to different values depending on the font. See https://bugzilla.mozilla.org/show_bug.cgi?id=1341775  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16016)

bors-servo · 2017-03-20T06:47:51Z

💔 Test failed - linux-rel-wpt


        Add separate specified value for keyword font sizes

In Gecko, these keywords compute to different values depending on the font. See https://bugzilla.mozilla.org/show_bug.cgi?id=1341775

Manishearth · 2017-03-20T07:06:20Z

@bors-servo r=upsuper

bors-servo · 2017-03-20T07:06:20Z

📌 Commit c9198d9 has been approved by upsuper

bors-servo · 2017-03-20T07:28:48Z

⌛ Testing commit c9198d9 with merge 50fd39f...


        Auto merge of #16016 - Manishearth:graft-font-size, r=upsuper

Add separate specified value for keyword font sizes In Gecko, these keywords compute to different values depending on the font. See https://bugzilla.mozilla.org/show_bug.cgi?id=1341775  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16016)

bors-servo · 2017-03-20T08:08:51Z

☀️ 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-msvc-dev
Approved by: upsuper
Pushing 50fd39f to master...

highfive assigned asajeffrey Mar 18, 2017

highfive added the S-awaiting-review label Mar 18, 2017

highfive assigned upsuper and unassigned asajeffrey Mar 18, 2017

wafflespeanut reviewed Mar 18, 2017

View changes

highfive added the S-needs-rebase label Mar 19, 2017

Manishearth force-pushed the Manishearth:graft-font-size branch from d641624 to 1695a2b Mar 19, 2017

upsuper reviewed Mar 20, 2017

View changes

upsuper added S-needs-code-changes and removed S-needs-rebase labels Mar 20, 2017

Manishearth force-pushed the Manishearth:graft-font-size branch from 1695a2b to fe8338c Mar 20, 2017

highfive removed the S-needs-code-changes label Mar 20, 2017

highfive added S-awaiting-merge and removed S-awaiting-review labels Mar 20, 2017

highfive added S-tests-failed and removed S-awaiting-merge labels Mar 20, 2017

Manishearth force-pushed the Manishearth:graft-font-size branch from fe8338c to 4a7fbfd Mar 20, 2017

highfive added S-awaiting-review and removed S-tests-failed labels Mar 20, 2017

highfive added S-awaiting-merge and removed S-awaiting-review labels Mar 20, 2017

highfive added S-tests-failed and removed S-awaiting-merge labels Mar 20, 2017

Add separate specified value for keyword font sizes

Loading status checks…

c9198d9

In Gecko, these keywords compute to different values depending on the font. See https://bugzilla.mozilla.org/show_bug.cgi?id=1341775

Manishearth force-pushed the Manishearth:graft-font-size branch from 4a7fbfd to c9198d9 Mar 20, 2017

highfive added S-awaiting-review and removed S-tests-failed labels Mar 20, 2017

highfive added S-awaiting-merge and removed S-awaiting-review labels Mar 20, 2017

bors-servo merged commit c9198d9 into servo:master Mar 20, 2017
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Mar 20, 2017

Manishearth deleted the Manishearth:graft-font-size branch May 7, 2019

servo / servo

Add separate specified value for keyword font sizes #16016

Add separate specified value for keyword font sizes #16016

Manishearth commented Mar 18, 2017 •

edited by larsbergstrom

highfive commented Mar 18, 2017

highfive commented Mar 18, 2017

Manishearth commented Mar 18, 2017

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bors-servo commented Mar 19, 2017

upsuper left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Manishearth commented Mar 20, 2017

upsuper commented Mar 20, 2017

bors-servo commented Mar 20, 2017

bors-servo commented Mar 20, 2017

bors-servo commented Mar 20, 2017

Manishearth commented Mar 20, 2017

bors-servo commented Mar 20, 2017

bors-servo commented Mar 20, 2017

bors-servo commented Mar 20, 2017

Manishearth commented Mar 20, 2017

bors-servo commented Mar 20, 2017

bors-servo commented Mar 20, 2017

bors-servo commented Mar 20, 2017

servo / servo

Join GitHub today

Add separate specified value for keyword font sizes #16016

Add separate specified value for keyword font sizes #16016

Conversation

Manishearth commented Mar 18, 2017 • edited by larsbergstrom

highfive commented Mar 18, 2017

highfive commented Mar 18, 2017

Manishearth commented Mar 18, 2017

This comment has been minimized.

wafflespeanut Mar 18, 2017

This comment has been minimized.

Manishearth Mar 18, 2017

This comment has been minimized.

wafflespeanut Mar 18, 2017

bors-servo commented Mar 19, 2017

upsuper left a comment

This comment has been minimized.

upsuper Mar 20, 2017

This comment has been minimized.

upsuper Mar 20, 2017

This comment has been minimized.

upsuper Mar 20, 2017

This comment has been minimized.

upsuper Mar 20, 2017

Manishearth commented Mar 20, 2017

upsuper commented Mar 20, 2017

bors-servo commented Mar 20, 2017

bors-servo commented Mar 20, 2017

bors-servo commented Mar 20, 2017

Manishearth commented Mar 20, 2017

bors-servo commented Mar 20, 2017

bors-servo commented Mar 20, 2017

bors-servo commented Mar 20, 2017

Manishearth commented Mar 20, 2017

bors-servo commented Mar 20, 2017

bors-servo commented Mar 20, 2017

bors-servo commented Mar 20, 2017

Manishearth commented Mar 18, 2017 •

edited by larsbergstrom