Make writing-mode affect computed display #16044

bd339 · 2017-03-20T13:14:38Z

The first manual test-case in #15754 passes now, but the second test-case still renders "Text" horizontally, which is apparently because of servo's experimental support for writing-mode.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #15754 (github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because _____

This change is

highfive · 2017-03-20T13:14:43Z

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @emilio (or someone else) soon.

highfive · 2017-03-20T13:14:44Z

Heads up! This PR modifies the following files:

@bholley: components/style/properties/properties.mako.rs
@emilio: components/style/properties/properties.mako.rs

highfive · 2017-03-20T13:14:45Z

Warning

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

emilio

We need to add a test for this. Even if the rendering it's not the expected one, we can test this with getComputedStyle, let's write a WPT test that does that.

emilio · 2017-03-20T13:46:49Z

components/style/properties/properties.mako.rs

+        use computed_values::display::T as display;
+        if context.layout_parent_style.writing_mode != style.writing_mode &&
+           style.get_box().clone_display() == display::inline {
+               style.mutate_box().set_display(display::inline_block);


nit: Indent this and the following line to the left, four spaces from the if.

emilio · 2017-03-20T13:46:49Z

components/style/properties/properties.mako.rs

@@ -2115,6 +2115,14 @@ pub fn apply_declarations<'a, F, I>(device: &Device,
    }

    {
+        use computed_values::display::T as display;
+        if context.layout_parent_style.writing_mode != style.writing_mode &&


Let's add here:

A quote and link to the spec.

A link to Boris' www-style mail.

(optionally) A link to the issue we're fixing to see the discussion.


        Make writing-mode affect computed display

If a box has a different writing-mode than its containing block, and has a specified display of inline, change the computed display to inline-block. Also adds the second manual testcase from #15754 as a WPT to assert that the computed display does in fact change.

bd339 · 2017-03-23T17:54:37Z

r?@emilio

emilio · 2017-03-23T18:24:12Z

Awesome, thank you for doing this!

@bors-servo r+

bors-servo · 2017-03-23T18:24:13Z

📌 Commit 8db0fca has been approved by emilio

bors-servo · 2017-03-23T18:39:38Z

⌛ Testing commit 8db0fca with merge eb0bb87...


        Auto merge of #16044 - bd339:iss15754, r=emilio

Make writing-mode affect computed display  The first manual test-case in #15754 passes now, but the second test-case still renders "Text" horizontally, which is apparently because of servo's experimental support for writing-mode. ---  - [x] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #15754 (github issue number if applicable).  - [X] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16044)

bors-servo · 2017-03-23T19:08:00Z

💔 Test failed - linux-rel-wpt

bd339 · 2017-03-24T09:38:51Z

Was that test failure because of these changes @emilio ?

jdm · 2017-03-24T13:57:55Z

@bors-servo: retry

#14323
It was not.

bors-servo · 2017-03-24T13:58:00Z

⌛ Testing commit 8db0fca with merge eb4a1e75d0acf613e390787f6b154240fa2b0eca...

bors-servo · 2017-03-24T14:24:15Z

💔 Test failed - linux-rel-wpt

jdm · 2017-03-24T14:51:28Z

@bors-servo: retry

#14323

bors-servo · 2017-03-24T15:05:38Z

⌛ Testing commit 8db0fca with merge 0f2c71a...


        Auto merge of #16044 - bd339:iss15754, r=emilio

Make writing-mode affect computed display  The first manual test-case in #15754 passes now, but the second test-case still renders "Text" horizontally, which is apparently because of servo's experimental support for writing-mode. ---  - [x] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #15754 (github issue number if applicable).  - [X] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16044)

bors-servo · 2017-03-24T15:47:39Z

☀️ 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: emilio
Pushing 0f2c71a to master...

bzbarsky · 2017-03-24T17:20:29Z

This broke a bunch of Gecko tests, because it's examining the value of the "writing mode" as defined at https://drafts.csswg.org/css-writing-modes-3/#writing-mode whereas it should be examining the value of the writing-mode property as defined at https://drafts.csswg.org/css-writing-modes-3/#propdef-writing-mode

As a simple example, this testcase: <span direction="ltr">some <span direction="rtl">long text that needs to wrap</span></span> gets broken because it makes the inner span an inline-block and then the wrapping is all wrong.

Emilio told me how to fix; I'll create a PR once I verify the fix actually does the right thing.

bzbarsky · 2017-03-24T17:21:15Z

But I don't have time today to figure out how to run servo reftests and in particular how to add new ones.

We really need to figure out some way to share reftests with Gecko. :(

bzbarsky · 2017-03-24T20:05:16Z

#16123 merged.

I really wish the CSS spec didn't have this naming confusion...

bzbarsky · 2017-03-24T20:29:42Z

For what it's worth, this commit made the following previously-failing Gecko tests pass: layout/reftests/css-display/display-contents-writing-mode-2.html , layout/reftests/writing-mode/1134849-orthogonal-inline.html, layout/reftests/writing-mode/1196887-1-computed-display-inline-block.html

which is excellent. ;)

highfive assigned emilio Mar 20, 2017

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

emilio requested changes Mar 20, 2017

View changes

jdm added S-needs-code-changes and removed S-awaiting-review labels Mar 20, 2017

bd339 force-pushed the bd339:iss15754 branch from c804070 to 8db0fca Mar 20, 2017

highfive added S-awaiting-review and removed S-needs-code-changes labels Mar 20, 2017

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

emilio approved these changes Mar 23, 2017

View changes

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

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

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

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

bors-servo merged commit 8db0fca into servo:master Mar 24, 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 24, 2017

emilio mentioned this pull request Mar 24, 2017

Fix writing-mode display fixup to check the right condition. #16123

Merged

2 of 4 tasks complete

servo / servo

Make writing-mode affect computed display #16044

Make writing-mode affect computed display #16044

bd339 commented Mar 20, 2017 •

edited by larsbergstrom

highfive commented Mar 20, 2017

highfive commented Mar 20, 2017

highfive commented Mar 20, 2017

emilio left a comment

This comment has been minimized.

This comment has been minimized.

bd339 commented Mar 23, 2017

emilio commented Mar 23, 2017

bors-servo commented Mar 23, 2017

bors-servo commented Mar 23, 2017

bors-servo commented Mar 23, 2017

bd339 commented Mar 24, 2017

jdm commented Mar 24, 2017

bors-servo commented Mar 24, 2017

bors-servo commented Mar 24, 2017

jdm commented Mar 24, 2017

bors-servo commented Mar 24, 2017

bors-servo commented Mar 24, 2017

bzbarsky commented Mar 24, 2017

bzbarsky commented Mar 24, 2017

bzbarsky commented Mar 24, 2017

bzbarsky commented Mar 24, 2017

servo / servo

Join GitHub today

Make writing-mode affect computed display #16044

Make writing-mode affect computed display #16044

Conversation

bd339 commented Mar 20, 2017 • edited by larsbergstrom

highfive commented Mar 20, 2017

highfive commented Mar 20, 2017

highfive commented Mar 20, 2017

emilio left a comment

This comment has been minimized.

emilio Mar 20, 2017

This comment has been minimized.

emilio Mar 20, 2017

bd339 commented Mar 23, 2017

emilio commented Mar 23, 2017

bors-servo commented Mar 23, 2017

bors-servo commented Mar 23, 2017

bors-servo commented Mar 23, 2017

bd339 commented Mar 24, 2017

jdm commented Mar 24, 2017

bors-servo commented Mar 24, 2017

bors-servo commented Mar 24, 2017

jdm commented Mar 24, 2017

bors-servo commented Mar 24, 2017

bors-servo commented Mar 24, 2017

bzbarsky commented Mar 24, 2017

bzbarsky commented Mar 24, 2017

bzbarsky commented Mar 24, 2017

bzbarsky commented Mar 24, 2017

bd339 commented Mar 20, 2017 •

edited by larsbergstrom