Fix handling of borders and padding for empty/stripped inline flows #10643

mbrubeck · 2016-04-16T00:02:44Z

This forces fragment generation for empty inline flows, if they have borders or padding. Fixes #10533 and #2001. Also includes fixes for other bugs that were uncovered by this change; see the individual commit messages for detailed explanations. r? @pcwalton

This change is

pcwalton · 2016-04-16T00:28:22Z

components/layout/fragment.rs

                }
            }
        }
    }

+    pub fn meld_with_prev_inline_fragment(&mut self, prev_fragment: &Fragment) {
+        if let Some(ref mut inline_context_of_this_fragment) = self.inline_context {
+            if let Some(ref inline_context_of_prev_fragment) = prev_fragment.inline_context {


nit: You could remove a level of indentation here by matching on a tuple, if you like. Up to you.

Tried out this suggestion, but ended up making the code less readable because of explicit reborrows needed to avoid moving from borrowed content into the tuple.

pcwalton · 2016-04-16T00:48:01Z

components/layout/construct.rs

+        padding.padding_top != LengthOrPercentage::Length(Au(0)) ||
+           padding.padding_right != LengthOrPercentage::Length(Au(0)) ||
+           padding.padding_bottom != LengthOrPercentage::Length(Au(0)) ||
+           padding.padding_left != LengthOrPercentage::Length(Au(0)) ||


Handle 0% maybe too?

If that's going to be a thing we do, we should probably add an is_zero() method to LengthOrPercentage or something.


        Improve debug logging of non-printable chars


        Fix meld_with_next_inline_fragment and add meld_with_prev_inline_frag…

@notriddle

…ment Factor out a new `meld_with_prev_inline_fragment` method that mirrors the existing `meld_with_next_inline_fragment`. This also fixes a bug in `meld_with_next` that was already fixed in the `meld_with_prev` by @notriddle in #10419. The bug is that it was traversing the inline context nodes in the wrong order. It should start at the outermost enclosing node, since the fragments might be at different nesting levels under some common ancestor.


        Fix up inline context flags during text fragment scanning

This fixes two problems that could cause scanned text fragments to end up with incorrect LAST_FRAGMENT_OF_ELEMENT or FIRST_FRAGMENT_OF_ELEMENT flags: 1. If a single unscanned fragment was split into multiple scanned fragments, then all of them would inherit its flags. We need to clear these flags, except for the first and last scanned fragment. 2. When an unscanned fragment generated zero scanned fragments, we correctly called `meld_with_next_inline_fragment` to transfer LAST_FRAGMENT flags to the preceding fragment, but we didn't do anything to transfer FIRST_FRAGMENT flags to the following fragment. We can fix this by calling `meld_with_prev_inline_fragment` on the following fragment.

mbrubeck · 2016-04-16T17:42:01Z

Addressed review comments.


        Generate a fragment for an empty elements with borders or padding

pcwalton · 2016-04-16T18:05:56Z

@bors-servo: r+

bors-servo · 2016-04-16T18:05:59Z

📌 Commit 6431d74 has been approved by pcwalton

bors-servo · 2016-04-16T18:23:51Z

⌛ Testing commit 6431d74 with merge b7c4404...


        Auto merge of #10643 - mbrubeck:empty-span-border, r=pcwalton

@pcwalton

Fix handling of borders and padding for empty/stripped inline flows This forces fragment generation for empty inline flows, if they have borders or padding. Fixes #10533 and #2001. Also includes fixes for other bugs that were uncovered by this change; see the individual commit messages for detailed explanations. r? @pcwalton  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10643)

bors-servo · 2016-04-16T19:04:22Z

💔 Test failed - linux-rel

jdm · 2016-04-16T19:23:22Z

  ▶ FAIL [expected PASS] /css21_dev/html4/run-in-listitem-between-002.htm
  └   → /css21_dev/html4/run-in-listitem-between-002.htm ef608dda75ea29fbec3a0a32f64cc80292e62d82
/css21_dev/html4/reference/run-in-text-ref.htm d068883ccfce1b3fdf24f9ea6b401c9a047b593c
Testing ef608dda75ea29fbec3a0a32f64cc80292e62d82 == d068883ccfce1b3fdf24f9ea6b401c9a047b593c

  ▶ FAIL [expected PASS] /css21_dev/html4/run-in-listitem-between-001.htm
  └   → /css21_dev/html4/run-in-listitem-between-001.htm ef608dda75ea29fbec3a0a32f64cc80292e62d82
/css21_dev/html4/reference/run-in-text-ref.htm d068883ccfce1b3fdf24f9ea6b401c9a047b593c
Testing ef608dda75ea29fbec3a0a32f64cc80292e62d82 == d068883ccfce1b3fdf24f9ea6b401c9a047b593c


        Don't strip out all empty fragments

Empty fragments may need to be layed out to draw borders, padding/background, and insertion points. (Fragments that consist of discardable whitespace and control characters, on the other hand, can still be discarded.) This ends up preserving some useless empty fragments. It's possible we could avoid this by storing some sort of flag on "important" empty fragments, so we can discard the rest.

mbrubeck · 2016-04-16T20:14:13Z

@bors-servo r=pcwalton

Reverted some of the metadata changes.

bors-servo · 2016-04-16T20:14:14Z

📌 Commit 1695d14 has been approved by pcwalton

bors-servo · 2016-04-16T20:39:33Z

⌛ Testing commit 1695d14 with merge 7cb0391...


        Auto merge of #10643 - mbrubeck:empty-span-border, r=pcwalton

@pcwalton

Fix handling of borders and padding for empty/stripped inline flows This forces fragment generation for empty inline flows, if they have borders or padding. Fixes #10533 and #2001. Also includes fixes for other bugs that were uncovered by this change; see the individual commit messages for detailed explanations. r? @pcwalton  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10643)

bors-servo · 2016-04-16T21:20:18Z

💔 Test failed - mac-rel-wpt

mbrubeck · 2016-04-16T21:32:21Z

I don't think this is related to my changes:

Tests with unexpected results:
  ▶ CRASH [expected OK] /webgl/conformance-1.0.3/conformance/limits/gl-min-attribs.html

  ▶ OK [expected CRASH] /webgl/conformance-1.0.3/conformance/more/conformance/quickCheckAPI-G_I.html

  ▶ Unexpected subtest result in /webgl/conformance-1.0.3/conformance/more/conformance/quickCheckAPI-G_I.html:
  │ FAIL [expected PASS] WebGL test #0: testValidArgs

  ▶ CRASH [expected OK] /webgl/conformance-1.0.3/conformance/textures/texture-draw-with-2d-and-cube.html

mbrubeck · 2016-04-16T21:33:00Z

@bors-servo retry

will file new issue if this turns out to be intermittent

bors-servo · 2016-04-16T22:01:13Z

⌛ Testing commit 1695d14 with merge 15e76eb...


        Auto merge of #10643 - mbrubeck:empty-span-border, r=pcwalton

@pcwalton

Fix handling of borders and padding for empty/stripped inline flows This forces fragment generation for empty inline flows, if they have borders or padding. Fixes #10533 and #2001. Also includes fixes for other bugs that were uncovered by this change; see the individual commit messages for detailed explanations. r? @pcwalton  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10643)

jdm · 2016-04-16T22:09:24Z

Yeah, those are the temperamental new WebGL tests that were just enabled after a long fight in #10373.

bors-servo · 2016-04-16T22:43:39Z

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

mbrubeck added I-wrong A-layout/inline labels Apr 16, 2016

highfive assigned pcwalton Apr 16, 2016

highfive added the S-awaiting-review label Apr 16, 2016

mbrubeck force-pushed the mbrubeck:empty-span-border branch from cfdccf5 to 82201b7 Apr 16, 2016

pcwalton reviewed Apr 16, 2016
View changes

pcwalton added S-needs-code-changes and removed S-awaiting-review labels Apr 16, 2016

mbrubeck added 3 commits Apr 15, 2016

Improve debug logging of non-printable chars

587314e

mbrubeck force-pushed the mbrubeck:empty-span-border branch from 82201b7 to ec9afe9 Apr 16, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Apr 16, 2016

Generate a fragment for an empty elements with borders or padding

1807fd3

mbrubeck force-pushed the mbrubeck:empty-span-border branch from ec9afe9 to 6431d74 Apr 16, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Apr 16, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Apr 16, 2016

mbrubeck force-pushed the mbrubeck:empty-span-border branch from 6431d74 to 1695d14 Apr 16, 2016

highfive added S-awaiting-review and removed S-tests-failed labels Apr 16, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Apr 16, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Apr 16, 2016

highfive added S-awaiting-merge and removed S-tests-failed labels Apr 16, 2016

bors-servo merged commit 1695d14 into servo:master Apr 16, 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 Apr 16, 2016

notriddle mentioned this pull request Apr 16, 2016

Create boxes for ignorable whitespace if there are padding or borders #2001

Closed

mbrubeck deleted the mbrubeck:empty-span-border branch May 17, 2016

emilio mentioned this pull request Aug 17, 2016

Intermittent failures in /css21_dev/html4/run-in-listitem-between-001.htm /css21_dev/html4/run-in-listitem-between-002.htm #12901

Closed

servo / servo

Fix handling of borders and padding for empty/stripped inline flows #10643

Fix handling of borders and padding for empty/stripped inline flows #10643

mbrubeck commented Apr 16, 2016

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.

mbrubeck commented Apr 16, 2016

pcwalton commented Apr 16, 2016

bors-servo commented Apr 16, 2016

bors-servo commented Apr 16, 2016

bors-servo commented Apr 16, 2016

jdm commented Apr 16, 2016

mbrubeck commented Apr 16, 2016

bors-servo commented Apr 16, 2016

bors-servo commented Apr 16, 2016

bors-servo commented Apr 16, 2016

mbrubeck commented Apr 16, 2016

mbrubeck commented Apr 16, 2016

bors-servo commented Apr 16, 2016

jdm commented Apr 16, 2016

bors-servo commented Apr 16, 2016

servo / servo

Join GitHub today

Fix handling of borders and padding for empty/stripped inline flows #10643

Fix handling of borders and padding for empty/stripped inline flows #10643

Conversation

mbrubeck commented Apr 16, 2016

This comment has been minimized.

pcwalton Apr 16, 2016

This comment has been minimized.

mbrubeck Apr 16, 2016

This comment has been minimized.

pcwalton Apr 16, 2016

This comment has been minimized.

notriddle Apr 16, 2016

This comment has been minimized.

mbrubeck Apr 16, 2016

mbrubeck commented Apr 16, 2016

pcwalton commented Apr 16, 2016

bors-servo commented Apr 16, 2016

bors-servo commented Apr 16, 2016

bors-servo commented Apr 16, 2016

jdm commented Apr 16, 2016

mbrubeck commented Apr 16, 2016

bors-servo commented Apr 16, 2016

bors-servo commented Apr 16, 2016

bors-servo commented Apr 16, 2016

mbrubeck commented Apr 16, 2016

mbrubeck commented Apr 16, 2016

bors-servo commented Apr 16, 2016

jdm commented Apr 16, 2016

bors-servo commented Apr 16, 2016