Rounding is off-by-one #490

New Issue

Closed

opened 2017-03-22 15:16:30 -07:00 by arcanis · 10 comments

arcanis commented 2017-03-22 15:16:30 -07:00 (Migrated from github.com)

Copy Link

Here is a layout:

<div id="rounding_nested" experiments="Rounding" style="flex-direction: row; width: 25px; height: 25px;">
  <div style="height: 100%; width: 100%; flex: 1;"></div>
  <div style="height: 100%; width: 2px"></div>
  <div style="height: 100%; width: 100%; flex: 1; flex-direction: column;">
    <div style="width: 100%; height: 100%; flex: 1;"></div>
    <div style="width: 100%; height: 1px"></div>
    <div style="width: 100%; height: 100%; flex: 1;"></div>
  </div>
</div>

The autogenerated file is as such:

console.assert(0 === root.getComputedLeft(), "0 === root.getComputedLeft() (" + root.getComputedLeft() + ")");
console.assert(0 === root.getComputedTop(), "0 === root.getComputedTop() (" + root.getComputedTop() + ")");
console.assert(25 === root.getComputedWidth(), "25 === root.getComputedWidth() (" + root.getComputedWidth() + ")");
console.assert(25 === root.getComputedHeight(), "25 === root.getComputedHeight() (" + root.getComputedHeight() + ")");

console.assert(0 === root_child0.getComputedLeft(), "0 === root_child0.getComputedLeft() (" + root_child0.getComputedLeft() + ")");
console.assert(0 === root_child0.getComputedTop(), "0 === root_child0.getComputedTop() (" + root_child0.getComputedTop() + ")");
console.assert(12 === root_child0.getComputedWidth(), "12 === root_child0.getComputedWidth() (" + root_child0.getComputedWidth() + ")");
console.assert(25 === root_child0.getComputedHeight(), "25 === root_child0.getComputedHeight() (" + root_child0.getComputedHeight() + ")");

console.assert(12 === root_child1.getComputedLeft(), "12 === root_child1.getComputedLeft() (" + root_child1.getComputedLeft() + ")");
console.assert(0 === root_child1.getComputedTop(), "0 === root_child1.getComputedTop() (" + root_child1.getComputedTop() + ")");
console.assert(2 === root_child1.getComputedWidth(), "2 === root_child1.getComputedWidth() (" + root_child1.getComputedWidth() + ")");
console.assert(25 === root_child1.getComputedHeight(), "25 === root_child1.getComputedHeight() (" + root_child1.getComputedHeight() + ")");

console.assert(14 === root_child2.getComputedLeft(), "14 === root_child2.getComputedLeft() (" + root_child2.getComputedLeft() + ")");
console.assert(0 === root_child2.getComputedTop(), "0 === root_child2.getComputedTop() (" + root_child2.getComputedTop() + ")");
console.assert(11 === root_child2.getComputedWidth(), "11 === root_child2.getComputedWidth() (" + root_child2.getComputedWidth() + ")");
console.assert(25 === root_child2.getComputedHeight(), "25 === root_child2.getComputedHeight() (" + root_child2.getComputedHeight() + ")");

console.assert(0 === root_child2_child0.getComputedLeft(), "0 === root_child2_child0.getComputedLeft() (" + root_child2_child0.getComputedLeft() + ")");
console.assert(0 === root_child2_child0.getComputedTop(), "0 === root_child2_child0.getComputedTop() (" + root_child2_child0.getComputedTop() + ")");
console.assert(12 === root_child2_child0.getComputedWidth(), "12 === root_child2_child0.getComputedWidth() (" + root_child2_child0.getComputedWidth() + ")");
console.assert(12 === root_child2_child0.getComputedHeight(), "12 === root_child2_child0.getComputedHeight() (" + root_child2_child0.getComputedHeight() + ")");

