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

[LayoutNG] Text indent with leading float #20096

Merged
merged 1 commit into from Feb 14, 2020

Conversation

chromium-wpt-export-bot
Copy link
Collaborator

@chromium-wpt-export-bot chromium-wpt-export-bot commented Nov 5, 2019

This change avoids breaking after leading floats in the case where
there is a text indentation.

Doing so fixes the following scenario:

<div style="width: 100px; text-indent: 40px;">
<div style="float: left; width: 60px; height: 10px;"></div>
<div style="display: inline-block; width: 60px; height: 20px;"></div>
</div>

In the above example, the following steps will happen:

  1. The inline div will attempt to fit on the same line as the float.
    It does not, and a new line opportunity will be created on the next
    line.
  2. The inline div overflows this new line opportunity due to the indent.
  3. HandleOverflow() checks if the previous element (ie. the float)
    can break after. The float can, so it will attempt to rewind.
  4. This causes a break in a later DCHECK.

Not allowing breaks after leading floats in the case of an indentation
fixes the above issue, and appears to match the behavior of other
browsers.

Bug: 1014247
Change-Id: Icf551f5908a0fcb8e0b80a8b9329c965435dd805
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1896509
Reviewed-by: Koji Ishii <kojii@chromium.org>
Commit-Queue: Alison Maher <almaher@microsoft.com>
Cr-Commit-Position: refs/heads/master@{#741487}

Copy link
Collaborator

@wpt-pr-bot wpt-pr-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The review process for this patch is being conducted in the Chromium project.

@chromium-wpt-export-bot chromium-wpt-export-bot force-pushed the chromium-export-cl-1896509 branch 2 times, most recently from d0b38e4 to 39f9a9b Compare November 7, 2019 23:31
@chromium-wpt-export-bot chromium-wpt-export-bot force-pushed the chromium-export-cl-1896509 branch 2 times, most recently from 8c83387 to 717e26f Compare November 21, 2019 22:45
@gsnedders gsnedders closed this Jan 24, 2020
@gsnedders gsnedders deleted the chromium-export-cl-1896509 branch January 24, 2020 18:04
@gsnedders gsnedders restored the chromium-export-cl-1896509 branch January 24, 2020 18:47
@Hexcles Hexcles reopened this Jan 24, 2020
@chromium-wpt-export-bot chromium-wpt-export-bot changed the title [LayoutNG] Fix not to break after leading floats [LayoutNG] Text indent with leading float Feb 5, 2020
This change avoids breaking after leading floats in the case where
there is a text indentation.

Doing so fixes the following scenario:

<div style="width: 100px; text-indent: 40px;">
  <div style="float: left; width: 60px; height: 10px;"></div>
  <div style="display: inline-block; width: 60px; height: 20px;"></div>
</div>

In the above example, the following steps will happen:

1. The inline div will attempt to fit on the same line as the float.
   It does not, and a new line opportunity will be created on the next
   line.
2. The inline div overflows this new line opportunity due to the indent.
3. HandleOverflow() checks if the previous element (ie. the float)
   can break after. The float can, so it will attempt to rewind.
4. This causes a break in a later DCHECK.

Not allowing breaks after leading floats in the case of an indentation
fixes the above issue, and appears to match the behavior of other
browsers.

Bug: 1014247
Change-Id: Icf551f5908a0fcb8e0b80a8b9329c965435dd805
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1896509
Reviewed-by: Koji Ishii <kojii@chromium.org>
Commit-Queue: Alison Maher <almaher@microsoft.com>
Cr-Commit-Position: refs/heads/master@{#741487}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants