Inline pseudo elements layout#12367 #12669

splav · 2016-07-31T12:03:19Z

This PR fixes ignored paddings and margins for inline pseudo elements.

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

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

This change is

splav · 2016-07-31T12:21:17Z

This is a first version, another part is missing - in the following case the green zone is calculated incorrectly and a line is broken:

<!DOCTYPE html>
<html><head><style type="text/css">
  span:before {
    content:"a";
    margin-right:5em;
  }
  span:after {
    content:"c";
    margin-left:5em;
  }
  span {
    margin-left:4em;
    margin-right:4em;
  }
</style></head>
<body><div style="position: absolute; top: 20px;"><span>b</span></div></body>
</html>

wafflespeanut · 2016-07-31T14:23:30Z

r? @mbrubeck or someone else

jdm · 2016-07-31T14:35:03Z

Want to review this, @notriddle?

notriddle · 2016-07-31T15:13:23Z

Yeah, that sounds like something I could help with.

KiChjang · 2016-07-31T18:55:37Z

@bors-servo delegate=notriddle

r? @notriddle

bors-servo · 2016-07-31T18:55:39Z

✌️ @notriddle can now approve this pull request

splav · 2016-07-31T19:56:32Z

@notriddle may be change to something like this:

<style type="text/css">
  span:before { content:"a"; margin-right:5em; }
  span:after { content:"c"; margin-left:5em; }
  #first { margin-left:4em; margin-right:4em; }
  #second:before { margin-left:4em; }
  #second:after { margin-right:4em; }
</style>
<body>
    |<span id=first>b</span>|<br>
    |<span id=second>b</span>|
</body>

and

<style type="text/css">
  span:before { content:"a"; padding-right:5em; }
  span:after { content:"c"; padding-left:5em; }
  #first { padding-left:4em; padding-right:4em; }
  #second:before { padding-left:4em; }
  #second:after { padding-right:4em; }
</style>

<body>
    |<span id=first>b</span>|<br>
    |<span id=second>b</span>|
</body>

Could you please comment if the main idea of the fix is correct? I'm not very sure that both style-fixing functions can be just skipped for pseudo elements. I've read the specs and couldn't imagine the appropriate cases, but I didn't have much experience with CSS and could miss something.

notriddle · 2016-07-31T23:16:37Z

The modified version of the test looks good.

The big problem that the modify function tries to solve is that the style of the base element is copied verbatim to the pseudo-element. I think that was actually fixed in the style code awhile back. Let's try it.

@bors-servo try

bors-servo · 2016-07-31T23:16:41Z

⌛ Trying commit 8c8f5dd with merge 718cbfd...

bors-servo · 2016-07-31T23:59:12Z

💔 Test failed - mac-rel-css

highfive · 2016-07-31T23:59:14Z

  ▶ FAIL [expected PASS] /css21_dev/html4/insert-inline-in-blocks-n-inlines-begin-003.htm
  └   → /css21_dev/html4/insert-inline-in-blocks-n-inlines-begin-003.htm daa5c6726760140eb39cfd0ef842bc0a337bd0d6
/css21_dev/html4/reference/insert-inline-in-blocks-n-inlines-begin-003-ref.htm 11aaba585e29d82ac7512d6f646fa3a6962167f8
Testing daa5c6726760140eb39cfd0ef842bc0a337bd0d6 == 11aaba585e29d82ac7512d6f646fa3a6962167f8

  ▶ FAIL [expected PASS] /css21_dev/html4/insert-inline-in-blocks-n-inlines-middle-003.htm
  └   → /css21_dev/html4/insert-inline-in-blocks-n-inlines-middle-003.htm 84814dc899e920095373d52c0a0cca063929bce7
