[LayoutNG] Box margins are not properly set |
|||
Issue description
The following two unit tests differ only in the enabled status of LayoutNG, but the second one fails by reporting zero margin instead:
TEST_F(NGInlineLayoutTest, PWithoutLayoutNG) {
RuntimeEnabledFeatures::SetLayoutNGEnabled(false);
SimRequest main_resource("https://example.com/", "text/html");
LoadURL("https://example.com/");
main_resource.Complete(
"<p id=\"target\">foo bar baz</p>");
Compositor().BeginFrame();
ASSERT_FALSE(Compositor().NeedsBeginFrame());
Element* target = GetDocument().getElementById("target");
LayoutBox* box = ToLayoutBox(target->GetLayoutObject());
EXPECT_EQ(1.0, box->MarginTop().ToDouble());
EXPECT_EQ(1.0, box->MarginBottom().ToDouble());
EXPECT_EQ(0.0, box->MarginLeft().ToDouble());
EXPECT_EQ(0.0, box->MarginRight().ToDouble());
}
TEST_F(NGInlineLayoutTest, PWithLayoutNG) {
RuntimeEnabledFeatures::SetLayoutNGEnabled(true);
SimRequest main_resource("https://example.com/", "text/html");
LoadURL("https://example.com/");
main_resource.Complete(
"<p id=\"target\">foo bar baz</p>");
Compositor().BeginFrame();
ASSERT_FALSE(Compositor().NeedsBeginFrame());
Element* target = GetDocument().getElementById("target");
LayoutBox* box = ToLayoutBox(target->GetLayoutObject());
EXPECT_EQ(1.0, box->MarginTop().ToDouble());
EXPECT_EQ(1.0, box->MarginBottom().ToDouble());
EXPECT_EQ(0.0, box->MarginLeft().ToDouble());
EXPECT_EQ(0.0, box->MarginRight().ToDouble());
}
A deeper investigation shows that, the margin of <p> is never set in the second test.
,
Sep 6 2017
ikilpatrick@: Could you take a look? Or should we get the margin in some other way?
,
Sep 7 2017
Discussed offline with atotic@. Guess we've found the root cause. LayoutNG code only sets margin at two places: 1) LayoutNGBlockFlow::UpdateBlockLayout, which is called only for the outer-most NG blocks, and sets their margins 2) NGBlockNode::Layout, which is called for every NG block, but sets margins only for atomic inline blocks I think the fix should be: - Make 2) set margin for all blocks. - Remove 1) since it's now redundant 2) currently has a note saying "set this only for atomic inlines, or we end up adding margins twice", which, as we found, is already wrong as we can end up not adding margins at all. And I guess setting margins twice isn't a real problem. So I think the fix is just to remove the "atomic-inline-only" restriction.
,
Sep 7 2017
The attempt in #3 doesn't work... A lot of regressions. Attempt: https://crrev.com/c/655920 Try result: https://storage.googleapis.com/chromium-layout-test-archives/linux_chromium_rel_ng/541556/layout-test-results/test-expectations.html
,
Oct 26
This works as expected now, right? |
|||
►
Sign in to add a comment |
|||
Comment 1 by xiaoche...@chromium.org
, Sep 6 2017