Skip to content

/ servo

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix block size for absolute replaced element #19184

Merged
merged 5 commits into from Jul 14, 2018
+118 −1
Merged

fix block size for absolute replaced element #19184

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
21e2ed5
fix block size for absolute replaced element
bobthekingofegypt Nov 11, 2017
e926bef
add title to absolute div test case
bobthekingofegypt Nov 20, 2017
0eb8c06
move padding adjustments to inside is_replaced condition
bobthekingofegypt Nov 22, 2017
20cf171
update manifest
bobthekingofegypt Nov 22, 2017
d723712
fix linelength warning
bobthekingofegypt Nov 22, 2017
File filter...
Filter file types
Jump to…
Jump to file
Failed to load files.

Always

Just for now

Next

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
  • Loading branch information
@bobthekingofegypt
bobthekingofegypt committed Nov 22, 2017
commit 21e2ed522bb75591e8ebb944c9bf0f06ebb619c6
7 components/layout/block.rs
View file
Open in desktop
@@ -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() {

This comment has been minimized.

Sign in to view
Copy link
@emilio

emilio Nov 12, 2017

Member

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?).

This comment has been minimized.

Sign in to view
Copy link
@bobthekingofegypt

bobthekingofegypt Nov 12, 2017 •
edited

Author Contributor

I agree at the moment it looks more like fixing the cause rather than the effecteffect 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.

This comment has been minimized.

Sign in to view
Copy link
@emilio

emilio Nov 12, 2017

Member

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 :)

This comment has been minimized.

Sign in to view
Copy link
@mbrubeck

mbrubeck Nov 16, 2017

Contributor

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.)
solution.block_size
} else {
(solution.block_size + self.fragment.border_padding.block_start_end())

This comment has been minimized.

Sign in to view
Copy link
@emilio

emilio Nov 12, 2017

Member

nit: No need for parenthesis.
};

self.fragment.border_box.size.block = block_size;
self.base.position.size.block = block_size;

41 tests/wpt/mozilla/meta/MANIFEST.json
View file
Open in desktop
@@ -73,6 +73,18 @@
{}
]
],
"css/absolute_div_with_padding.html": [
[
"/_mozilla/css/absolute_div_with_padding.html",
[
[
"/_mozilla/css/absolute_div_with_padding_ref.html",
"=="
]
],
{}
]
],
"css/absolute_hypothetical_float.html": [
[
"/_mozilla/css/absolute_hypothetical_float.html",
@@ -109,6 +121,18 @@
{}
]
],
"css/absolute_img_with_padding.html": [
[
"/_mozilla/css/absolute_img_with_padding.html",
[
[
"/_mozilla/css/absolute_img_with_padding_ref.html",
"=="
]
],
{}
]
],
"css/absolute_inline_containing_block_a.html": [
[
"/_mozilla/css/absolute_inline_containing_block_a.html",
@@ -7906,6 +7930,11 @@
{}
]
],
"css/absolute_img_with_padding_ref.html": [
[
{}
]
],
"css/absolute_inline_containing_block_ref.html": [
[
{}
"e38f77f3a7691b11abeece839aba62ce9246ff6a",
"support"
],
"css/absolute_div_with_padding.html": [
"6bfd1ae130e22c3083070a3574f827ded86f6724",
"reftest"
],
"css/absolute_hypothetical_float.html": [
"1abe08648b55df2390cb2b6561923aa576212fbf",
"reftest"
"400117a1d118ee05db150877aa81bb414f4e7200",
"support"
],
"css/absolute_img_with_padding.html": [
"15896a5c1c77109c26624248d3df213788eb186c",
"reftest"
],
"css/absolute_img_with_padding_ref.html": [
"3f949e99ab09c5f8ce352a7987330d99023d7f0d",
"support"
],
"css/absolute_inline_containing_block_a.html": [
"8174a236497815db250a3b11aeb322e0e9c7c74f",
"reftest"
24 tests/wpt/mozilla/tests/css/absolute_div_with_padding.html
View file
Open in desktop
@@ -0,0 +1,24 @@
<!doctype html>
<meta charset="utf-8">
<title></title>
<link rel="match" href="absolute_div_with_padding_ref.html">
<style>
.absolute{
position: absolute;
top: 0px;
left: 0px;
padding: 100px;
width: 150px;
height: 150px;
background-color: green;
}
.box{
width: 350px;
height: 350px;
position: relative;
background: red;
}
</style>
<div class="box">
<div class="absolute"></div>
</div>
20 tests/wpt/mozilla/tests/css/absolute_div_with_padding_ref.html
View file
Open in desktop
@@ -0,0 +1,20 @@
<!doctype html>
<meta charset="utf-8">
<title></title>
<style>
.absolute{
padding: 100px;
width: 150px;
height: 150px;
background-color: green;
}
.box{
width: 350px;
height: 350px;
position: relative;
background: red;
}
</style>
<div class="box">
<div class="absolute"></div>
</div>
15 tests/wpt/mozilla/tests/css/absolute_img_with_padding.html
View file
Open in desktop
@@ -0,0 +1,15 @@
<!doctype html>
<meta charset="utf-8">
<title>CSS Test: Absolute position image with padding should not increase in size</title>
<link rel="match" href="absolute_img_with_padding_ref.html">
<body>
<style>
img{
position: absolute;
top: 0px;
left: 0px;
padding: 100px;
}
</style>
<img src="100x100_green.png" alt="img">
</body>
6 tests/wpt/mozilla/tests/css/absolute_img_with_padding_ref.html
View file
Open in desktop
@@ -0,0 +1,6 @@
<!doctype html>
<meta charset="utf-8">
<title>CSS Test: Absolute position image with padding should not increase in size</title>
<body style="margin:0">
<img style="padding: 100px;" src="100x100_green.png" alt="img">
</body>
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.