Support logical properties in style #14120

Manishearth · 2016-11-07T23:06:59Z

Adds support for the logical block-end/inline-start/etc properties. These properties (like border-block-end-color) map to "physical" properties (e.g. border-top-color) depending on the writing mode.

Todo:

Handle shorthands
Make geckolib setters work
Handle padding/offset logical properties
Perhaps handle -block-size, -inline-size type logical properties?
Tests?

This will overall add 16 new longhands and 4 new shorthands, taking a big bite out of the remaining properties work

f? @emilio @SimonSapin

This change is

highfive · 2016-11-07T23:07:03Z

Heads up! This PR modifies the following files:

@bholley: components/style/properties/properties.mako.rs, components/style/properties/longhand/border.mako.rs, components/style/properties/helpers.mako.rs, components/style/properties/data.py
@KiChjang: components/script/dom/webidls/CSSStyleDeclaration.webidl
@fitzgen: components/script/dom/webidls/CSSStyleDeclaration.webidl
@emilio: components/style/properties/properties.mako.rs, components/style/properties/longhand/border.mako.rs, components/style/properties/helpers.mako.rs, components/style/properties/data.py

highfive · 2016-11-07T23:07:04Z

Warning

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

Manishearth · 2016-11-07T23:08:47Z

I'm unsure about the cascade behavior; currently the logical properties cascade normally, but their set_foo function redirects. I'm not sure if more changes need to be made here.

In particular, I'm not sure how to handle inherit values.

Manishearth · 2016-11-08T01:17:25Z

Manually tested a bit, seems to work. Some prefs need to be enabled for writing-mode to work.

Manishearth · 2016-11-08T20:32:32Z

I created a nice testcase for this, which shows up as (in pure gecko)

(Styled version of https://drafts.csswg.org/css-writing-modes/#logical-to-physical)

<!DOCTYPE html>
<html>
<head>
<style> 

#block-start div.outer div {
  border-block-start-color: red;
  border-block-start-width: 2px;
  border-block-start-style: dotted;
}
#block-end div.outer div {
  border-block-end-color: red;
  border-block-end-width: 2px;
  border-block-end-style: dotted;
}
#inline-start div.outer div {
  border-inline-start-color: red;
  border-inline-start-width: 2px;
  border-inline-start-style: dotted;
}
#inline-end div.outer div {
  border-inline-end-color: red;
  border-inline-end-width: 2px;
  border-inline-end-style: dotted;
}

#body tr td:nth-child(2) div {
  writing-mode: horizontal-tb;
}
#body tr td:nth-child(3) div {
  writing-mode: horizontal-tb;
}
#body tr td:nth-child(4) div {
  writing-mode: vertical-rl;
}
#body tr td:nth-child(5) div {
  writing-mode: vertical-rl;
}
#body tr td:nth-child(6) div {
  writing-mode: vertical-lr;
}
#body tr td:nth-child(7) div {
  writing-mode: vertical-lr;
}
/* gecko */

#body tr td:nth-child(8) div {
  writing-mode: sideways-lr;
}
#body tr td:nth-child(9) div {
  writing-mode: sideways-lr;
}

/* servo */
/*
#body tr td:nth-child(8) div {
  writing-mode: vertical-lr;
  text-orientation: sideways;
}
#body tr td:nth-child(9) div {
  writing-mode: vertical-lr;
  text-orientation: sideways;
}
*/

#body tr td:nth-child(2n+2) div {
  direction: ltr;
}
#body tr td:nth-child(2n+3) div {
  direction: rtl;
}
td {
  padding: 50px;
  background-color: yellow;
}
.top {
  border-bottom: 2px dotted green;
}
.right {
  border-left: 2px dotted green;

}

.bottom {
  border-top: 2px dotted green;

}
.left {
  border-right: 2px dotted green;

}

</style>
</head>
<body>
<table>
<thead>
  <tr><th>writing mode</th><th colspan=2>horizontal-tb</th><th colspan=2>vertical-rl</th><th colspan=2>vertical-lr</th><th colspan=2>sideways-lr</th></tr>
  <tr><th>direction</th><th>ltr</th><th>rtl</th><th>ltr</th><th>rtl</th><th>ltr</th><th>rtl</th><th>ltr</th><th>rtl</th></tr>
</thead>
<tbody id=body>
  <tr id=block-start><th>block-start</th>   <td><div class=outer><div class=top>top</div></div></td><td><div class=outer><div class=top>top</div></div></td><td><div class=outer><div class=right>right</div></div></td><td><div class=outer><div class=right>right</div></div></td><td><div class=outer><div class=left>left</div></div></td><td><div class=outer><div class=left>left</div></div></td><td><div class=outer><div class=left>left</div></div></td><td><div class=outer><div class=left>left</div></div></td></tr>
  <tr id=block-end><th>block-end</th>       <td><div class=outer><div class=bottom>bottom</div></div></td><td><div class=outer><div class=bottom>bottom</div></div></td><td><div class=outer><div class=left>left</div></div></td><td><div class=outer><div class=left>left</div></div></td><td><div class=outer><div class=right>right</div></div></td><td><div class=outer><div class=right>right</div></div></td><td><div class=outer><div class=right>right</div></div></td><td><div class=outer><div class=right>right</div></div></td></tr>
  <tr id=inline-start><th>inline-start</th> <td><div class=outer><div class=left>left</div></div></td><td><div class=outer><div class=right>right</div></div></td><td><div class=outer><div class=top>top</div></div></td><td><div class=outer><div class=bottom>bottom</div></div></td><td><div class=outer><div class=top>top</div></div></td><td><div class=outer><div class=bottom>bottom</div></div></td><td><div class=outer><div class=bottom>bottom</div></div></td><td><div class=outer><div class=top>top</div></div></td></tr>
  <tr id=inline-end><th>inline-end</th>     <td><div class=outer><div class=right>right</div></div></td><td><div class=outer><div class=left>left</div></div></td><td><div class=outer><div class=bottom>bottom</div></div></td><td><div class=outer><div class=top>top</div></div></td><td><div class=outer><div class=bottom>bottom</div></div></td><td><div class=outer><div class=top>top</div></div></td><td><div class=outer><div class=top>top</div></div></td><td><div class=outer><div class=bottom>bottom</div></div></td></tr>
</tbody>
</table>

</body>
</html>

The test passes when the green dotted line is opposite the red one.

Servo doesn't support writing-mode: sideways-* (but does support text-orientation: sideways and uses it in the WritingMode struct to determine box flow (this is wrong)), so you have to uncomment the servo decl blocks (and comment the gecko ones) for this to work in servo.

However, writing modes in general are broken in Servo layout so instead we get something like

After inspecting this a bit more and looking at individual cases it seems like this PR handles the logical mapping correctly, except for inline-* in vertical-lr mode. Small testcase:

<!DOCTYPE html>
<html>
<head>
<style> 

#testing {
  writing-mode: vertical-lr;
  width: 300px;
  height: 300px;
  position: relative;
  top: 100px;
  left: 100px;

  background-color: yellow;
}
#testing div {

  border-inline-start-color: red;
  border-inline-start-width: 5px;
  border-inline-start-style: dotted;
  border-top: blue 5px dotted;
  width: 100px;
  height: 100px;
  background-color: green;
}
</style>
</head>
<body>
<div id=testing>
<div>AAAAA </div>
</div>

<script type="text/javascript">

  console.log(getComputedStyle(document.querySelector("#testing div")).borderBottomColor);
</script>


</body>
</html>

Should print the default color (white or currentColor) to the console, instead prints red. When displaying (hard to get it to display), the blue border should coincide with the red one, but they're on opposite sides in Servo.

Probably just needs swapping in the WritingMode methods. Though we may need more changes to support sideways-*.

Manishearth · 2016-11-08T21:11:59Z

Hm, when I try in stylo the bug goes away. I think it's just servo doing crazy things with the layout.

Manishearth · 2016-11-08T21:48:32Z

Filed #14144 for the writing mode stuff.

Manishearth · 2016-11-08T22:15:39Z

@bors-servo try

Manishearth · 2016-11-08T22:38:53Z

