Don't mark the first/last fragment of an {ib} split as FIRST/LAST_FRAGMENT_OF_ELEMENT. #14035

Permutatrix · 2016-11-03T03:41:48Z

This change allows inline margins, borders, and padding to interact correctly with {ib} splits.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #14030
There are tests for these changes

This change is


        Don't mark start/end of {ib} split as FIRST/LAST_FRAGMENT_OF_ELEMENT

highfive · 2016-11-03T03:41:53Z

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

highfive · 2016-11-03T03:41:54Z

Heads up! This PR modifies the following files:

@emilio: components/layout/construct.rs

emilio · 2016-11-03T09:46:09Z

@bors-servo try

bors-servo · 2016-11-03T09:46:15Z

⌛ Trying commit 8279114 with merge 744cf46...


        Auto merge of #14035 - Permutatrix:iss-14030, r=<try>

Don't mark the first/last fragment of an {ib} split as FIRST/LAST_FRAGMENT_OF_ELEMENT.  This change allows inline margins, borders, and padding to interact correctly with {ib} splits. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #14030  - [X] 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/14035)

emilio · 2016-11-03T10:14:56Z

components/layout/construct.rs

@@ -222,7 +248,7 @@ impl InlineFragmentsAccumulator {
                pseudo: node.get_pseudo_element_type().strip(),
                style: node.style(style_context),
                selected_style: node.selected_style(),
-                flags: InlineFragmentNodeFlags::empty(),
+                flags: FIRST_FRAGMENT_OF_ELEMENT | LAST_FRAGMENT_OF_ELEMENT,


This looks good to me, but I wonder if there's a way to organize this so this doesn't start with this flags. It feels weird to use these flags here since we always remove them in to_intermediate_inline_fragments.

Maybe keeping in the accumulator something like saw_ib_split flag or similar, and don't insert the flags in that case?

I think of InlineFragmentsAccumulator's enclosing_node as representing the "whole" InlineFragmentNodeInfo for this accumulator, i.e. what it would be if it was all one fragment. It's split in two each time an {ib} split is encountered, then split up again in to_intermediate_inline_fragments. I'm not convinced that it's conceptually more reasonable to save instructions for how to mark the nodes and follow them as late as possible rather than simply handling it this way as we go.

Practically speaking, Servo will one day have to handle the positioning of backgrounds across multiple line boxes. Information about this will likely be stored in InlineFragmentNodeInfo. Using a conceptual model that allows that information to be reused will make it easier.

Comments would definitely help, though. If we stick with my model, I'll add some explaining it.

Actually, I suppose information about positioning backgrounds won't be split at this stage. But, uh, you know... Something else could be. In theory.

I concur with @Permutatrix; let's make things as simple as possible. But definitely comment this explaining how things work and the rationale.

bors-servo · 2016-11-03T11:00:20Z

💔 Test failed - linux-rel-css

highfive · 2016-11-03T11:00:22Z

  ▶ FAIL [expected PASS] /css-text-3_dev/html/css3-text-line-break-opclns-015.htm
  └   → /css-text-3_dev/html/css3-text-line-break-opclns-015.htm f8e79ff73dc35295804b13d87eab4cd865e38b1d
/css-text-3_dev/html/reference/css3-text-line-break-opclns-015-ref.htm 7c6a28563e81830e495a39b5f3e330a451b159bb
Testing f8e79ff73dc35295804b13d87eab4cd865e38b1d == 7c6a28563e81830e495a39b5f3e330a451b159bb

  ▶ FAIL [expected PASS] /css-text-3_dev/html/css3-text-line-break-opclns-014.htm
  └   → /css-text-3_dev/html/css3-text-line-break-opclns-014.htm 61f979a39fbbfa31e4ea1a6c17f6750c8f3725ed
/css-text-3_dev/html/reference/css3-text-line-break-opclns-014-ref.htm 491175f313283d2b697821fcd32f219d7c893538
Testing 61f979a39fbbfa31e4ea1a6c17f6750c8f3725ed == 491175f313283d2b697821fcd32f219d7c893538

  ▶ FAIL [expected PASS] /css-text-3_dev/html/css3-text-line-break-opclns-016.htm
  └   → /css-text-3_dev/html/css3-text-line-break-opclns-016.htm 8c2d1348e1986079d4444079a09494265a0ed12b
/css-text-3_dev/html/reference/css3-text-line-break-opclns-016-ref.htm 3554fe6375cd9c7f5c7485500ba5c60d79bb6958
Testing 8c2d1348e1986079d4444079a09494265a0ed12b == 3554fe6375cd9c7f5c7485500ba5c60d79bb6958

  ▶ FAIL [expected PASS] /css-text-3_dev/html/css3-text-line-break-opclns-020.htm
  └   → /css-text-3_dev/html/css3-text-line-break-opclns-020.htm e5012808fba17daa51408aa2ac69eaa413ad2333
/css-text-3_dev/html/reference/css3-text-line-break-opclns-020-ref.htm d4babe9b789027d9e320564b6b5c4966099b6da4
Testing e5012808fba17daa51408aa2ac69eaa413ad2333 == d4babe9b789027d9e320564b6b5c4966099b6da4

  ▶ FAIL [expected PASS] /css-text-3_dev/html/css3-text-line-break-opclns-019.htm
  └   → /css-text-3_dev/html/css3-text-line-break-opclns-019.htm de8db60f480534ce37fe1474fd0fffd09ce2cdc0
/css-text-3_dev/html/reference/css3-text-line-break-opclns-019-ref.htm 2148c53841ae125941772b5a73d0a9f72f7de20f
Testing de8db60f480534ce37fe1474fd0fffd09ce2cdc0 == 2148c53841ae125941772b5a73d0a9f72f7de20f

  ▶ FAIL [expected PASS] /css-text-3_dev/html/css3-text-line-break-opclns-018.htm
  └   → /css-text-3_dev/html/css3-text-line-break-opclns-018.htm fb4f5452a6274f73f0f0c14bd583f6c83039d4ff
/css-text-3_dev/html/reference/css3-text-line-break-opclns-018-ref.htm 8c1ae6293027f68fa0de8e73f7b94e8a95fed5ec
Testing fb4f5452a6274f73f0f0c14bd583f6c83039d4ff == 8c1ae6293027f68fa0de8e73f7b94e8a95fed5ec

  ▶ FAIL [expected PASS] /css-text-3_dev/html/css3-text-line-break-opclns-023.htm
  └   → /css-text-3_dev/html/css3-text-line-break-opclns-023.htm 84976e54167032f80607e64ae9700bd1978c6f30
/css-text-3_dev/html/reference/css3-text-line-break-opclns-023-ref.htm 31b0f5b321d7129fa9e20c0d4cfbdb2b303f87c5
Testing 84976e54167032f80607e64ae9700bd1978c6f30 == 31b0f5b321d7129fa9e20c0d4cfbdb2b303f87c5

  ▶ FAIL [expected PASS] /css-text-3_dev/html/css3-text-line-break-opclns-120.htm
  └   → /css-text-3_dev/html/css3-text-line-break-opclns-120.htm ac6b55a069326d64e8880848ed1e2ae9e5adfbaf
/css-text-3_dev/html/reference/css3-text-line-break-opclns-120-ref.htm d5b49d26c38c415327c7f36c113b4d90dfcc6819
Testing ac6b55a069326d64e8880848ed1e2ae9e5adfbaf == d5b49d26c38c415327c7f36c113b4d90dfcc6819

  ▶ FAIL [expected PASS] /css-text-3_dev/html/css3-text-line-break-opclns-119.htm
  └   → /css-text-3_dev/html/css3-text-line-break-opclns-119.htm 311c1c4db0a40b9cef8fbe3fe36637b470f89e16
/css-text-3_dev/html/reference/css3-text-line-break-opclns-119-ref.htm 090e6a27a1e539c8594bb61edeffa47631f2d873
Testing 311c1c4db0a40b9cef8fbe3fe36637b470f89e16 == 090e6a27a1e539c8594bb61edeffa47631f2d873

  ▶ FAIL [expected PASS] /css-text-3_dev/html/css3-text-line-break-opclns-122.htm
  └   → /css-text-3_dev/html/css3-text-line-break-opclns-122.htm 124022eba63504aaa05caf152f1f5965ede81d62
/css-text-3_dev/html/reference/css3-text-line-break-opclns-122-ref.htm 54fe158f0b787221168158b5a28a90b7016d3787
Testing 124022eba63504aaa05caf152f1f5965ede81d62 == 54fe158f0b787221168158b5a28a90b7016d3787

  ▶ FAIL [expected PASS] /css-text-3_dev/html/css3-text-line-break-opclns-125.htm
  └   → /css-text-3_dev/html/css3-text-line-break-opclns-125.htm d83b12118c4faa33027d5becd1f4fb21eaac6de6
/css-text-3_dev/html/reference/css3-text-line-break-opclns-125-ref.htm 3b9f729d4d2d15c9975dc1cac231a97771532672
Testing d83b12118c4faa33027d5becd1f4fb21eaac6de6 == 3b9f729d4d2d15c9975dc1cac231a97771532672

  ▶ FAIL [expected PASS] /css-text-3_dev/html/css3-text-line-break-opclns-124.htm
  └   → /css-text-3_dev/html/css3-text-line-break-opclns-124.htm ad75f2e379c27aed968c6a823532a636e74087ce
/css-text-3_dev/html/reference/css3-text-line-break-opclns-124-ref.htm f728041eb0e34743789d6e5c322d6bd4abf2d7f6
Testing ad75f2e379c27aed968c6a823532a636e74087ce == f728041eb0e34743789d6e5c322d6bd4abf2d7f6

  ▶ FAIL [expected PASS] /css-text-3_dev/html/css3-text-line-break-opclns-123.htm
  └   → /css-text-3_dev/html/css3-text-line-break-opclns-123.htm 941d61740e6a3b687717c5ee53ac55cd24afceba
/css-text-3_dev/html/reference/css3-text-line-break-opclns-123-ref.htm 1f5c344cc1d92d5f01d71a8c4d870bce707b697e
Testing 941d61740e6a3b687717c5ee53ac55cd24afceba == 1f5c344cc1d92d5f01d71a8c4d870bce707b697e

  ▶ FAIL [expected PASS] /css-text-3_dev/html/css3-text-line-break-opclns-128.htm
  └   → /css-text-3_dev/html/css3-text-line-break-opclns-128.htm dc685a0d62f1f095f471960f7aa4e64bbbec0132
/css-text-3_dev/html/reference/css3-text-line-break-opclns-128-ref.htm 0a3a629cc71356657b0c3fd8ffdb197e28323083
Testing dc685a0d62f1f095f471960f7aa4e64bbbec0132 == 0a3a629cc71356657b0c3fd8ffdb197e28323083

  ▶ FAIL [expected PASS] /css-text-3_dev/html/word-break-normal-002.htm
  └   → /css-text-3_dev/html/word-break-normal-002.htm c66c86cd5a673e2052e0130da76f1518fdf5d0c1
/css-text-3_dev/html/reference/word-break-normal-002-ref.htm 2cd9afc05d29f3991c889f9dc4999bd056455e7e
Testing c66c86cd5a673e2052e0130da76f1518fdf5d0c1 == 2cd9afc05d29f3991c889f9dc4999bd056455e7e

notriddle · 2016-11-03T16:27:45Z

Sorry, but the "opclns" tests and "word-break-normal" pass on real desktop installs but fail in CI. It's font-related, I think. Remove those test expectations, and try should pass.


        Updated test expectations

Permutatrix · 2016-11-03T19:53:39Z

Thanks, @notriddle.

notriddle · 2016-11-03T20:09:38Z

@bors-servo try

bors-servo · 2016-11-03T20:09:41Z

⌛ Trying commit 6321f70 with merge df68c13...


        Auto merge of #14035 - Permutatrix:iss-14030, r=<try>

Don't mark the first/last fragment of an {ib} split as FIRST/LAST_FRAGMENT_OF_ELEMENT.  This change allows inline margins, borders, and padding to interact correctly with {ib} splits. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #14030  - [X] 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/14035)

bors-servo · 2016-11-04T06:18:35Z

💥 Test timed out

emilio · 2016-11-04T17:26:48Z

@bors-servo retry

notriddle · 2016-11-04T17:29:03Z

@bors-servo clean try

bors-servo · 2016-11-04T17:29:07Z

⌛ Trying commit 6321f70 with merge dc8e468...


        Auto merge of #14035 - Permutatrix:iss-14030, r=<try>

Don't mark the first/last fragment of an {ib} split as FIRST/LAST_FRAGMENT_OF_ELEMENT.  This change allows inline margins, borders, and padding to interact correctly with {ib} splits. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #14030  - [X] 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/14035)

bors-servo · 2016-11-05T04:18:37Z

💥 Test timed out

notriddle · 2016-11-05T13:09:57Z

Last attempt

@bors-servo retry

bors-servo · 2016-11-05T16:49:18Z

⌛ Trying commit 6321f70 with merge 8ff2958...


        Auto merge of #14035 - Permutatrix:iss-14030, r=<try>

Don't mark the first/last fragment of an {ib} split as FIRST/LAST_FRAGMENT_OF_ELEMENT.  This change allows inline margins, borders, and padding to interact correctly with {ib} splits. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #14030  - [X] 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/14035)

SimonSapin · 2016-11-05T20:04:18Z

Trying to get homu unstuck.


        Auto merge of #14035 - Permutatrix:iss-14030, r=<try>

Don't mark the first/last fragment of an {ib} split as FIRST/LAST_FRAGMENT_OF_ELEMENT.  This change allows inline margins, borders, and padding to interact correctly with {ib} splits. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #14030  - [X] 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/14035)

bors-servo · 2016-11-05T23:49:34Z

☀️ 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

Permutatrix · 2016-11-06T00:12:52Z

Cool. So, are we doing things my way or @emilio's? Any more thoughts on that?

emilio · 2016-11-06T10:10:05Z

I don't really care that much as long as it's well documented, but probably @pcwalton or @notriddle have some opinions on it?

notriddle · 2016-11-06T14:38:14Z

No complaints here about doing it the @Permutatrix way.


        Elaborated on enclosing_node in comment, moved existing comments around

Permutatrix · 2016-11-07T07:36:14Z

Okay, I've added a comment about enclosing_node. Should @Ms2ger look at this now?

Ms2ger · 2016-11-07T12:05:32Z

Calling not it, I don't know anything about layout :)

r? @SimonSapin

emilio · 2016-11-09T20:53:28Z

@bors-servo r+

bors-servo · 2016-11-09T20:53:29Z

📌 Commit 0db5c55 has been approved by emilio

bors-servo · 2016-11-09T23:28:23Z

⌛ Testing commit 0db5c55 with merge d55a832...


        Auto merge of #14035 - Permutatrix:iss-14030, r=emilio

Don't mark the first/last fragment of an {ib} split as FIRST/LAST_FRAGMENT_OF_ELEMENT.  This change allows inline margins, borders, and padding to interact correctly with {ib} splits. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #14030 - [X] 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/14035)

bors-servo · 2016-11-10T00:13:04Z

💔 Test failed - linux-rel-wpt

jdm · 2016-11-10T00:20:00Z

@bors-servo: retry

#10473 #13360 #12797 #14157

bors-servo · 2016-11-10T01:08:53Z

⌛ Testing commit 0db5c55 with merge 959dfe4...


        Auto merge of #14035 - Permutatrix:iss-14030, r=emilio

Don't mark the first/last fragment of an {ib} split as FIRST/LAST_FRAGMENT_OF_ELEMENT.  This change allows inline margins, borders, and padding to interact correctly with {ib} splits. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #14030 - [X] 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/14035)

bors-servo · 2016-11-10T02:00:29Z

☀️ 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

Don't mark start/end of {ib} split as FIRST/LAST_FRAGMENT_OF_ELEMENT

Verified

This commit was signed with a verified signature.

Permutatrix Permutator

GPG key ID: 62D7A8D792C3B058 Learn about signing commits

fec3a49

highfive assigned Ms2ger Nov 3, 2016

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

emilio reviewed Nov 3, 2016

View changes

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

Updated test expectations

Verified

This commit was signed with a verified signature.

Permutatrix Permutator

GPG key ID: 62D7A8D792C3B058 Learn about signing commits

Loading status checks…

6321f70

Permutatrix force-pushed the Permutatrix:iss-14030 branch from 8279114 to 6321f70 Nov 3, 2016

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

Elaborated on enclosing_node in comment, moved existing comments around

Verified

This commit was signed with a verified signature.

Permutatrix Permutator

GPG key ID: 62D7A8D792C3B058 Learn about signing commits

Loading status checks…

0db5c55

Permutatrix force-pushed the Permutatrix:iss-14030 branch from f202bfa to 0db5c55 Nov 7, 2016

highfive assigned SimonSapin and unassigned Ms2ger Nov 7, 2016

highfive assigned emilio and unassigned SimonSapin Nov 9, 2016

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

highfive added S-tests-failed and removed S-awaiting-merge labels Nov 10, 2016

highfive added S-awaiting-merge and removed S-tests-failed labels Nov 10, 2016

bors-servo merged commit 0db5c55 into servo:master Nov 10, 2016
2 of 3 checks passed

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details

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

homu Test successful
Details

highfive removed the S-awaiting-merge label Nov 10, 2016

servo / servo

Don't mark the first/last fragment of an {ib} split as FIRST/LAST_FRAGMENT_OF_ELEMENT. #14035

Don't mark the first/last fragment of an {ib} split as FIRST/LAST_FRAGMENT_OF_ELEMENT. #14035

Permutatrix commented Nov 3, 2016 •

edited

highfive commented Nov 3, 2016

highfive commented Nov 3, 2016

emilio commented Nov 3, 2016

bors-servo commented Nov 3, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bors-servo commented Nov 3, 2016

highfive commented Nov 3, 2016

notriddle commented Nov 3, 2016 •

edited

Permutatrix commented Nov 3, 2016

notriddle commented Nov 3, 2016

bors-servo commented Nov 3, 2016

bors-servo commented Nov 4, 2016

emilio commented Nov 4, 2016

notriddle commented Nov 4, 2016

bors-servo commented Nov 4, 2016

bors-servo commented Nov 5, 2016

notriddle commented Nov 5, 2016 •

edited

bors-servo commented Nov 5, 2016

SimonSapin commented Nov 5, 2016

bors-servo commented Nov 5, 2016

Permutatrix commented Nov 6, 2016

emilio commented Nov 6, 2016

notriddle commented Nov 6, 2016

Permutatrix commented Nov 7, 2016

Ms2ger commented Nov 7, 2016

emilio commented Nov 9, 2016

bors-servo commented Nov 9, 2016

bors-servo commented Nov 9, 2016

bors-servo commented Nov 10, 2016

jdm commented Nov 10, 2016

bors-servo commented Nov 10, 2016

bors-servo commented Nov 10, 2016

servo / servo

Join GitHub today

Don't mark the first/last fragment of an {ib} split as FIRST/LAST_FRAGMENT_OF_ELEMENT. #14035

Don't mark the first/last fragment of an {ib} split as FIRST/LAST_FRAGMENT_OF_ELEMENT. #14035

Conversation

Permutatrix commented Nov 3, 2016 • edited

highfive commented Nov 3, 2016

highfive commented Nov 3, 2016

emilio commented Nov 3, 2016

bors-servo commented Nov 3, 2016

This comment has been minimized.

emilio Nov 3, 2016 • edited

This comment has been minimized.

Permutatrix Nov 3, 2016 • edited

This comment has been minimized.

Permutatrix Nov 3, 2016

This comment has been minimized.

pcwalton Nov 8, 2016

bors-servo commented Nov 3, 2016

highfive commented Nov 3, 2016

notriddle commented Nov 3, 2016 • edited

Permutatrix commented Nov 3, 2016

notriddle commented Nov 3, 2016

bors-servo commented Nov 3, 2016

bors-servo commented Nov 4, 2016

emilio commented Nov 4, 2016

notriddle commented Nov 4, 2016

bors-servo commented Nov 4, 2016

bors-servo commented Nov 5, 2016

notriddle commented Nov 5, 2016 • edited

bors-servo commented Nov 5, 2016

SimonSapin commented Nov 5, 2016

bors-servo commented Nov 5, 2016

Permutatrix commented Nov 6, 2016

emilio commented Nov 6, 2016

notriddle commented Nov 6, 2016

Permutatrix commented Nov 7, 2016

Ms2ger commented Nov 7, 2016

emilio commented Nov 9, 2016

bors-servo commented Nov 9, 2016

bors-servo commented Nov 9, 2016

bors-servo commented Nov 10, 2016

jdm commented Nov 10, 2016

bors-servo commented Nov 10, 2016

bors-servo commented Nov 10, 2016

Permutatrix commented Nov 3, 2016 •

edited

emilio Nov 3, 2016 •

edited

Permutatrix Nov 3, 2016 •

edited

notriddle commented Nov 3, 2016 •

edited

notriddle commented Nov 5, 2016 •

edited