Skip to content
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

Text node shrinks on each layout pass due to bug in YGRoundToPixelGrid #824

Closed
rigdern opened this issue Oct 9, 2018 · 0 comments
Closed

Comments

@rigdern
Copy link

rigdern commented Oct 9, 2018

Repro

If you run this program:

static YGSize measureText(
                           YGNodeRef node,
                           float width,
                           YGMeasureMode widthMode,
                           float height,
                           YGMeasureMode heightMode
                           ) {
  return (YGSize){
    .width = 10,
    .height = 10
  };
}

int main(int argc, const char * argv[]) {
  const YGConfigRef config = YGConfigNew();
  YGConfigSetPointScaleFactor(config, 2);
  
  const YGNodeRef root = YGNodeNewWithConfig(config);
  YGNodeStyleSetMargin(root, YGEdgeTop, -1.49);
  YGNodeStyleSetWidth(root, 500);
  YGNodeStyleSetHeight(root, 500);

  const YGNodeRef node0 = YGNodeNewWithConfig(config);
  YGNodeInsertChild(root, node0, 0);
  
  const YGNodeRef node1 = YGNodeNewWithConfig(config);
  YGNodeSetMeasureFunc(node1, measureText);
  YGNodeInsertChild(node0, node1, 0);
  
  for (int i = 0; i < 5; i++) {
    // dirty the tree so YGRoundToPixelGrid runs again
    YGNodeStyleSetMargin(root, YGEdgeLeft, i + 1);

    YGNodeCalculateLayout(root, YGUndefined, YGUndefined, YGDirectionLTR);
    YGNodePrint(node1, YGPrintOptionsLayout);
    printf("\n");
  }
  
  YGNodeFreeRecursive(root);
  
  YGConfigFree(config);

  return 0;
}

you'll see this output:

<div layout="width: 500; height: 9.5; top: 0; left: 0;" ></div>
<div layout="width: 500; height: 9; top: 0; left: 0;" ></div>
<div layout="width: 500; height: 8.5; top: 0; left: 0;" ></div>
<div layout="width: 500; height: 8; top: 0; left: 0;" ></div>
<div layout="width: 500; height: 7.5; top: 0; left: 0;" ></div>

Unexpected result: On each layout pass node1's height shrinks even though the input tree hasn't changed in a way that should affect node1's height.

Analysis

There seem to be a couple of factors that make this bug possible:

  1. The rounded height calculated by YGRoundToPixelGrid in the previous layout pass is used as input to YGRoundToPixelGrid in the next layout pass. Consequently, the input to the layout pass is different enabling us to get a different result.
  2. The top of the node has a negative position and the bottom of the node has a positive position. This enables the top and bottom to round differently such that we lose 0.5 on each layout pass.

Here's an example of the bug in YGRoundToPixelGrid in action. YGRoundToPixelGrid has this expression:

  node->setLayoutDimension(
      YGRoundValueToPixelGrid(
          absoluteNodeBottom,
          pointScaleFactor,
          (textRounding && hasFractionalHeight),
          (textRounding && !hasFractionalHeight)) -
          YGRoundValueToPixelGrid(
              absoluteNodeTop, pointScaleFactor, false, textRounding),
      YGDimensionHeight);

Let's simplify it to:

  roundedNodeHeight =
      YGRoundValueToPixelGrid(
          absoluteNodeBottom,
          pointScaleFactor,
          (textRounding && hasFractionalHeight),
          (textRounding && !hasFractionalHeight)) -
          YGRoundValueToPixelGrid(
              absoluteNodeTop, pointScaleFactor, false, textRounding)

And we'll use Round(x) to mean YGRoundValueToPixelGrid(x, ...) (the rest of YGRoundValueToPixelGrid's arguments are omitted from Round to make it easier to focus on the important stuff for this bug). So the expression becomes:

roundedNodeHeight = Round(absoluteNodeBottom) - Round(absoluteNodeTop)

Layout pass 1

Now suppose that we have the following YGRoundToPixelGrid variables:

nodeHeight = 10
absoluteNodeTop = -1.49
absoluteNodeBottom = absoluteNodeTop + nodeHeight = -1.49 + 10 = 8.51

Then roundedNodeHeight is:

roundedNodeHeight = Round(absoluteNodeBottom) - Round(absoluteNodeTop)
                  = Round(8.51) - Round(-1.49) = 8.5 - -1.0 = 9.5

So the input node height was 10 and the output is 9.5 which is also used as input for the next layout pass.

Layout pass 2

nodeHeight = 9.5 // from the previous layout pass
absoluteNodeTop = -1.49
absoluteNodeBottom = absoluteNodeTop + nodeHeight = -1.49 + 9.5 = 8.01

Then roundedNodeHeight is:

roundedNodeHeight = Round(absoluteNodeBottom) - Round(absoluteNodeTop)
                  = Round(8.01) - Round(-1.49) = 8.0 - -1.0 = 9.0

So the input node height was 9.5 and the output is 9.0.

In this way, each layout pass will decrease the node's height by 0.5.

Adam Comella
Microsoft Corp.

rigdern pushed a commit to rigdern/css-layout that referenced this issue Oct 9, 2018
`YGRoundValueToPixelGrid` currently rounds negative numbers incorrectly. For example:

```
YGRoundValueToPixelGrid(-2.2, 1.0, /* ceil */ false, /* floor */ true) = -2.0
```

However, that operation is supposed to take the floor of the number so the result should acutally be `-3.0`.

There's a detailed comment in `YGRoundValueToPixelGrid` about the fix and why it works.

A symptom that manifested because of this bug is that text nodes could get smaller and smaller on each layout pass. For details see facebook#824.

Adam Comella
Microsoft Corp.
rigdern pushed a commit to rigdern/css-layout that referenced this issue Oct 9, 2018
`YGRoundValueToPixelGrid` currently rounds negative numbers incorrectly. For example:

```
YGRoundValueToPixelGrid(-2.2, 1.0, /* ceil */ false, /* floor */ true) = -2.0
```

However, that operation is supposed to take the floor of the number so the result should acutally be `-3.0`.

There's a detailed comment in `YGRoundValueToPixelGrid` about the fix and why it works.

A symptom that manifested because of this bug is that text nodes could get smaller and smaller on each layout pass. For details see facebook#824.

Adam Comella
Microsoft Corp.
rigdern pushed a commit to rigdern/css-layout that referenced this issue Oct 9, 2018
`YGRoundValueToPixelGrid` currently rounds negative numbers incorrectly. For example:

```
YGRoundValueToPixelGrid(-2.2, 1.0, /* ceil */ false, /* floor */ true) = -2.0
```

However, that operation is supposed to take the floor of the number so the result should acutally be `-3.0`.

There's a detailed comment in `YGRoundValueToPixelGrid` about the fix and why it works.

A symptom that manifested because of this bug is that text nodes could get smaller and smaller on each layout pass. For details see facebook#824.

Fixes facebook#824

Adam Comella
Microsoft Corp.
facebook-github-bot pushed a commit to facebook/litho that referenced this issue Oct 12, 2018
Summary:
`YGRoundValueToPixelGrid` currently rounds negative numbers incorrectly. For example:

```
YGRoundValueToPixelGrid(-2.2, 1.0, /* ceil */ false, /* floor */ true) = -2.0
```

However, that operation is supposed to take the floor of the number so the result should acutally be `-3.0`.

There's a detailed comment in `YGRoundValueToPixelGrid` about the fix and why it works.

A symptom that manifested because of this bug is that text nodes could get smaller and smaller on each layout pass. For details see facebook/yoga#824.

Fixes #824

Adam Comella
Microsoft Corp.
Pull Request resolved: facebook/yoga#825

Reviewed By: priteshrnandgaonkar

Differential Revision: D10282064

Pulled By: shergin

fbshipit-source-id: 16ca966e6cb0cfc88b1dbf4ba31e7b1dbe1f2049
facebook-github-bot pushed a commit to facebook/react-native that referenced this issue Oct 12, 2018
Summary:
`YGRoundValueToPixelGrid` currently rounds negative numbers incorrectly. For example:

```
YGRoundValueToPixelGrid(-2.2, 1.0, /* ceil */ false, /* floor */ true) = -2.0
```

However, that operation is supposed to take the floor of the number so the result should acutally be `-3.0`.

There's a detailed comment in `YGRoundValueToPixelGrid` about the fix and why it works.

A symptom that manifested because of this bug is that text nodes could get smaller and smaller on each layout pass. For details see facebook/yoga#824.

Fixes #824

Adam Comella
Microsoft Corp.
Pull Request resolved: facebook/yoga#825

Reviewed By: priteshrnandgaonkar

Differential Revision: D10282064

Pulled By: shergin

fbshipit-source-id: 16ca966e6cb0cfc88b1dbf4ba31e7b1dbe1f2049
t-nanava pushed a commit to microsoft/react-native-macos that referenced this issue Jun 17, 2019
Summary:
`YGRoundValueToPixelGrid` currently rounds negative numbers incorrectly. For example:

```
YGRoundValueToPixelGrid(-2.2, 1.0, /* ceil */ false, /* floor */ true) = -2.0
```

However, that operation is supposed to take the floor of the number so the result should acutally be `-3.0`.

There's a detailed comment in `YGRoundValueToPixelGrid` about the fix and why it works.

A symptom that manifested because of this bug is that text nodes could get smaller and smaller on each layout pass. For details see facebook/yoga#824.

Fixes #824

Adam Comella
Microsoft Corp.
Pull Request resolved: facebook/yoga#825

Reviewed By: priteshrnandgaonkar

Differential Revision: D10282064

Pulled By: shergin

fbshipit-source-id: 16ca966e6cb0cfc88b1dbf4ba31e7b1dbe1f2049
CodeWitchBella pushed a commit to CodeWitchBella/yoga-wasm that referenced this issue Sep 2, 2019
Summary:
`YGRoundValueToPixelGrid` currently rounds negative numbers incorrectly. For example:

```
YGRoundValueToPixelGrid(-2.2, 1.0, /* ceil */ false, /* floor */ true) = -2.0
```

However, that operation is supposed to take the floor of the number so the result should acutally be `-3.0`.

There's a detailed comment in `YGRoundValueToPixelGrid` about the fix and why it works.

A symptom that manifested because of this bug is that text nodes could get smaller and smaller on each layout pass. For details see facebook/yoga#824.

Fixes #824

Adam Comella
Microsoft Corp.
Pull Request resolved: facebook/yoga#825

Reviewed By: priteshrnandgaonkar

Differential Revision: D10282064

Pulled By: shergin

fbshipit-source-id: 16ca966e6cb0cfc88b1dbf4ba31e7b1dbe1f2049
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant