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 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 left a comment

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 Nov 7, 2019
@chromium-wpt-export-bot chromium-wpt-export-bot force-pushed the chromium-export-cl-1896509 branch 2 times, most recently from 8c83387 to 717e26f Nov 21, 2019
@gsnedders gsnedders closed this Jan 24, 2020
@gsnedders gsnedders deleted the chromium-export-cl-1896509 branch Jan 24, 2020
@gsnedders gsnedders restored the chromium-export-cl-1896509 branch Jan 24, 2020
@Hexcles Hexcles reopened this Jan 24, 2020
@chromium-wpt-export-bot chromium-wpt-export-bot force-pushed the chromium-export-cl-1896509 branch from 717e26f to 9f31412 Feb 5, 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
@chromium-wpt-export-bot chromium-wpt-export-bot force-pushed the chromium-export-cl-1896509 branch from 9f31412 to e77b166 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}
@chromium-wpt-export-bot chromium-wpt-export-bot force-pushed the chromium-export-cl-1896509 branch from e77b166 to 75daa4b Feb 14, 2020
@chromium-wpt-export-bot chromium-wpt-export-bot merged commit 3c631e2 into master Feb 14, 2020
13 checks passed
13 checks passed
Azure Pipelines Build #20200214.42 succeeded
Details
Azure Pipelines (./wpt test-jobs) ./wpt test-jobs succeeded
Details
Azure Pipelines (affected tests without changes: Safari Technology Preview) affected tests without changes: Safari Technology Preview succeeded
Details
Azure Pipelines (affected tests: Safari Technology Preview) affected tests: Safari Technology Preview succeeded
Details
Azure Pipelines (wpt.fyi hook: safari-preview-affected-tests) wpt.fyi hook: safari-preview-affected-tests succeeded
Details
Azure Pipelines (wpt.fyi hook: safari-preview-affected-tests-without-changes) wpt.fyi hook: safari-preview-affected-tests-without-changes succeeded
Details
Community-TC (pull_request) TaskGroup: success
Details
staging.wpt.fyi - chrome[experimental] Chrome results
Details
staging.wpt.fyi - firefox[experimental] Firefox results
Details
staging.wpt.fyi - safari[experimental] Safari results
Details
wpt.fyi - chrome[experimental] Chrome results
Details
wpt.fyi - firefox[experimental] Firefox results
Details
wpt.fyi - safari[experimental] Safari results
Details
@chromium-wpt-export-bot chromium-wpt-export-bot deleted the chromium-export-cl-1896509 branch Feb 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.