New issue
Advanced search Search tips

Issue 762731 link

Starred by 1 user

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocking:
issue 758816



Sign in to add a comment

[LayoutNG] Box margins are not properly set

Project Member Reported by xiaoche...@chromium.org, Sep 6 2017

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.
 
Blocking: 758816
Owner: ikilpatrick@chromium.org
Status: Assigned (was: Available)
ikilpatrick@: Could you take a look? Or should we get the margin in some other way?
Cc: atotic@chromium.org
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.
This works as expected now, right?

Sign in to add a comment