console.assert(0 === root_child2_child1.getComputedLeft(), "0 === root_child2_child1.getComputedLeft() (" + root_child2_child1.getComputedLeft() + ")");
console.assert(12 === root_child2_child1.getComputedTop(), "12 === root_child2_child1.getComputedTop() (" + root_child2_child1.getComputedTop() + ")");
console.assert(12 === root_child2_child1.getComputedWidth(), "12 === root_child2_child1.getComputedWidth() (" + root_child2_child1.getComputedWidth() + ")");
console.assert(1 === root_child2_child1.getComputedHeight(), "1 === root_child2_child1.getComputedHeight() (" + root_child2_child1.getComputedHeight() + ")");

console.assert(0 === root_child2_child2.getComputedLeft(), "0 === root_child2_child2.getComputedLeft() (" + root_child2_child2.getComputedLeft() + ")");
console.assert(13 === root_child2_child2.getComputedTop(), "13 === root_child2_child2.getComputedTop() (" + root_child2_child2.getComputedTop() + ")");
console.assert(12 === root_child2_child2.getComputedWidth(), "12 === root_child2_child2.getComputedWidth() (" + root_child2_child2.getComputedWidth() + ")");
console.assert(12 === root_child2_child2.getComputedHeight(), "12 === root_child2_child2.getComputedHeight() (" + root_child2_child2.getComputedHeight() + ")");

As you can see, the last three elements all have a computed width equal to 12px. However, their parent node has a computed width equal to 11px!

Funnily, that's not the behaviour that I observe in my app (but it is still broken). Instead, the left panel computed width is 11px, the right panel computed width is 12px, and the children computed width is 11px wide (making them smaller than their parent, instead of bigger as in the aforementioned issue). You can witness this issue here (you can play with the container width to see that the other sizes aren't affected).

Here is a layout: ```html <div id="rounding_nested" experiments="Rounding" style="flex-direction: row; width: 25px; height: 25px;"> <div style="height: 100%; width: 100%; flex: 1;"></div> <div style="height: 100%; width: 2px"></div> <div style="height: 100%; width: 100%; flex: 1; flex-direction: column;"> <div style="width: 100%; height: 100%; flex: 1;"></div> <div style="width: 100%; height: 1px"></div> <div style="width: 100%; height: 100%; flex: 1;"></div> </div> </div> ``` The autogenerated file is as such: ```js console.assert(0 === root.getComputedLeft(), "0 === root.getComputedLeft() (" + root.getComputedLeft() + ")"); console.assert(0 === root.getComputedTop(), "0 === root.getComputedTop() (" + root.getComputedTop() + ")"); console.assert(25 === root.getComputedWidth(), "25 === root.getComputedWidth() (" + root.getComputedWidth() + ")"); console.assert(25 === root.getComputedHeight(), "25 === root.getComputedHeight() (" + root.getComputedHeight() + ")"); console.assert(0 === root_child0.getComputedLeft(), "0 === root_child0.getComputedLeft() (" + root_child0.getComputedLeft() + ")"); console.assert(0 === root_child0.getComputedTop(), "0 === root_child0.getComputedTop() (" + root_child0.getComputedTop() + ")"); console.assert(12 === root_child0.getComputedWidth(), "12 === root_child0.getComputedWidth() (" + root_child0.getComputedWidth() + ")"); console.assert(25 === root_child0.getComputedHeight(), "25 === root_child0.getComputedHeight() (" + root_child0.getComputedHeight() + ")"); console.assert(12 === root_child1.getComputedLeft(), "12 === root_child1.getComputedLeft() (" + root_child1.getComputedLeft() + ")"); console.assert(0 === root_child1.getComputedTop(), "0 === root_child1.getComputedTop() (" + root_child1.getComputedTop() + ")"); console.assert(2 === root_child1.getComputedWidth(), "2 === root_child1.getComputedWidth() (" + root_child1.getComputedWidth() + ")"); console.assert(25 === root_child1.getComputedHeight(), "25 === root_child1.getComputedHeight() (" + root_child1.getComputedHeight() + ")"); console.assert(14 === root_child2.getComputedLeft(), "14 === root_child2.getComputedLeft() (" + root_child2.getComputedLeft() + ")"); console.assert(0 === root_child2.getComputedTop(), "0 === root_child2.getComputedTop() (" + root_child2.getComputedTop() + ")"); console.assert(11 === root_child2.getComputedWidth(), "11 === root_child2.getComputedWidth() (" + root_child2.getComputedWidth() + ")"); console.assert(25 === root_child2.getComputedHeight(), "25 === root_child2.getComputedHeight() (" + root_child2.getComputedHeight() + ")"); console.assert(0 === root_child2_child0.getComputedLeft(), "0 === root_child2_child0.getComputedLeft() (" + root_child2_child0.getComputedLeft() + ")"); console.assert(0 === root_child2_child0.getComputedTop(), "0 === root_child2_child0.getComputedTop() (" + root_child2_child0.getComputedTop() + ")"); console.assert(12 === root_child2_child0.getComputedWidth(), "12 === root_child2_child0.getComputedWidth() (" + root_child2_child0.getComputedWidth() + ")"); console.assert(12 === root_child2_child0.getComputedHeight(), "12 === root_child2_child0.getComputedHeight() (" + root_child2_child0.getComputedHeight() + ")"); console.assert(0 === root_child2_child1.getComputedLeft(), "0 === root_child2_child1.getComputedLeft() (" + root_child2_child1.getComputedLeft() + ")"); console.assert(12 === root_child2_child1.getComputedTop(), "12 === root_child2_child1.getComputedTop() (" + root_child2_child1.getComputedTop() + ")"); console.assert(12 === root_child2_child1.getComputedWidth(), "12 === root_child2_child1.getComputedWidth() (" + root_child2_child1.getComputedWidth() + ")"); console.assert(1 === root_child2_child1.getComputedHeight(), "1 === root_child2_child1.getComputedHeight() (" + root_child2_child1.getComputedHeight() + ")"); console.assert(0 === root_child2_child2.getComputedLeft(), "0 === root_child2_child2.getComputedLeft() (" + root_child2_child2.getComputedLeft() + ")"); console.assert(13 === root_child2_child2.getComputedTop(), "13 === root_child2_child2.getComputedTop() (" + root_child2_child2.getComputedTop() + ")"); console.assert(12 === root_child2_child2.getComputedWidth(), "12 === root_child2_child2.getComputedWidth() (" + root_child2_child2.getComputedWidth() + ")"); console.assert(12 === root_child2_child2.getComputedHeight(), "12 === root_child2_child2.getComputedHeight() (" + root_child2_child2.getComputedHeight() + ")"); ``` As you can see, the last three elements all have a computed width equal to 12px. However, their parent node has a computed width equal to 11px! Funnily, that's not the behaviour that I observe in my app (but it is still broken). Instead, the left panel computed width is 11px, the right panel computed width is 12px, and the children computed width is 11px wide (making them smaller than their parent, instead of bigger as in the aforementioned issue). You can witness this issue [here](https://manaflair.github.io/mylittledom/demo/#custom:import%20%7B%20render%20%7D%20%20%20%20%20%20from%20'%40manaflair%2Fmylittledom%2Fterm%2Freact'%3B%0Aimport%20%7B%20TermElement%20%7D%20from%20'%40manaflair%2Fmylittledom%2Fterm'%3B%0A%0Alet%20A%20%3D%20%7B%20flex%3A%201%2C%20background%3A%20%60red%60%2C%20width%3A%20%60100%25%60%2C%20height%3A%20%60100%25%60%20%7D%3B%0A%0Alet%20B1%20%3D%20%7B%20flex%3A%20null%2C%20background%3A%20%60white%60%2C%20width%3A%202%2C%20height%3A%20%60100%25%60%20%7D%3B%0Alet%20B2%20%3D%20%7B%20flex%3A%20null%2C%20background%3A%20%60white%60%2C%20width%3A%20%60100%25%60%2C%20height%3A%201%20%7D%3B%0A%0Alet%20C1%20%3D%20%7B%20flex%3A%201%2C%20background%3A%20%60blue%60%2C%20width%3A%20%60100%25%60%2C%20height%3A%20%60100%25%60%2C%20flexDirection%3A%20%60column%60%20%7D%3B%0Alet%20C2%20%3D%20%7B%20flex%3A%201%2C%20background%3A%20%60blue%60%2C%20width%3A%20%60100%25%60%2C%20height%3A%20%60100%25%60%2C%20flexDirection%3A%20%60row%60%20%7D%3B%0A%0Arender(%3Cdiv%20style%3D%7B%7B%20width%3A%2025%2C%20height%3A%2025%2C%20flexDirection%3A%20%60row%60%20%7D%7D%3E%0A%0A%20%20%3Cdiv%20style%3D%7BA%7D%20%2F%3E%0A%20%20%3Cdiv%20style%3D%7BB1%7D%20%2F%3E%0A%0A%20%20%3Cdiv%20style%3D%7BC1%7D%3E%0A%20%20%20%20%3Cdiv%20style%3D%7BA%7D%20%2F%3E%0A%20%20%20%20%3Cdiv%20style%3D%7BB2%7D%20%2F%3E%0A%20%20%20%20%3Cdiv%20style%3D%7BC2%7D%20%2F%3E%0A%20%20%3C%2Fdiv%3E%0A%0A%3C%2Fdiv%3E%2C%20screen)%3B%0A) (you can play with the container width to see that the other sizes aren't affected).

woehrl01 commented 2017-03-23 12:22:03 -07:00 (Migrated from github.com)

Copy Link

This is pretty easily solvable by using the absolute position instead of the local position here.

I had that at first, but the generated tests are failing against the result from chrome with this. Not sure how to combine this with an assertion on the chrome outputs. Use this just with an experimental?

This is pretty easily solvable by using the absolute position instead of the local position [here](https://github.com/facebook/yoga/blob/master/yoga/Yoga.c#L3328-L3329). I had that at first, but the generated tests are failing against the result from chrome with this. Not sure how to combine this with an assertion on the chrome outputs. Use this just with an experimental?

arcanis commented 2017-03-23 12:33:09 -07:00 (Migrated from github.com)

Copy Link

Hm ... would that fix everything if the tests relying on Yoga-only features (such as rounding) were manually written?

Hm ... would that fix everything if the tests relying on Yoga-only features (such as rounding) were manually written?

woehrl01 commented 2017-03-23 12:37:52 -07:00 (Migrated from github.com)

Copy Link

Jep, those tests must be manually written. But also the percentage feature in some cases relies on the rounding things. One idea could be to introduce an experimental feature which rounds "correctly" which is enabled per default, and can be disabled for the tests, to still relie on chrome.

Jep, those tests must be manually written. But also the percentage feature in some cases relies on the rounding things. One idea could be to introduce an experimental feature which rounds "correctly" which is enabled per default, and can be disabled for the tests, to still relie on chrome.

arcanis commented 2017-03-23 16:57:34 -07:00 (Migrated from github.com)

Copy Link

Hm I see. However, I'm not completely sure it's only because of the position: if I take an 13px element and split it in two equal parts (each ending up being 11.5px), then the rounding will adjust them to respectively 12px and 11px. The issue is when the second one has children, because in this case those children still think that they are 11.5px, and so the rounding put them at 12px (because they don't benefit from the fractial leftovers from their parents).

Maybe passing the leftovers from a node to its children would be enough to fix this...

You can see a better example here. The blue box is the parent, and its child (the yellow box) takes more space than it should.

Hm I see. However, I'm not completely sure it's only because of the position: if I take an 13px element and split it in two equal parts (each ending up being 11.5px), then the rounding will adjust them to respectively 12px and 11px. The issue is when the second one has children, because in this case those children still think that they are 11.5px, and so the rounding put them at 12px (because they don't benefit from the fractial leftovers from their parents). Maybe passing the leftovers from a node to its children would be enough to fix this... You can see a better example [here](https://manaflair.github.io/mylittledom/demo/#custom:import%20%7B%20render%20%7D%20%20%20%20%20%20from%20'%40manaflair%2Fmylittledom%2Fterm%2Freact'%3B%0Aimport%20%7B%20TermElement%20%7D%20from%20'%40manaflair%2Fmylittledom%2Fterm'%3B%0A%0Alet%20A%20%3D%20%7B%20flex%3A%201%2C%20background%3A%20%60red%60%2C%20width%3A%20%60100%25%60%2C%20height%3A%20%60100%25%60%20%7D%3B%0A%0Alet%20B1%20%3D%20%7B%20flex%3A%20null%2C%20background%3A%20%60white%60%2C%20width%3A%202%2C%20height%3A%20%60100%25%60%20%7D%3B%0Alet%20B2%20%3D%20%7B%20flex%3A%20null%2C%20background%3A%20%60white%60%2C%20width%3A%20%60100%25%60%2C%20height%3A%201%20%7D%3B%0A%0Alet%20C1%20%3D%20%7B%20flex%3A%201%2C%20background%3A%20%60blue%60%2C%20width%3A%20%60100%25%60%2C%20height%3A%20%60100%25%60%2C%20flexDirection%3A%20%60column%60%20%7D%3B%0Alet%20C2%20%3D%20%7B%20flex%3A%201%2C%20background%3A%20%60blue%60%2C%20width%3A%20%60100%25%60%2C%20height%3A%20%60100%25%60%2C%20flexDirection%3A%20%60row%60%20%7D%3B%0A%0Arender(%3Cdiv%20style%3D%7B%7B%20width%3A%2011%2C%20height%3A%2010%2C%20flexDirection%3A%20%60row%60%20%7D%7D%3E%0A%09%3Cdiv%20style%3D%7B%7B%20flex%3A%201%2C%20height%3A%20%60100%25%60%2C%20background%3A%20%60red%60%20%7D%7D%20%2F%3E%0A%09%3Cdiv%20style%3D%7B%7B%20flex%3A%201%2C%20height%3A%20%60100%25%60%2C%20background%3A%20%60blue%60%20%7D%7D%3E%0A%20%20%20%20%20%20%20%09%3Cdiv%20style%3D%7B%7B%20width%3A%20%60100%25%60%2C%20height%3A%20%6050%25%60%2C%20background%3A%20%60yellow%60%20%7D%7D%20%2F%3E%0A%20%20%20%20%3C%2Fdiv%3E%0A%3C%2Fdiv%3E%2C%20screen)%3B%0A). The blue box is the parent, and its child (the yellow box) takes more space than it should.

arcanis commented 2017-03-24 09:30:49 -07:00 (Migrated from github.com)

Copy Link

I have made a tentative fix (396050e1dd (diff-2886721427e8c2829a4b8cd59f1c62c9)), I'll check later if it actually works in every case.

I have made a tentative fix (https://github.com/arcanis/yoga/commit/396050e1ddc698ede6d9ec7c08bec8ba98b86eee#diff-2886721427e8c2829a4b8cd59f1c62c9), I'll check later if it actually works in every case.

arcanis commented 2017-03-27 02:26:50 -07:00 (Migrated from github.com)

Copy Link

The previous commit was faulty, but the following one works fine in all the cases I've stress-tested:

acfe842a21

A single test is failing (rounding_nested_3, I think), but I haven't discovered yet how experiment assertions such as those used for the rounding experiment are generated from the fixtures, so I haven't been able to fix it.

Would you like a PR with this commit?

The previous commit was faulty, but the following one works fine in all the cases I've stress-tested: https://github.com/arcanis/yoga/commit/acfe842a210d0252304f6babab00f5491938221c A single test is failing (rounding_nested_3, I think), but I haven't discovered yet how experiment assertions such as those used for the rounding experiment are generated from the fixtures, so I haven't been able to fix it. Would you like a PR with this commit?

arcanis commented 2017-04-12 03:00:28 -07:00 (Migrated from github.com)

Copy Link

@woehrl01 Would you accept a PR that would add support for expect-*="<number>" attributes, to override the values returned by Chrome? This way the tests could still be mostly autogenerated.

@woehrl01 Would you accept a PR that would add support for `expect-*="<number>"` attributes, to override the values returned by Chrome? This way the tests could still be mostly autogenerated.

woehrl01 commented 2017-04-12 03:03:16 -07:00 (Migrated from github.com)

Copy Link

sounds like a good idea, but the decision is up to @emilsjolander .

sounds like a good idea, but the decision is up to @emilsjolander .

shergin commented 2017-04-25 18:03:43 -07:00 (Migrated from github.com)

Copy Link

I fixed it in aa5b296ac7. 🎉
(Sorry @arcanis, I had not seen your approach before I landed my solution today... 😳 )

I fixed it in aa5b296ac78f7a22e1aeaf4891243c6bb76488e2. 🎉 (Sorry @arcanis, I had not seen your approach before I landed my solution today... 😳 )

arcanis commented 2017-04-26 14:52:27 -07:00 (Migrated from github.com)

Copy Link

Happy to see it fixed, whoever landed the final hit! 👍

I've checked my use case, and it seems to work fine now, good job :)

Happy to see it fixed, whoever landed the final hit! 👍 I've checked my use case, and it seems to work fine now, good job :)

Sign in to join this conversation.

No Branch/Tag Specified

Branches Tags

main

export-D80452590

export-D80174965

export-D80174963

export-D70986391

export-D70792047

NickGerleman-ubuntu-24-04

release-v3.2

dependabot/npm_and_yarn/cross-spawn-7.0.6

dependabot/npm_and_yarn/http-proxy-middleware-2.0.7

dependabot/npm_and_yarn/express-4.21.0

dependabot/npm_and_yarn/micromatch-4.0.8

dependabot/npm_and_yarn/webpack-5.94.0

release-v3.1

release-v3.0

release-v2.0

v3.2.1

v3.2.0

v3.1.0

v3.0.4

v3.0.3

v3.0.2

v3.0.1

v3.0.0

v2.0.1

v2.0.0

v2.0.0-beta.2

v2.0.0-beta.1

v1.19.0

1.18.0

v1.17.0

1.17.0

1.15.0

1.16.0

1.14.0

1.12.0-pre.3

1.12.0-pre.2

v1.13.0

v1.12.0-pre.1

1.12.0-pre.1

v1.12.0-pre.0

1.11.0

1.10.0

1.9.0

v1.8.0

1.8.0

1.7.0

1.6.0

1.5.2

1.5.1

1.5.0

1.4.1

1.4.0

1.3.0

1.2.0

v2017.02.07.00

v2017.01.27.00

v2017.01.23.00

v2017.01.16.00

v2017.01.09.00

v2017.01.02.00

v2016.12.26.00

v2016.12.19.00

v2016.12.12.00

v2016.12.05.00

v2016.11.28.00

v2016.11.21.00

v2016.11.14.00

v1.1.1

v1.1.0

v1.0.0

v0.0.6

v0.0.5

v0.0.4

0.0.3

Labels

Clear labels

Algorithm

Build and Infrastructure

C#

C++ Enhancement

CLA Signed

Discussion

Documentation

Feature Request

Java

JavaScript

Merged

Question

Reverted

Shared with Meta

Applied via automation to indicate that an Issue or Pull Request has been shared with the team.

YogaKit

YogaLayout ViewGroup

bug

dependencies

Pull requests that update a dependency file

duplicate

enhancement

fb-exported

fh:direct-merge-enabled

invalid

ruby

Pull requests that update Ruby code

wontfix

No Label

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

DaddyFrosty

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: DaddyFrosty/yoga#490

No description provided.