This PR is more or less complete. Manually tested in stylo and works. Servo regurgitates writing modes so it's a bit harder to test, but no reason why it shouldn't work. The current test is manual; I'll see if I can get it into the CSS wg testsuite. A similar test can be written with JS and getComputedStyle, not sure if it's worth it.

r? @SimonSapin

Manishearth · 2016-11-08T23:10:23Z

Reftests put up in https://bugzilla.mozilla.org/show_bug.cgi?id=1316172 . The exact reftest won't work for stylo until we sort out #14144.

Manishearth · 2016-11-10T22:30:56Z

@bors-servo try

bors-servo · 2016-11-10T22:30:58Z

⌛ Trying commit 0d8266c with merge 942cd30...


        Auto merge of #14120 - Manishearth:logical, r=<try>

@emilio

Support logical properties in style Adds support for the logical block-end/inline-start/etc properties. These properties (like `border-block-end-color`) map to "physical" properties (e.g. `border-top-color`) depending on the writing mode. Todo: - [x] Handle shorthands - [x] Make geckolib setters work - [x] Handle padding/offset logical properties - [x] Perhaps handle `-block-size`, `-inline-size` type logical properties? - [x] Tests? This will overall add 16 new longhands and 4 new shorthands, taking a big bite out of the [remaining properties work](https://manishearth.github.io/css-properties-list/?stylo=hide&servo=hide&firefox=only&chrome=show&mdn=false&alexa=false) f? @emilio @SimonSapin  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14120)

bors-servo · 2016-11-10T22:51:12Z

💔 Test failed - linux-dev

emilio

r=me with those nits addressed :)

emilio · 2016-11-11T12:13:09Z

components/style/properties/data.py

+    if isinstance(arg, bool):
+        return arg
+    else:
+        assert arg == "True" or arg == "False"


nit: No need for the else branch, you can just dedent this block. Also, assert arg in ["True", "False"]?

emilio · 2016-11-11T12:13:09Z

components/style/properties/helpers.mako.rs

+        if len(maybe_side) == 1:
+            side = maybe_side[0]
+        elif len(maybe_size) == 1:
+            size = maybe_size[0]


Probably just assert size is not None or side is not None, and just use else below?

We lose the custom error message then. Should I?

emilio · 2016-11-11T12:13:09Z

components/style/properties/helpers.mako.rs

+</%def>
+
+<%def name="logical_setter(name, need_clone=False)">
+


nit: extra newline?

emilio · 2016-11-11T12:13:09Z

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

