Prefer length and percentage for word spacing #12783

wafflespeanut · 2016-08-09T10:07:56Z

The goal is to make use of LengthOrPercentage for word spacing in ShapingOptions, but since it makes use of f32 which doesn't implement Hash, we're going for NotNan<f32> from ordered-float, which supports hashing. Instead of implementing Hash for LengthOrPercentage and thereby the inner types like CSSFloat, CalcLengthOrPercentage, etc., we convert it to (Au, NotNan<f32>).

./mach build -d does not report any errors
./mach test-tidy does not report any errors

There are tests for these changes

This change is

highfive · 2016-08-09T10:08:03Z

Heads up! This PR modifies the following files:

@bholley: components/style/Cargo.toml, components/style/properties/longhand/inherited_text.mako.rs, components/style/lib.rs, components/style/values/computed/mod.rs

highfive · 2016-08-09T10:08:04Z

Warning

These commits modify style, layout, and gfx code, but no tests are modified. Please consider adding a test!

wafflespeanut · 2016-08-09T10:08:25Z

It requires tests (I guess?)

r? @Manishearth and @SimonSapin

Manishearth · 2016-08-09T10:09:37Z

components/gfx/font.rs

@@ -225,7 +226,8 @@ impl Font {

            let mut advance = Au::from_f64_px(self.glyph_h_advance(glyph_id));
            if character == ' ' {
-                advance += options.word_spacing;
+                let (length, percent) = options.word_spacing;
+                advance = (advance + length) + Au((advance.0 as f32 * percent.into_inner()) as i32);


Link to https://www.w3.org/TR/css-text-3/#word-spacing

This looks correct, but I'd like to wait for Simon's review

Manishearth · 2016-08-09T10:10:04Z

components/gfx/text/shaping/harfbuzz.rs

@@ -403,7 +403,8 @@ impl Shaper {
        // applied. The effect of the property on other word-separator characters is undefined."
        // We elect to only space the two required code points.
        if character == ' ' || character == '\u{a0}' {
-            advance = advance + options.word_spacing
+            let (length, percent) = options.word_spacing;
+            advance = (advance + length) + Au((advance.0 as f32 * percent.into_inner()) as i32);


Ditto about the spec link

Manishearth · 2016-08-09T10:11:14Z

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

@@ -285,23 +285,23 @@
    #[cfg_attr(feature = "servo", derive(HeapSizeOf))]
    pub enum SpecifiedValue {
        Normal,
-        Specified(specified::Length),  // FIXME(SimonSapin) support percentages
+        LengthOrPercentage(specified::LengthOrPercentage),


Don't rename it from Specified, let it be Specified(specified::Lop)

Manishearth · 2016-08-09T10:12:32Z

LGTM modulo nits.

r=me post @SimonSapin's sign-off

Manishearth · 2016-08-09T11:46:56Z

Post IRC discussion we know that your method of calculation is correct.

r=me

needs lockfile updates:

diff --git a/ports/geckolib/Cargo.lock b/ports/geckolib/Cargo.lock
index 405fadd..b60e592 100644
--- a/ports/geckolib/Cargo.lock
+++ b/ports/geckolib/Cargo.lock
@@ -215,6 +215,33 @@ dependencies = [
 ]

 [[package]]
+name = "num"
+version = "0.1.32"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+dependencies = [
+ "num-integer 0.1.32 (registry+https://github.com/rust-lang/crates.io-index)",
+ "num-iter 0.1.32 (registry+https://github.com/rust-lang/crates.io-index)",
+ "num-traits 0.1.32 (registry+https://github.com/rust-lang/crates.io-index)",
+]
+
+[[package]]
+name = "num-integer"
+version = "0.1.32"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+dependencies = [
+ "num-traits 0.1.32 (registry+https://github.com/rust-lang/crates.io-index)",
+]
+
+[[package]]
+name = "num-iter"
+version = "0.1.32"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+dependencies = [
+ "num-integer 0.1.32 (registry+https://github.com/rust-lang/crates.io-index)",
+ "num-traits 0.1.32 (registry+https://github.com/rust-lang/crates.io-index)",
+]
+
+[[package]]
 name = "num-traits"
 version = "0.1.32"
 source = "registry+https://github.com/rust-lang/crates.io-index"
@@ -228,6 +255,15 @@ dependencies = [
 ]

 [[package]]
+name = "ordered-float"
+version = "0.2.2"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+dependencies = [
+ "num 0.1.32 (registry+https://github.com/rust-lang/crates.io-index)",
+ "unreachable 0.0.2 (registry+https://github.com/rust-lang/crates.io-index)",
+]
+
+[[package]]
 name = "quickersort"
 version = "2.0.1"
 source = "registry+https://github.com/rust-lang/crates.io-index"
@@ -325,6 +361,7 @@ dependencies = [
  "log 0.3.6 (registry+https://github.com/rust-lang/crates.io-index)",
  "matches 0.1.2 (registry+https://github.com/rust-lang/crates.io-index)",
  "num-traits 0.1.32 (registry+https://github.com/rust-lang/crates.io-index)",
+ "ordered-float 0.2.2 (registry+https://github.com/rust-lang/crates.io-index)",
  "rand 0.3.14 (registry+https://github.com/rust-lang/crates.io-index)",
  "rustc-serialize 0.3.19 (registry+https://github.com/rust-lang/crates.io-index)",
  "selectors 0.7.0 (registry+https://github.com/rust-lang/crates.io-index)",
@@ -388,6 +425,14 @@ source = "registry+https://github.com/rust-lang/crates.io-index"

 [[package]]
 name = "unreachable"
+version = "0.0.2"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+dependencies = [
+ "void 0.0.5 (registry+https://github.com/rust-lang/crates.io-index)",
+]
+
+[[package]]
+name = "unreachable"
 version = "0.1.1"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 dependencies = [
@@ -427,6 +472,11 @@ dependencies = [

 [[package]]
 name = "void"
+version = "0.0.5"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+
+[[package]]
+name = "void"
 version = "1.0.2"
 source = "registry+https://github.com/rust-lang/crates.io-index"

wafflespeanut · 2016-08-09T12:07:37Z

lock file updated :)
@bors-servo r=Manishearth

Thanks! 😄

bors-servo · 2016-08-09T12:07:39Z

📌 Commit 3b131ee has been approved by Manishearth

bors-servo · 2016-08-09T12:07:42Z

⌛ Testing commit 3b131ee with merge 178adcd...


        Auto merge of #12783 - Wafflespeanut:word_spacing, r=Manishearth

Prefer length and percentage for word spacing  The goal is to make use of `LengthOrPercentage` for word spacing in `ShapingOptions`, but since it makes use of `f32` which doesn't implement `Hash`, we're going for `NotNan<f32>` from [ordered-float](https://github.com/reem/rust-ordered-float/), which supports hashing. Instead of implementing `Hash` for `LengthOrPercentage` and thereby the inner types like `CSSFloat`, `CalcLengthOrPercentage`, etc., we convert it to `(Au, NotNan<f32>)`. ---  - [x] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors  - [ ] There are tests for these changes   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12783)

bors-servo · 2016-08-09T12:19:14Z

💔 Test failed - linux-dev

wafflespeanut · 2016-08-09T12:22:18Z

forgot to update ports/cef/Cargo.lock - sorry


        Prefer length and percentage for word spacing

wafflespeanut · 2016-08-09T12:26:08Z

@bors-servo r=Manishearth

bors-servo · 2016-08-09T12:26:10Z

📌 Commit a04028e has been approved by Manishearth

bors-servo · 2016-08-09T12:26:12Z

⌛ Testing commit a04028e with merge 6142508...


        Auto merge of #12783 - Wafflespeanut:word_spacing, r=Manishearth

Prefer length and percentage for word spacing  The goal is to make use of `LengthOrPercentage` for word spacing in `ShapingOptions`, but since it makes use of `f32` which doesn't implement `Hash`, we're going for `NotNan<f32>` from [ordered-float](https://github.com/reem/rust-ordered-float/), which supports hashing. Instead of implementing `Hash` for `LengthOrPercentage` and thereby the inner types like `CSSFloat`, `CalcLengthOrPercentage`, etc., we convert it to `(Au, NotNan<f32>)`. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors  - [ ] There are tests for these changes   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12783)

bors-servo · 2016-08-09T13:15:51Z

💔 Test failed - linux-rel

highfive · 2016-08-09T13:15:54Z

  ▶ FAIL [expected PASS] /css-transforms-1_dev/html/transform3d-translatez-001.htm
  └   → /css-transforms-1_dev/html/transform3d-translatez-001.htm 902d90a8d198625335e738587fdfe81dcc90392d
/css-transforms-1_dev/html/reference/transform3d-translatez-ref.htm b14318ccfd8a59b7b249d41521e178d617678823
Testing 902d90a8d198625335e738587fdfe81dcc90392d == b14318ccfd8a59b7b249d41521e178d617678823
/css-transforms-1_dev/html/transform3d-translatez-001.htm 902d90a8d198625335e738587fdfe81dcc90392d
/css-transforms-1_dev/html/reference/transform3d-translatez-notref.htm 902d90a8d198625335e738587fdfe81dcc90392d
Testing 902d90a8d198625335e738587fdfe81dcc90392d != 902d90a8d198625335e738587fdfe81dcc90392d

wafflespeanut · 2016-08-09T13:17:08Z

@bors-servo retry #11631

bors-servo · 2016-08-09T15:13:31Z

⌛ Testing commit a04028e with merge 3b676bc...


        Auto merge of #12783 - Wafflespeanut:word_spacing, r=Manishearth

Prefer length and percentage for word spacing  The goal is to make use of `LengthOrPercentage` for word spacing in `ShapingOptions`, but since it makes use of `f32` which doesn't implement `Hash`, we're going for `NotNan<f32>` from [ordered-float](https://github.com/reem/rust-ordered-float/), which supports hashing. Instead of implementing `Hash` for `LengthOrPercentage` and thereby the inner types like `CSSFloat`, `CalcLengthOrPercentage`, etc., we convert it to `(Au, NotNan<f32>)`. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors  - [ ] There are tests for these changes   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12783)

bors-servo · 2016-08-09T16:05:54Z

☀️ Test successful - arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows-dev

highfive assigned cbrewster Aug 9, 2016

highfive added the S-awaiting-review label Aug 9, 2016

highfive assigned Manishearth and unassigned cbrewster Aug 9, 2016

Manishearth reviewed Aug 9, 2016
View changes

wafflespeanut force-pushed the wafflespeanut:word_spacing branch from 2c54092 to ee82ff6 Aug 9, 2016

wafflespeanut force-pushed the wafflespeanut:word_spacing branch from ee82ff6 to 3b131ee Aug 9, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Aug 9, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Aug 9, 2016

Prefer length and percentage for word spacing

Loading status checks…

a04028e

wafflespeanut force-pushed the wafflespeanut:word_spacing branch from 3b131ee to a04028e Aug 9, 2016

highfive added S-awaiting-review and removed S-tests-failed labels Aug 9, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Aug 9, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Aug 9, 2016

highfive added S-awaiting-merge and removed S-tests-failed labels Aug 9, 2016

bors-servo mentioned this pull request Aug 9, 2016

Inline split fixes#12699 #12760

Merged

4 of 5 tasks complete

bors-servo merged commit a04028e into servo:master Aug 9, 2016
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 Aug 9, 2016

bors-servo mentioned this pull request Aug 9, 2016

Update to selectors 0.8.2 (generic over atoms) #12571

Merged

4 of 5 tasks complete

wafflespeanut deleted the wafflespeanut:word_spacing branch Aug 9, 2016

servo / servo

Prefer length and percentage for word spacing #12783

Prefer length and percentage for word spacing #12783

wafflespeanut commented Aug 9, 2016 •

edited

highfive commented Aug 9, 2016

highfive commented Aug 9, 2016

wafflespeanut commented Aug 9, 2016 •

edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Manishearth commented Aug 9, 2016

Manishearth commented Aug 9, 2016 •

edited

wafflespeanut commented Aug 9, 2016

bors-servo commented Aug 9, 2016

bors-servo commented Aug 9, 2016

bors-servo commented Aug 9, 2016

wafflespeanut commented Aug 9, 2016 •

edited

wafflespeanut commented Aug 9, 2016

bors-servo commented Aug 9, 2016

bors-servo commented Aug 9, 2016

bors-servo commented Aug 9, 2016

highfive commented Aug 9, 2016

wafflespeanut commented Aug 9, 2016

bors-servo commented Aug 9, 2016

bors-servo commented Aug 9, 2016

servo / servo

Join GitHub today

Prefer length and percentage for word spacing #12783

Prefer length and percentage for word spacing #12783

Conversation

wafflespeanut commented Aug 9, 2016 • edited

highfive commented Aug 9, 2016

highfive commented Aug 9, 2016

wafflespeanut commented Aug 9, 2016 • edited

This comment has been minimized.

Manishearth Aug 9, 2016 • edited

This comment has been minimized.

Manishearth Aug 9, 2016

This comment has been minimized.

Manishearth Aug 9, 2016

Manishearth commented Aug 9, 2016

Manishearth commented Aug 9, 2016 • edited

wafflespeanut commented Aug 9, 2016

bors-servo commented Aug 9, 2016

bors-servo commented Aug 9, 2016

bors-servo commented Aug 9, 2016

wafflespeanut commented Aug 9, 2016 • edited

wafflespeanut commented Aug 9, 2016

bors-servo commented Aug 9, 2016

bors-servo commented Aug 9, 2016

bors-servo commented Aug 9, 2016

highfive commented Aug 9, 2016

wafflespeanut commented Aug 9, 2016

bors-servo commented Aug 9, 2016

bors-servo commented Aug 9, 2016

wafflespeanut commented Aug 9, 2016 •

edited

wafflespeanut commented Aug 9, 2016 •

edited

Manishearth Aug 9, 2016 •

edited

Manishearth commented Aug 9, 2016 •

edited

wafflespeanut commented Aug 9, 2016 •

edited