fix block size for absolute replaced element #19184

bobthekingofegypt · 2017-11-11T19:36:32Z

Absolutely replaced elements with padding were incorrectly setting their
block size to include twice the padding values. This attempts to stop
that extra padding for replaced elements but leave the working flow for
non replaced elements

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

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

This change is

highfive · 2017-11-11T19:36:37Z

Heads up! This PR modifies the following files:

@emilio: components/layout/block.rs

highfive · 2017-11-11T19:36:39Z

Warning

These commits include an empty title element (<title></title>). Consider adding appropriate metadata.

KiChjang · 2017-11-11T19:46:19Z

@bors-servo try

bors-servo · 2017-11-11T19:46:24Z

⌛ Trying commit 7009750 with merge 7acdfdb...


        Auto merge of #19184 - bobthekingofegypt:fix/18706-abs-image-with-pad…

…ding, r=<try> fix block size for absolute replaced element Absolutely replaced elements with padding were incorrectly setting their block size to include twice the padding values. This attempts to stop that extra padding for replaced elements but leave the working flow for non replaced elements  ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #18706 (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/19184)

bors-servo · 2017-11-11T20:03:56Z

💔 Test failed - mac-dev-unit

jdm · 2017-11-11T20:28:11Z

This apparently makes an existing test pass, too!

  ▶ Unexpected subtest result in /css/cssom-view/elementsFromPoint-iframes.html:
  └ PASS [expected FAIL] elementsFromPoint on the root document for points in iframe elements

bobthekingofegypt · 2017-11-11T21:08:44Z

Is that why mac-dev-unit failed, I didn't understand the report?

I believe it's fixed that test because that test has an absolutely positioned replaced element (iframe) which I assume has a default border like firefox of 2px which was being added to the size of the element. I can check for sure tomorrow.

jdm · 2017-11-12T00:18:01Z

mac-dev-unit failed because the manifest needs to be updated whenever test content is changed. You need to run ./mach manifest-update to do that.

emilio · 2017-11-12T04:05:07Z

components/layout/block.rs

@@ -1300,7 +1300,12 @@ impl BlockFlow {
            self.base.position.start.b = solution.block_start + self.fragment.margin.block_start
        }

-        let block_size = solution.block_size + self.fragment.border_padding.block_start_end();
+        let block_size = if self.fragment.is_replaced() {


Seems like it'd be cleaner to not include the padding in the block size in the first time? Looks like this is done in assign_replaced_inline_size_if_necessary (though it probably should be done in assign_replaced_block_size_if_necessary?).

I agree at the moment it looks more like fixing the ~~cause rather than the effect~~effect rather than the cause.

I'm not sure if I understand though, from those functions I'm guessing you mean change the block size set in border_box (the value that is passed to solve_vertical_constraints_abs_replaced as the block size). I was afraid to mess with the value of border_box as its doc string said it should contain the padding and border.

The position of this fragment relative to its owning flow. The size includes padding and border, but not margin.

I tried to find a way to pass the something other than border_box value on https://github.com/bobthekingofegypt/servo/blob/70097503396aab585cc565aaeaa693cce4757800/components/layout/block.rs#L1262 but I don't have enough knowledge of layout to figure out one. I could adjust the value directly on line 1262 I guess but it's a similar confusion of just removing padding and borders to re-add them a few lines later.

Err, you're totally right, I guess we should substract the padding in solve_vertical_constraints_abs_replaced? @mbrubeck, could you take a look? You reviewed those functions when they were added, and I should also read a bit to see what's the right thing here :)

Ugh, this code is all a bit of a mess, isn't it? Umm, maybe this is a better place to subtract the padding:

servo/components/layout/block.rs

Line 1262 in 5c7e79e

let block_size_used_val = self.fragment.border_box.size.block;

(See also the TODO comments above this line which hint at some of the bigger issues here.)

emilio · 2017-11-12T04:05:07Z

components/layout/block.rs