+% for (size, logical) in ALL_SIZES:
+    // width, height, block-size, inline-size
+    ${helpers.predefined_type("%s" % size,
+                            "LengthOrPercentageOrAuto",


nit: Alignment, here and below.


        Support logical properties


        Add logical padding, margin longhands and border shorthands


        Add support for inline-size, block-size, and max-*-size/min-*-size

Manishearth · 2016-11-11T19:56:46Z

Addressed, except for changing the exception to an assert

emilio · 2016-11-11T19:58:16Z

Ok, it's fine :)

@bors-servo r+

bors-servo · 2016-11-11T19:58:18Z

📌 Commit 5fad7b0 has been approved by emilio

bors-servo · 2016-11-11T21:41:09Z

⌛ Testing commit 5fad7b0 with merge 4b9693c...


        Auto merge of #14120 - Manishearth:logical, r=emilio

@emilio

Support logical properties in style Adds support for the logical block-end/inline-start/etc properties. These properties (like `border-block-end-color`) map to "physical" properties (e.g. `border-top-color`) depending on the writing mode. Todo: - [x] Handle shorthands - [x] Make geckolib setters work - [x] Handle padding/offset logical properties - [x] Perhaps handle `-block-size`, `-inline-size` type logical properties? - [x] Tests? This will overall add 16 new longhands and 4 new shorthands, taking a big bite out of the [remaining properties work](https://manishearth.github.io/css-properties-list/?stylo=hide&servo=hide&firefox=only&chrome=show&mdn=false&alexa=false) f? @emilio @SimonSapin  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14120)

bors-servo · 2016-11-11T22:35:32Z

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

highfive assigned metajack Nov 7, 2016

highfive added the S-awaiting-review label Nov 7, 2016

Manishearth force-pushed the Manishearth:logical branch 2 times, most recently from 6068179 to 933ae4f Nov 8, 2016

Manishearth force-pushed the Manishearth:logical branch from 933ae4f to 743bd0c Nov 8, 2016

Manishearth changed the title ~~[WIP] Support logical properties in style~~ Support logical properties in style Nov 8, 2016

highfive assigned SimonSapin and unassigned metajack Nov 8, 2016

Manishearth force-pushed the Manishearth:logical branch from 46e24d0 to 0d8266c Nov 10, 2016

highfive added the S-tests-failed label Nov 10, 2016

Manishearth force-pushed the Manishearth:logical branch from 0d8266c to a9736a3 Nov 10, 2016

highfive removed the S-tests-failed label Nov 10, 2016

emilio approved these changes Nov 11, 2016

View changes

Manishearth added 3 commits Nov 7, 2016

Support logical properties

e34eb13

Add logical padding, margin longhands and border shorthands

f974719

Add support for inline-size, block-size, and max-*-size/min-*-size

Loading status checks…

5fad7b0

Manishearth force-pushed the Manishearth:logical branch from a9736a3 to 5fad7b0 Nov 11, 2016

highfive assigned emilio and unassigned SimonSapin Nov 11, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Nov 11, 2016

bors-servo merged commit 5fad7b0 into servo:master Nov 11, 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 Nov 11, 2016

Manishearth mentioned this pull request Nov 16, 2016

Support keyword values for min- and max-width #13821

Closed

Manishearth deleted the Manishearth:logical branch Feb 17, 2017

servo / servo

Support logical properties in style #14120

Support logical properties in style #14120

Manishearth commented Nov 7, 2016 •

edited

highfive commented Nov 7, 2016

highfive commented Nov 7, 2016

Manishearth commented Nov 7, 2016

Manishearth commented Nov 8, 2016

Manishearth commented Nov 8, 2016 •

edited

Manishearth commented Nov 8, 2016 •

edited

Manishearth commented Nov 8, 2016 •

edited

Manishearth commented Nov 8, 2016

Manishearth commented Nov 8, 2016

Manishearth commented Nov 8, 2016

Manishearth commented Nov 10, 2016

bors-servo commented Nov 10, 2016

bors-servo commented Nov 10, 2016

emilio left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Manishearth commented Nov 11, 2016

emilio commented Nov 11, 2016

bors-servo commented Nov 11, 2016

bors-servo commented Nov 11, 2016

bors-servo commented Nov 11, 2016

		</%def>

		<%def name="logical_setter(name, need_clone=False)">

servo / servo

Join GitHub today

Support logical properties in style #14120

Support logical properties in style #14120

Conversation

Manishearth commented Nov 7, 2016 • edited

highfive commented Nov 7, 2016

highfive commented Nov 7, 2016

Manishearth commented Nov 7, 2016

Manishearth commented Nov 8, 2016

Manishearth commented Nov 8, 2016 • edited

Manishearth commented Nov 8, 2016 • edited

Manishearth commented Nov 8, 2016 • edited

Manishearth commented Nov 8, 2016

Manishearth commented Nov 8, 2016

Manishearth commented Nov 8, 2016

Manishearth commented Nov 10, 2016

bors-servo commented Nov 10, 2016

bors-servo commented Nov 10, 2016

emilio left a comment

This comment has been minimized.

emilio Nov 11, 2016

This comment has been minimized.

emilio Nov 11, 2016

This comment has been minimized.

Manishearth Nov 11, 2016

This comment has been minimized.

emilio Nov 11, 2016

This comment has been minimized.

emilio Nov 11, 2016

Manishearth commented Nov 11, 2016

emilio commented Nov 11, 2016

bors-servo commented Nov 11, 2016

bors-servo commented Nov 11, 2016

bors-servo commented Nov 11, 2016

Manishearth commented Nov 7, 2016 •

edited

Manishearth commented Nov 8, 2016 •

edited

Manishearth commented Nov 8, 2016 •

edited

Manishearth commented Nov 8, 2016 •

edited