Add support for `pre-wrap` and `pre-line` values for `white-space`. #7951

eefriedman · 2015-10-10T01:13:55Z

This is mostly straightforward. I had to modify a couple of places
which were accidentally discarding whitespace.

This fixes some relevant tests from the CSS testsuite... but a lot of
them are either manual, or don't pass because of unrelated issues. (For
example, white-space-mixed-002 renders correctly, but
white-space-mixed-002-ref doesn't because of a float bug.)

I'd appreciate any suggestions for how to go about adding tests for this.

highfive · 2015-10-10T01:14:00Z

Warning

These commits modify layout code, but no reftests are modified. Please consider adding a reftest!

pcwalton · 2015-10-10T14:37:26Z

components/gfx/text/util.rs

@@ -38,7 +38,7 @@ pub fn transform_text(text: &str,
                    output_text.push(ch);
                }
            }
-            text.len() > 0 && is_in_whitespace(text.char_at_reverse(0), mode)
+            false


Why this change?

This fixes https://github.com/servo/servo/blob/master/tests/wpt/css-tests/css21_dev/html4/white-space-mixed-002.htm and some related issues. The return value here basically means "should leading whitespace be discarded from the next fragment", and the answer is always no in white-space: pre or white-space: pre-wrap mode.

pcwalton · 2015-10-10T14:38:20Z

components/layout/fragment.rs

+            white_space::T::normal |
+            white_space::T::pre_wrap |
+            white_space::T::pre_line => true,
+        }


Matches very similar to this are duplicated throughout the code. Maybe we should have a whitespace_is_pre_or_nowrap() kind of function?

Actually, never mind, I see the matches are subtly different each time.

Hmm... there are basically three useful ways to divide up the values of white-space: "are spaces preserved", "are newlines preserved", and "is wrapping allowed". I could add helpers for all three, which might be useful for readability even if they don't have very many uses.

Yeah, I agree those would be useful distinctions for readability.

pcwalton · 2015-10-10T14:39:53Z

components/layout/fragment.rs

-                pieces_processed_count += 1;
+                debug!("calculate_split_position_using_breaking_strategy: enlarging span");
+                remaining_inline_size = remaining_inline_size - advance;
+                inline_start_range.extend_by(slice.range.length());


Why unconditionally adjust remaining_inline_size now?

Like the debug! log said, the first branch would trim leading whitespace. That doesn't really work out very well in pre-wrap mode, where whitespace is supposed to be preserved.

I was going to add the condition that the fragment isn't using a whitespace-preserving mode... but then I realized we never actually end up in that situation: we strip leading whitespace before we reach this code.

pcwalton · 2015-10-10T14:41:24Z

Looks good other than these couple of questions I had.

Ms2ger · 2015-10-11T14:55:17Z

This needs at least some automated testing. If you know which csswg-test tests this causes to pass, feel free to submit references upstream and ask me to review. We can land this PR once I've integrated those references.

eefriedman · 2015-10-11T22:28:29Z

Refactored the code a bit, and added some tests.

pcwalton · 2015-10-13T00:04:56Z

@bors-servo: r+

bors-servo · 2015-10-13T00:04:56Z

📌 Commit 9202ebc has been approved by pcwalton

pcwalton · 2015-10-13T00:04:59Z

Looks great, thanks!

bors-servo · 2015-10-13T00:05:00Z

⌛ Testing commit 9202ebc with merge 477440c...


        Auto merge of #7951 - eefriedman:white-space, r=pcwalton

Add support for `pre-wrap` and `pre-line` values for `white-space`. This is mostly straightforward. I had to modify a couple of places which were accidentally discarding whitespace. Fixes #1513. This fixes some relevant tests from the CSS testsuite... but a lot of them are either manual, or don't pass because of unrelated issues. (For example, white-space-mixed-002 renders correctly, but white-space-mixed-002-ref doesn't because of a float bug.) I'd appreciate any suggestions for how to go about adding tests for this.  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7951)

bors-servo · 2015-10-13T00:30:22Z

💔 Test failed - linux-dev

eefriedman · 2015-10-13T01:04:35Z

Looks like a legitimate bug in my patch.

eefriedman · 2015-10-13T02:10:03Z