/css21_dev/html4/reference/insert-inline-in-blocks-n-inlines-middle-003-ref.htm 021252e76c13ae9d890</span><span class="stdout">677f08d3b957f46be4c3f
Testing 84814dc899e920095373d52c0a0cca063929bce7 == 021252e76c13ae9d890677f08d3b957f46be4c3f

  ▶ Unexpected subtest result in /css-transitions-1_dev/html/pseudo-elements-001.htm:
  └ PASS [expected FAIL] transition padding-left on :before, changing content / values

  ▶ Unexpected subtest result in /css-transitions-1_dev/html/pseudo-elements-001.htm:
  └ PASS [expected FAIL] transition padding-left on :after, changing content / values

  ▶ Unexpected subtest result in /css21_dev/html4/pseudo-elements-001.htm:
  └ PASS [expected FAIL] transition padding-left on :before, changing content / values

  ▶ Unexpected subtest result in /css21_dev/html4/pseudo-elements-001.htm:
  └ PASS [expected FAIL] transition padding-left on :after, changing content / values

notriddle · 2016-08-01T00:35:17Z

The passes look good, since they're part of your PR. I'm not sure about the failures, because there aren't any pseudo elements in either of those tests to trigger your code changes.

notriddle · 2016-08-01T00:57:49Z

Reverting c1243fe, the one that prevents text fragments from being merged, fixes the test failures. Reverting f787f3a, the one that stops pseudo-element styles from being reset, does not. (I ran the failing tests on my box to check them.)

So turning off the style reset didn't cause any tests to fail, and it makes sense that we need to store the style of the pseudo-elements somewhere, since they're not going to get their own flows, so let's put it in the fragments.

splav · 2016-08-01T05:02:41Z

@notriddle OK, thank you. I'll investigate what's going on in failed tests. And also in the case with absolutely positioned div, I've posted before.

notriddle · 2016-08-09T18:11:07Z

@bors-servo retry

Let's see how #12760 affected the test results.

bors-servo · 2016-08-09T18:11:23Z

⌛ Trying commit 8c8f5dd with merge 68096b8...


        Auto merge of #12669 - splav:inline_pseudo_elements_layout#12367, r=<…

…try> Inline pseudo elements layout#12367  This PR fixes ignored paddings and margins for inline pseudo elements. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #12367 (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/12669)

bors-servo · 2016-08-09T18:59:57Z

💔 Test failed - mac-rel-css

bors-servo · 2016-08-10T14:02:21Z

⌛ Testing commit d3ccbc5 with merge 4ca4c7c...


        Auto merge of #12669 - splav:inline_pseudo_elements_layout#12367, r=n…

…otriddle Inline pseudo elements layout#12367  This PR fixes ignored paddings and margins for inline pseudo elements. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #12367 (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/12669)

bors-servo · 2016-08-10T14:49:55Z

💔 Test failed - linux-rel

highfive · 2016-08-10T14:49:58Z

  ▶ FAIL [expected PASS] /css-transforms-1_dev/html/transform-table-007.htm
  └   → /css-transforms-1_dev/html/transform-table-007.htm a5c014b20ef1363bea6f24eda28c7efb7c45698a
/css-transforms-1_dev/html/reference/transform-blank-ref.htm fa6407b1acbbfea27e27061e7d1bdeca98e4a728
Testing a5c014b20ef1363bea6f24eda28c7efb7c45698a == fa6407b1acbbfea27e27061e7d1bdeca98e4a728

bors-servo · 2016-08-11T03:02:13Z

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


        do not reset pseudo elements style


        do not merge inline fragments if there is space in between


        add tests to check if inline pseudo elements paddings and margins are…

… accounted correctly


        update tests expectations

splav · 2016-08-11T07:35:38Z

@jdm rebased to current master.

notriddle · 2016-08-11T07:46:04Z

@bors-servo r+

bors-servo · 2016-08-11T07:46:05Z

📌 Commit a9c1817 has been approved by notriddle

bors-servo · 2016-08-11T08:38:14Z

⌛ Testing commit a9c1817 with merge 9ffda4c...


        Auto merge of #12669 - splav:inline_pseudo_elements_layout#12367, r=n…

…otriddle Inline pseudo elements layout#12367  This PR fixes ignored paddings and margins for inline pseudo elements. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #12367 (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/12669)

bors-servo · 2016-08-11T09:36:21Z

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

highfive assigned wafflespeanut Jul 31, 2016

highfive added the S-awaiting-review label Jul 31, 2016

highfive assigned mbrubeck and unassigned wafflespeanut Jul 31, 2016