+        let block_size = if self.fragment.is_replaced() {
+            solution.block_size
+        } else {
+            (solution.block_size + self.fragment.border_padding.block_start_end())


nit: No need for parenthesis.


        fix block size for absolute replaced element

Absolutely replaced elements with padding were incorrectly setting their block size to include twice the padding values. This attempts to stop that extra padding for replaced elements but leave the working flow for non replaced elements


        add title to absolute div test case


        move padding adjustments to inside is_replaced condition


        update manifest


        fix linelength warning

jdm · 2018-05-25T18:01:38Z

@mbrubeck Are you able to review these changes?

mbrubeck · 2018-05-25T18:04:42Z

@bors-servo r+

bors-servo · 2018-05-25T18:04:43Z

📌 Commit d723712 has been approved by mbrubeck

bors-servo · 2018-05-25T18:04:49Z

⌛ Testing commit d723712 with merge 2e20753...


        Auto merge of #19184 - bobthekingofegypt:fix/18706-abs-image-with-pad…

…ding, r=mbrubeck fix block size for absolute replaced element Absolutely replaced elements with padding were incorrectly setting their block size to include twice the padding values. This attempts to stop that extra padding for replaced elements but leave the working flow for non replaced elements  ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #18706 (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/19184)

bors-servo · 2018-05-25T18:38:36Z

💔 Test failed - mac-rel-wpt3

jdm · 2018-05-25T19:40:16Z

@bors-servo retry

#20796

bors-servo · 2018-05-25T19:40:19Z

⚡ Previous build results for android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt4, windows-msvc-dev are reusable. Rebuilding only mac-dev-unit, mac-rel-css1, mac-rel-wpt3...

bors-servo · 2018-05-26T06:06:25Z

💥 Test timed out

bors-servo · 2018-06-30T08:39:56Z

⌛ Testing commit d723712 with merge 82e2450...


        Auto merge of #19184 - bobthekingofegypt:fix/18706-abs-image-with-pad…

…ding, r=mbrubeck fix block size for absolute replaced element Absolutely replaced elements with padding were incorrectly setting their block size to include twice the padding values. This attempts to stop that extra padding for replaced elements but leave the working flow for non replaced elements  ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #18706 (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/19184)

bors-servo · 2018-06-30T08:40:02Z

💔 Test failed - android

edunham · 2018-07-14T00:18:24Z

@bors-servo retry

bors-servo · 2018-07-14T00:18:32Z

⌛ Testing commit d723712 with merge 3961e8d...


        Auto merge of #19184 - bobthekingofegypt:fix/18706-abs-image-with-pad…

…ding, r=mbrubeck fix block size for absolute replaced element Absolutely replaced elements with padding were incorrectly setting their block size to include twice the padding values. This attempts to stop that extra padding for replaced elements but leave the working flow for non replaced elements  ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #18706 (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/19184)

bors-servo · 2018-07-14T01:22:23Z

☀️ Test successful - android, android-x86, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: mbrubeck
Pushing 3961e8d to master...

highfive assigned metajack Nov 11, 2017

highfive added the S-awaiting-review label Nov 11, 2017

highfive added the S-tests-failed label Nov 11, 2017

emilio reviewed Nov 12, 2017

View changes

bobthekingofegypt added 4 commits Nov 11, 2017

fix block size for absolute replaced element

21e2ed5

Absolutely replaced elements with padding were incorrectly setting their block size to include twice the padding values. This attempts to stop that extra padding for replaced elements but leave the working flow for non replaced elements

add title to absolute div test case

e926bef

move padding adjustments to inside is_replaced condition

0eb8c06

update manifest

Loading status checks…

20cf171

bobthekingofegypt force-pushed the bobthekingofegypt:fix/18706-abs-image-with-padding branch from 7009750 to 20cf171 Nov 22, 2017

highfive removed the S-tests-failed label Nov 22, 2017

fix linelength warning

Loading status checks…

d723712

jdm assigned emilio and unassigned metajack Mar 6, 2018

highfive assigned mbrubeck and unassigned emilio May 25, 2018

highfive added S-awaiting-merge and removed S-awaiting-review labels May 25, 2018

highfive added S-tests-failed and removed S-awaiting-merge labels May 25, 2018

highfive added S-awaiting-merge and removed S-tests-failed labels Jun 30, 2018

highfive added S-tests-failed and removed S-awaiting-merge labels Jun 30, 2018

highfive added S-awaiting-merge and removed S-tests-failed labels Jul 14, 2018

bors-servo merged commit d723712 into servo:master Jul 14, 2018
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 Jul 14, 2018

servo / servo

fix block size for absolute replaced element #19184

fix block size for absolute replaced element #19184

bobthekingofegypt commented Nov 11, 2017 •

edited by SimonSapin

highfive commented Nov 11, 2017

highfive commented Nov 11, 2017

KiChjang commented Nov 11, 2017

bors-servo commented Nov 11, 2017

bors-servo commented Nov 11, 2017

jdm commented Nov 11, 2017

bobthekingofegypt commented Nov 11, 2017

jdm commented Nov 12, 2017

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.

jdm commented May 25, 2018

mbrubeck commented May 25, 2018

bors-servo commented May 25, 2018

bors-servo commented May 25, 2018

bors-servo commented May 25, 2018

jdm commented May 25, 2018

bors-servo commented May 25, 2018

bors-servo commented May 26, 2018

bors-servo commented Jun 30, 2018

bors-servo commented Jun 30, 2018

edunham commented Jul 14, 2018

bors-servo commented Jul 14, 2018

bors-servo commented Jul 14, 2018

servo / servo

Join GitHub today

fix block size for absolute replaced element #19184

fix block size for absolute replaced element #19184

Conversation

bobthekingofegypt commented Nov 11, 2017 • edited by SimonSapin

highfive commented Nov 11, 2017

highfive commented Nov 11, 2017

KiChjang commented Nov 11, 2017

bors-servo commented Nov 11, 2017

bors-servo commented Nov 11, 2017

jdm commented Nov 11, 2017

bobthekingofegypt commented Nov 11, 2017

jdm commented Nov 12, 2017

This comment has been minimized.

emilio Nov 12, 2017

This comment has been minimized.

bobthekingofegypt Nov 12, 2017 • edited

This comment has been minimized.

emilio Nov 12, 2017

This comment has been minimized.

mbrubeck Nov 16, 2017

This comment has been minimized.

emilio Nov 12, 2017

jdm commented May 25, 2018

mbrubeck commented May 25, 2018

bors-servo commented May 25, 2018

bors-servo commented May 25, 2018

bors-servo commented May 25, 2018

jdm commented May 25, 2018

bors-servo commented May 25, 2018

bors-servo commented May 26, 2018

bors-servo commented Jun 30, 2018

bors-servo commented Jun 30, 2018

edunham commented Jul 14, 2018

bors-servo commented Jul 14, 2018

bors-servo commented Jul 14, 2018

bobthekingofegypt commented Nov 11, 2017 •

edited by SimonSapin

bobthekingofegypt Nov 12, 2017 •

edited