Fix #6799: set stacking_context_position correctly on fragment_border_iterator #16317

eloycoto · 2017-04-09T11:51:40Z

Hey,

First of all, this is my first PR to Servo project and I'm learning Rust, so sorry if you see something that it's not correct. I did that as best as I know.

This PR fix the issue #6799; I tried all the corner cases that I can think about it and always get the right result and the same as other browsers.

Related to the build:

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #6799

In the other hand, I added the test in the cssom folder, is where getBoundingClientRect is defined, so I think that is the best place.

I'm sure that the line 122 can be better, but I didn't find a way to transform a Point2D from f32 to px in a easy way.

I'm here to listen to your recommendations and fix any issue.
Thanks!

This change is

highfive · 2017-04-09T11:51:45Z

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

highfive · 2017-04-09T11:51:46Z

Heads up! This PR modifies the following files:

@emilio: components/layout/sequential.rs

highfive · 2017-04-09T11:51:47Z

Warning

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

stshine · 2017-04-09T12:01:32Z

@bors-servo try

bors-servo · 2017-04-09T12:01:35Z

⌛ Trying commit d993266 with merge a5b8da8...


        Auto merge of #16317 - eloycoto:issue6799, r=<try>

Fix #6799: set stacking_context_position correctly on fragment_border_iterator Hey, First of all, this is my first PR to Servo project and I'm learning Rust, so sorry if you see something that it's not correct. I did that as best as I know. This PR fix the issue #6799; I tried all the corner cases that I can think about it and always get the right result and the same as other browsers. Related to the build: - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #6799 In the other hand, I added the test in the cssom folder, is where getBoundingClientRect is defined, so I think that is the best place. I'm sure that the line 122 can be better, but I didn't find a way to transform a Point2D from f32 to px in a easy way. I'm here to listen to your recommendations and fix any issue. Thanks!  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16317)

bors-servo · 2017-04-09T12:04:40Z

💔 Test failed - linux-dev

jdm · 2017-04-09T21:42:07Z

@glennw Is this something you could look at?

emilio · 2017-04-10T05:27:02Z

@bors-servo try

bors-servo · 2017-04-10T05:27:06Z

⌛ Trying commit 53197c3 with merge 7453bd0...


        Auto merge of #16317 - eloycoto:issue6799, r=<try>

Fix #6799: set stacking_context_position correctly on fragment_border_iterator Hey, First of all, this is my first PR to Servo project and I'm learning Rust, so sorry if you see something that it's not correct. I did that as best as I know. This PR fix the issue #6799; I tried all the corner cases that I can think about it and always get the right result and the same as other browsers. Related to the build: - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #6799 In the other hand, I added the test in the cssom folder, is where getBoundingClientRect is defined, so I think that is the best place. I'm sure that the line 122 can be better, but I didn't find a way to transform a Point2D from f32 to px in a easy way. I'm here to listen to your recommendations and fix any issue. Thanks!  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16317)

bors-servo · 2017-04-10T05:36:10Z

💔 Test failed - arm64

emilio · 2017-04-10T05:44:01Z

components/layout/sequential.rs

@@ -23,7 +24,7 @@ pub use style::sequential::traverse_dom;
 pub fn resolve_generated_content(root: &mut Flow, layout_context: &LayoutContext) {
    fn doit(flow: &mut Flow, level: u32, traversal: &mut ResolveGeneratedContent) {
        if !traversal.should_process(flow) {
-            return
+            return;


It'd be nice to move the purely-stylistic changes to another commit so it's easier to review.

emilio · 2017-04-10T05:44:01Z

components/layout/sequential.rs

+                    let transform = fragment
+                        .fragment
+                        .transform_matrix(&relative_position)
+                        .unwrap_or(Matrix4D::identity())


This can't be hit, right? (we check for transform.is_some).

What about organizing this as follows?

let mut stacking_context_position = *stacking_context_position; if kid.is_block_flow() && kid.as_block().fragment.establishes_stacking_context() { stacking_context_position += flow::base(kid).stacking_context_position(); stacking_context_position += Point2D::new(kid.as_block().fragment.margin.inline_start, Au(0)); let relative_position = kid.as_block().stacking_relative_position(CoordinateSystem::Own); if let Some(matrix) = kid.as_block().transform_matrix(&relative_position) { stacking_context_position += matrix.transform_point(&Point2D::zero()); } }

emilio · 2017-04-10T05:44:01Z

components/layout/sequential.rs

+                if fragment.fragment.style.get_box().transform.0.is_some() {
+                    let relative_position = fragment
+                        .fragment
+                        .stacking_relative_border_box(&fragment.base.stacking_relative_position,


Let's factor this into a helper that takes a CoordinateSystem argument, since it's called from multiple places of layout with the same arguments.

emilio · 2017-04-10T05:51:24Z

You need to update the manifest with ./mach update-manifest

eloycoto · 2017-04-10T06:04:16Z

Hey @emilio

I'll try to make the changes this night and I'll come back to you tomorrow.

Regards and thanks!

nox · 2017-04-11T08:30:11Z

@bors-servo r=emilio

bors-servo · 2017-04-11T08:30:12Z

📌 Commit 53197c3 has been approved by emilio

nox · 2017-04-11T08:48:34Z

UGH.

@bors-servo r-

r? @emilio

jdm · 2017-05-28T13:24:08Z

@bors-servo: try

emilio

Looks fine to me, modulo that. Thanks a lot for working on this! :)

emilio · 2017-05-28T16:57:03Z

components/layout/block.rs

@@ -647,6 +647,14 @@ impl BlockFlow {
        &mut self.fragment
    }

+    pub fn stacking_relative_position(&self, coor: CoordinateSystem) -> Rect<Au> {


We should be able to reuse this function in block.rs, and also in inline.rs (InlineFlow::compute_absolute_position). Also in BlockFlow::transform_clip_to_coordinate_space in display_list_builder.rs, and I may have missed others.

It's fine for me if you don't want to do that (is fine to file a followup for that).

Blockflow is fixed. The inline one is a bit harder, I need to learn a bit more.

Regards.

stshine · 2017-05-30T06:59:13Z

@bors-servo try

bors-servo · 2017-05-30T06:59:17Z

⌛ Trying commit 33a4659 with merge 89b68c2...


        Auto merge of #16317 - eloycoto:issue6799, r=<try>

Fix #6799: set stacking_context_position correctly on fragment_border_iterator Hey, First of all, this is my first PR to Servo project and I'm learning Rust, so sorry if you see something that it's not correct. I did that as best as I know. This PR fix the issue #6799; I tried all the corner cases that I can think about it and always get the right result and the same as other browsers. Related to the build: - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #6799 In the other hand, I added the test in the cssom folder, is where getBoundingClientRect is defined, so I think that is the best place. I'm sure that the line 122 can be better, but I didn't find a way to transform a Point2D from f32 to px in a easy way. I'm here to listen to your recommendations and fix any issue. Thanks!  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16317)


        Fix #6799: set stacking_context_position correctly on

fragment_border_iterator

bors-servo · 2017-05-30T08:37:02Z

☀️ Test successful - android, 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
State: approved= try=True

emilio · 2017-05-30T09:15:35Z

@bors-servo r+

Thanks a lot!

bors-servo · 2017-05-30T09:15:36Z

📌 Commit 33a4659 has been approved by emilio

bors-servo · 2017-05-30T13:31:17Z

⌛ Testing commit 33a4659 with merge af159be...


        Auto merge of #16317 - eloycoto:issue6799, r=emilio

Fix #6799: set stacking_context_position correctly on fragment_border_iterator Hey, First of all, this is my first PR to Servo project and I'm learning Rust, so sorry if you see something that it's not correct. I did that as best as I know. This PR fix the issue #6799; I tried all the corner cases that I can think about it and always get the right result and the same as other browsers. Related to the build: - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #6799 In the other hand, I added the test in the cssom folder, is where getBoundingClientRect is defined, so I think that is the best place. I'm sure that the line 122 can be better, but I didn't find a way to transform a Point2D from f32 to px in a easy way. I'm here to listen to your recommendations and fix any issue. Thanks!  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16317)

bors-servo · 2017-05-30T13:47:09Z

💔 Test failed - linux-dev

stshine · 2017-05-30T14:07:09Z

@bors-servo retry

infra

bors-servo · 2017-05-30T16:28:28Z

⌛ Testing commit 33a4659 with merge 9d32b9c...


        Auto merge of #16317 - eloycoto:issue6799, r=emilio

Fix #6799: set stacking_context_position correctly on fragment_border_iterator Hey, First of all, this is my first PR to Servo project and I'm learning Rust, so sorry if you see something that it's not correct. I did that as best as I know. This PR fix the issue #6799; I tried all the corner cases that I can think about it and always get the right result and the same as other browsers. Related to the build: - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #6799 In the other hand, I added the test in the cssom folder, is where getBoundingClientRect is defined, so I think that is the best place. I'm sure that the line 122 can be better, but I didn't find a way to transform a Point2D from f32 to px in a easy way. I'm here to listen to your recommendations and fix any issue. Thanks!  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16317)

bors-servo · 2017-05-30T17:29:11Z

☀️ Test successful - android, 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: emilio
Pushing 9d32b9c to master...


        Fix #6799: set stacking_context_position correctly on

fragment_border_iterator Upstreamed from servo/servo#16317 [ci skip]

highfive assigned nox Apr 9, 2017

highfive added the S-awaiting-review label Apr 9, 2017

highfive added S-tests-failed and removed S-tests-failed labels Apr 9, 2017

highfive added the S-tests-failed label Apr 10, 2017

emilio reviewed Apr 10, 2017

View changes

highfive assigned emilio and unassigned nox Apr 11, 2017

highfive added S-awaiting-merge and removed S-awaiting-review S-tests-failed labels Apr 11, 2017

nox added S-needs-code-changes and removed S-awaiting-merge labels Apr 11, 2017

emilio reviewed May 28, 2017

View changes

eloycoto force-pushed the eloycoto:issue6799 branch from f5c011f to 33a4659 May 30, 2017

highfive removed the S-tests-failed label May 30, 2017

Fix #6799: set stacking_context_position correctly on

Loading status checks…

33a4659

fragment_border_iterator

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-rebase labels May 30, 2017

highfive added S-tests-failed and removed S-awaiting-merge labels May 30, 2017

highfive added S-awaiting-merge and removed S-tests-failed labels May 30, 2017

bors-servo merged commit 33a4659 into servo:master May 30, 2017
2 of 3 checks passed

2 of 3 checks passed

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

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label May 30, 2017

jdm mentioned this pull request Jun 5, 2017

"Node was being treated as inline, but it has an associated fragment!" on washingtonpost.com #17156

Open

jdm added a commit to web-platform-tests/wpt that referenced this pull request Jun 19, 2017

Fix #6799: set stacking_context_position correctly on

f13010a

fragment_border_iterator Upstreamed from servo/servo#16317 [ci skip]

jdm mentioned this pull request Jun 19, 2017

Fix #6799: set stacking_context_position correctly on web-platform-tests/wpt#6282

Merged

servo / servo

Fix #6799: set stacking_context_position correctly on fragment_border_iterator #16317

Fix #6799: set stacking_context_position correctly on fragment_border_iterator #16317

eloycoto commented Apr 9, 2017 •

edited by larsbergstrom

highfive commented Apr 9, 2017

highfive commented Apr 9, 2017

highfive commented Apr 9, 2017

stshine commented Apr 9, 2017

bors-servo commented Apr 9, 2017

bors-servo commented Apr 9, 2017

jdm commented Apr 9, 2017

emilio commented Apr 10, 2017

bors-servo commented Apr 10, 2017

bors-servo commented Apr 10, 2017

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

emilio commented Apr 10, 2017

eloycoto commented Apr 10, 2017

nox commented Apr 11, 2017

bors-servo commented Apr 11, 2017

nox commented Apr 11, 2017

jdm commented May 28, 2017

emilio left a comment

This comment has been minimized.

This comment has been minimized.

stshine commented May 30, 2017

bors-servo commented May 30, 2017

bors-servo commented May 30, 2017

emilio commented May 30, 2017

bors-servo commented May 30, 2017

bors-servo commented May 30, 2017

bors-servo commented May 30, 2017

stshine commented May 30, 2017

bors-servo commented May 30, 2017

bors-servo commented May 30, 2017

servo / servo

Join GitHub today

Fix #6799: set stacking_context_position correctly on fragment_border_iterator #16317

Fix #6799: set stacking_context_position correctly on fragment_border_iterator #16317

Conversation

eloycoto commented Apr 9, 2017 • edited by larsbergstrom

highfive commented Apr 9, 2017

highfive commented Apr 9, 2017

highfive commented Apr 9, 2017

stshine commented Apr 9, 2017

bors-servo commented Apr 9, 2017

bors-servo commented Apr 9, 2017

jdm commented Apr 9, 2017

emilio commented Apr 10, 2017

bors-servo commented Apr 10, 2017

bors-servo commented Apr 10, 2017

This comment has been minimized.

emilio Apr 10, 2017

This comment has been minimized.

emilio Apr 10, 2017

This comment has been minimized.

emilio Apr 10, 2017

emilio commented Apr 10, 2017

eloycoto commented Apr 10, 2017

nox commented Apr 11, 2017

bors-servo commented Apr 11, 2017

nox commented Apr 11, 2017

jdm commented May 28, 2017

emilio left a comment

This comment has been minimized.

emilio May 28, 2017

This comment has been minimized.

eloycoto May 30, 2017

stshine commented May 30, 2017

bors-servo commented May 30, 2017

bors-servo commented May 30, 2017

emilio commented May 30, 2017

bors-servo commented May 30, 2017

bors-servo commented May 30, 2017

bors-servo commented May 30, 2017

stshine commented May 30, 2017

bors-servo commented May 30, 2017

bors-servo commented May 30, 2017

eloycoto commented Apr 9, 2017 •

edited by larsbergstrom