highfive assigned notriddle and unassigned mbrubeck Jul 31, 2016

highfive added the S-tests-failed label Jul 31, 2016

splav mentioned this pull request Aug 2, 2016

Borders of inline elements are wrong for broken lines #12699

Closed

highfive added the S-tests-failed label Aug 9, 2016

splav force-pushed the splav:inline_pseudo_elements_layout#12367 branch from 8c8f5dd to d3ccbc5 Aug 9, 2016

highfive removed the S-tests-failed label Aug 9, 2016

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

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

jdm mentioned this pull request Aug 10, 2016

Intermittent failure in /css-transforms-1_dev/html/transform-table-007.htm #11574

Closed

highfive added the S-needs-rebase label Aug 11, 2016

splav added 4 commits Jul 31, 2016

do not reset pseudo elements style

69769e4

do not merge inline fragments if there is space in between

49b50e4

add tests to check if inline pseudo elements paddings and margins are…

93334cf

… accounted correctly

update tests expectations

Loading status checks…

a9c1817

splav force-pushed the splav:inline_pseudo_elements_layout#12367 branch from d3ccbc5 to a9c1817 Aug 11, 2016

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

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-rebase labels Aug 11, 2016

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

splav deleted the splav:inline_pseudo_elements_layout#12367 branch Aug 11, 2016

servo / servo

Inline pseudo elements layout#12367 #12669

Inline pseudo elements layout#12367 #12669

splav commented Jul 31, 2016 •

edited by jdm

splav commented Jul 31, 2016

wafflespeanut commented Jul 31, 2016

jdm commented Jul 31, 2016

notriddle commented Jul 31, 2016

KiChjang commented Jul 31, 2016

bors-servo commented Jul 31, 2016

splav commented Jul 31, 2016 •

edited

notriddle commented Jul 31, 2016

bors-servo commented Jul 31, 2016

bors-servo commented Jul 31, 2016

highfive commented Jul 31, 2016

notriddle commented Aug 1, 2016

notriddle commented Aug 1, 2016 •

edited

splav commented Aug 1, 2016

notriddle commented Aug 9, 2016 •

edited

bors-servo commented Aug 9, 2016

bors-servo commented Aug 9, 2016

bors-servo commented Aug 10, 2016

bors-servo commented Aug 10, 2016

highfive commented Aug 10, 2016

bors-servo commented Aug 11, 2016

splav commented Aug 11, 2016 •

edited

notriddle commented Aug 11, 2016

bors-servo commented Aug 11, 2016

bors-servo commented Aug 11, 2016

bors-servo commented Aug 11, 2016

servo / servo

Join GitHub today

Inline pseudo elements layout#12367 #12669

Inline pseudo elements layout#12367 #12669

Conversation

splav commented Jul 31, 2016 • edited by jdm

splav commented Jul 31, 2016

wafflespeanut commented Jul 31, 2016

jdm commented Jul 31, 2016

notriddle commented Jul 31, 2016

KiChjang commented Jul 31, 2016

bors-servo commented Jul 31, 2016

splav commented Jul 31, 2016 • edited

notriddle commented Jul 31, 2016

bors-servo commented Jul 31, 2016

bors-servo commented Jul 31, 2016

highfive commented Jul 31, 2016

notriddle commented Aug 1, 2016

notriddle commented Aug 1, 2016 • edited

splav commented Aug 1, 2016

notriddle commented Aug 9, 2016 • edited

bors-servo commented Aug 9, 2016

bors-servo commented Aug 9, 2016

bors-servo commented Aug 10, 2016

bors-servo commented Aug 10, 2016

highfive commented Aug 10, 2016

bors-servo commented Aug 11, 2016

splav commented Aug 11, 2016 • edited

notriddle commented Aug 11, 2016

bors-servo commented Aug 11, 2016

bors-servo commented Aug 11, 2016

bors-servo commented Aug 11, 2016

splav commented Jul 31, 2016 •

edited by jdm

splav commented Jul 31, 2016 •

edited

notriddle commented Aug 1, 2016 •

edited

notriddle commented Aug 9, 2016 •

edited

splav commented Aug 11, 2016 •

edited