Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2815003006:
[LayoutNG] Handle floats/OOF at the beginning of inline in inline context (Closed)
|
Created:
3 years, 8 months ago by kojii Modified:
3 years, 8 months ago CC:
atotic+reviews_chromium.org, blink-reviews, blink-reviews-layout_chromium.org, cbiesinger, chromium-reviews, dgrogan+ng_chromium.org, eae+blinkwatch, glebl+reviews_chromium.org, jchaffraix+rendering, leviw+renderwatch, ojan+watch_chromium.org, pdr+renderingwatchlist_chromium.org, szager+layoutwatch_chromium.org, zoltan1 Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[LayoutNG] Handle floats/OOF at the beginning of inline in inline context
Discussion doc:
https://docs.google.com/a/chromium.org/document/d/1t3NOXd-0edfe6IG8-sBsrG9mICnOImSVQzuYRxw-nGM/edit?usp=sharing
BUG=636993
Review-Url: https://codereview.chromium.org/2815003006
Cr-Commit-Position: refs/heads/master@{#465509}
Committed: https://chromium.googlesource.com/chromium/src/+/a9545a8ace162fa58176e88d0f471f1ac2626186
Patch Set 1 #Patch Set 2 : Handle floats only as inline too, add tests #Patch Set 3 : Handle floats only as block, add more tests #Patch Set 4 : TestExpectations #Patch Set 5 : Resolve merge conflict #
Messages
Total messages: 59 (38 generated)
Patchset #3 (id:40001) has been deleted
Description was changed from ========== ngoofctx BUG= ========== to ========== [LayoutNG] Handle floats/OOF at the beginning of inline in inline context BUG=636993 ==========
Description was changed from ========== [LayoutNG] Handle floats/OOF at the beginning of inline in inline context BUG=636993 ========== to ========== [LayoutNG] Handle floats/OOF at the beginning of inline in inline context Discussion doc: https://docs.google.com/a/chromium.org/document/d/1t3NOXd-0edfe6IG8-sBsrG9mIC... BUG=636993 ==========
The CQ bit was checked by kojii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #3 (id:60001) has been deleted
The CQ bit was checked by kojii@chromium.org to run a CQ dry run
Patchset #3 (id:80001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
kojii@chromium.org changed reviewers: + atotic@chromium.org, eae@chromium.org, glebl@chromium.org, ikilpatrick@chromium.org
The one I stated in the e-mail discussion. PS1/PS3 uses block algorithm if it has only float/oof children. PS2 uses inline for that case. Currently inline skips some postprocess for empty inlines, so that's probably the primary reasons for the failures of PS2.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Description was changed from ========== [LayoutNG] Handle floats/OOF at the beginning of inline in inline context Discussion doc: https://docs.google.com/a/chromium.org/document/d/1t3NOXd-0edfe6IG8-sBsrG9mIC... BUG=636993 ========== to ========== [LayoutNG] Handle floats/OOF at the beginning of inline in inline context Discussion doc: https://docs.google.com/a/chromium.org/document/d/1t3NOXd-0edfe6IG8-sBsrG9mIC... BUG=636993 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng ==========
The CQ bit was checked by kojii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
One new test failure floats-wrap-top-below-inline-003r.htm actually turned to pass, but we fail to render -expected correctly due to lack of inline-margin support.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_layout_tests_layout_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)
Description was changed from ========== [LayoutNG] Handle floats/OOF at the beginning of inline in inline context Discussion doc: https://docs.google.com/a/chromium.org/document/d/1t3NOXd-0edfe6IG8-sBsrG9mIC... BUG=636993 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng ========== to ========== [LayoutNG] Handle floats/OOF at the beginning of inline in inline context Discussion doc: https://docs.google.com/a/chromium.org/document/d/1t3NOXd-0edfe6IG8-sBsrG9mIC... The new test failure floats-wrap-top-below-inline-003r.htm is because this patch fixes the test, which uses floats, but does not fix the -expected.htm which uses inline margins, and thus images do not match. BUG=636993 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng ==========
LGTM
Ian told me that maybe we can rely on LayoutObject::ChlidrenInline() for this
purpose. I tried, but 6 tests failed:
6 tests failed:
NGBlockLayoutAlgorithmTest.CollapsingMarginsCase7
NGBlockLayoutAlgorithmTest.PositionFloatInsideEmptyBlocks
NGBlockNodeForTest.ChildFloatOnly
NGBlockNodeForTest.ChildFloatWithSpaces
NGOutOfFlowLayoutPartTest.OrthogonalWritingMode1
NGOutOfFlowLayoutPartTest.OrthogonalWritingMode2
From "NGBlockNodeForTest.ChildFloatOnly" failing, we probably make a box only
with floats as inline in legacy. That looks reasonable to me but we were not so
sure yesterday, and this CL doesn't do so yet (has TODO.)
Ian, are you ok to land this as is as a first step, and I'll task on myself for
further investigation how LayoutObject::ChildrenInline() differs and why these
tests fail?
ok, that sounds good, lgtm.
The CQ bit was checked by kojii@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_layout_tests_layout_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)
The CQ bit was checked by kojii@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_layout_tests_layout_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)
The CQ bit was checked by kojii@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_layout_tests_layout_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)
The CQ bit was checked by kojii@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org, ikilpatrick@chromium.org Link to the patchset: https://codereview.chromium.org/2815003006/#ps140001 (title: "Resolve merge conflict")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by kojii@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [LayoutNG] Handle floats/OOF at the beginning of inline in inline context Discussion doc: https://docs.google.com/a/chromium.org/document/d/1t3NOXd-0edfe6IG8-sBsrG9mIC... The new test failure floats-wrap-top-below-inline-003r.htm is because this patch fixes the test, which uses floats, but does not fix the -expected.htm which uses inline margins, and thus images do not match. BUG=636993 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng ========== to ========== [LayoutNG] Handle floats/OOF at the beginning of inline in inline context Discussion doc: https://docs.google.com/a/chromium.org/document/d/1t3NOXd-0edfe6IG8-sBsrG9mIC... BUG=636993 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng ==========
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_layout_tests_layout_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)
The CQ bit was checked by kojii@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_layout_tests_layout_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)
The CQ bit was checked by kojii@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_layout_tests_layout_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)
Description was changed from ========== [LayoutNG] Handle floats/OOF at the beginning of inline in inline context Discussion doc: https://docs.google.com/a/chromium.org/document/d/1t3NOXd-0edfe6IG8-sBsrG9mIC... BUG=636993 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng ========== to ========== [LayoutNG] Handle floats/OOF at the beginning of inline in inline context Discussion doc: https://docs.google.com/a/chromium.org/document/d/1t3NOXd-0edfe6IG8-sBsrG9mIC... BUG=636993 ==========
The CQ bit was checked by kojii@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1492582377293570,
"parent_rev": "0d59c0629864712c49a732ddb1af18cafce314e7", "commit_rev":
"a9545a8ace162fa58176e88d0f471f1ac2626186"}
Message was sent while issue was closed.
Description was changed from ========== [LayoutNG] Handle floats/OOF at the beginning of inline in inline context Discussion doc: https://docs.google.com/a/chromium.org/document/d/1t3NOXd-0edfe6IG8-sBsrG9mIC... BUG=636993 ========== to ========== [LayoutNG] Handle floats/OOF at the beginning of inline in inline context Discussion doc: https://docs.google.com/a/chromium.org/document/d/1t3NOXd-0edfe6IG8-sBsrG9mIC... BUG=636993 Review-Url: https://codereview.chromium.org/2815003006 Cr-Commit-Position: refs/heads/master@{#465509} Committed: https://chromium.googlesource.com/chromium/src/+/a9545a8ace162fa58176e88d0f47... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:140001) as https://chromium.googlesource.com/chromium/src/+/a9545a8ace162fa58176e88d0f47... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
