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 getBoundingClientRect does not account for transform or margin #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...

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

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:27:48Z

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:40:12Z

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:40:39Z

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:56:36Z

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

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)

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

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

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

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

highfive assigned nox Apr 9, 2017

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Apr 9, 2017

highfive added S-tests-failed The changes caused existing tests to fail. and removed S-tests-failed The changes caused existing tests to fail. labels Apr 9, 2017

highfive added the S-tests-failed The changes caused existing tests to fail. label Apr 10, 2017

emilio reviewed Apr 10, 2017

View reviewed changes

highfive assigned emilio and unassigned nox Apr 11, 2017

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. S-tests-failed The changes caused existing tests to fail. labels Apr 11, 2017

nox added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Apr 11, 2017

emilio reviewed May 28, 2017

View reviewed changes

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

highfive removed the S-tests-failed The changes caused existing tests to fail. label May 30, 2017

Fix servo#6799: set stacking_context_position correctly on

33a4659

fragment_border_iterator

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. S-needs-rebase There are merge conflict errors. labels May 30, 2017

highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels May 30, 2017

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-tests-failed The changes caused existing tests to fail. labels May 30, 2017

bors-servo merged commit 33a4659 into servo:master May 30, 2017

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. 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 pushed 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

emilio Apr 10, 2017

emilio Apr 10, 2017

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

emilio May 28, 2017

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

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

emilio Apr 10, 2017

Choose a reason for hiding this comment

emilio Apr 10, 2017

Choose a reason for hiding this comment

emilio Apr 10, 2017

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

emilio May 28, 2017

Choose a reason for hiding this comment

eloycoto May 30, 2017

Choose a reason for hiding this comment

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