Added a fix for newlines in white-space: pre mode. (See https://github.com/servo/servo/pull/7951/files#diff-013814e2f277c3fe9e1b72e33bd8ad2aL621) Patch should be good to go now.

eefriedman · 2015-10-15T19:29:08Z

@pcwalton Ping. This is ready to merge.

eefriedman · 2015-10-19T18:47:21Z

Rebased.

bors-servo · 2015-10-20T13:36:33Z

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


        Add support for `pre-wrap` and `pre-line` values for `white-space`.

This is mostly straightforward. I had to modify a couple of places which were accidentally discarding whitespace. Fixes #1513.

eefriedman · 2015-10-20T17:46:03Z

Rebased again.

@pcwalton ping.

pcwalton · 2015-10-20T18:38:52Z

@bors-servo: r+

bors-servo · 2015-10-20T18:38:53Z

📌 Commit 3a451ff has been approved by pcwalton

bors-servo · 2015-10-20T18:38:55Z

⌛ Testing commit 3a451ff with merge c3ab711...


        Auto merge of #7951 - eefriedman:white-space, r=pcwalton

Add support for `pre-wrap` and `pre-line` values for `white-space`. This is mostly straightforward. I had to modify a couple of places which were accidentally discarding whitespace. Fixes #1513. This fixes some relevant tests from the CSS testsuite... but a lot of them are either manual, or don't pass because of unrelated issues. (For example, white-space-mixed-002 renders correctly, but white-space-mixed-002-ref doesn't because of a float bug.) I'd appreciate any suggestions for how to go about adding tests for this.  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7951)

pcwalton · 2015-10-20T18:38:57Z

Sorry this fell through the cracks!

bors-servo · 2015-10-20T19:27:08Z

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt

highfive added the S-awaiting-review label Oct 10, 2015

nox closed this Oct 10, 2015

nox reopened this Oct 10, 2015

nox added the A-content/css label Oct 10, 2015

pcwalton reviewed Oct 10, 2015
View changes

jdm assigned pcwalton Oct 10, 2015

Ms2ger added the S-needs-tests label Oct 11, 2015

eefriedman force-pushed the eefriedman:white-space branch from 4093c9d to 9202ebc Oct 11, 2015

highfive added S-awaiting-merge and removed S-awaiting-review labels Oct 13, 2015

highfive added S-tests-failed and removed S-awaiting-merge labels Oct 13, 2015

eefriedman force-pushed the eefriedman:white-space branch from 9202ebc to c1655d6 Oct 13, 2015

highfive added S-awaiting-review and removed S-tests-failed labels Oct 13, 2015

jdm removed the S-needs-tests label Oct 15, 2015

eefriedman force-pushed the eefriedman:white-space branch from c1655d6 to 79109df Oct 19, 2015

highfive added S-needs-rebase and removed S-awaiting-merge labels Oct 20, 2015

Add support for `pre-wrap` and `pre-line` values for `white-space`.

Loading status checks…

3a451ff

This is mostly straightforward. I had to modify a couple of places which were accidentally discarding whitespace. Fixes #1513.

eefriedman force-pushed the eefriedman:white-space branch from 79109df to 3a451ff Oct 20, 2015

highfive removed S-awaiting-review S-needs-rebase labels Oct 20, 2015

highfive added the S-awaiting-merge label Oct 20, 2015

bors-servo merged commit 3a451ff into servo:master Oct 20, 2015
2 checks passed

2 checks passed

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

homu Test successful
Details

jdm mentioned this pull request Oct 26, 2015

twis39 servo/blog.servo.org#37

Merged

servo / servo

Add support for `pre-wrap` and `pre-line` values for `white-space`. #7951

Add support for `pre-wrap` and `pre-line` values for `white-space`. #7951

eefriedman commented Oct 10, 2015

highfive commented Oct 10, 2015

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.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

pcwalton commented Oct 10, 2015

Ms2ger commented Oct 11, 2015

eefriedman commented Oct 11, 2015

pcwalton commented Oct 13, 2015

bors-servo commented Oct 13, 2015

pcwalton commented Oct 13, 2015

bors-servo commented Oct 13, 2015

bors-servo commented Oct 13, 2015

eefriedman commented Oct 13, 2015

eefriedman commented Oct 13, 2015

eefriedman commented Oct 15, 2015

eefriedman commented Oct 19, 2015

bors-servo commented Oct 20, 2015

eefriedman commented Oct 20, 2015

pcwalton commented Oct 20, 2015

bors-servo commented Oct 20, 2015

bors-servo commented Oct 20, 2015

pcwalton commented Oct 20, 2015

bors-servo commented Oct 20, 2015

servo / servo

Join GitHub today

Add support for `pre-wrap` and `pre-line` values for `white-space`. #7951

Add support for `pre-wrap` and `pre-line` values for `white-space`. #7951

Conversation

eefriedman commented Oct 10, 2015

highfive commented Oct 10, 2015

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.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

pcwalton commented Oct 10, 2015

Ms2ger commented Oct 11, 2015

eefriedman commented Oct 11, 2015

pcwalton commented Oct 13, 2015

bors-servo commented Oct 13, 2015

pcwalton commented Oct 13, 2015

bors-servo commented Oct 13, 2015

bors-servo commented Oct 13, 2015

eefriedman commented Oct 13, 2015

eefriedman commented Oct 13, 2015

eefriedman commented Oct 15, 2015

eefriedman commented Oct 19, 2015

bors-servo commented Oct 20, 2015

eefriedman commented Oct 20, 2015

pcwalton commented Oct 20, 2015

bors-servo commented Oct 20, 2015

bors-servo commented Oct 20, 2015

pcwalton commented Oct 20, 2015

bors-servo commented Oct 20, 2015