Fix incorrect tree construction by the AAA [chromium/src : main]

Keiichiro Nagashima (Gerrit)

unread,

Sep 15, 2021, 11:27:16 AM9/15/21

to Mason Freed, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, loading-rev...@chromium.org

Attention is currently required from: Mason Freed.

Keiichiro Nagashima would like Mason Freed to review this change.

View Change

Fix incorrect tree construction by the AAA

Prior to this CL, the following code:
  <code some-attribute><div><code><code><code><code></code></code></code></code>
parsed to this:
  <code some-attribute></code>
  <div><code some-attribute><code><code><code><code></code></code></code></code></code></div>

The adoption agency algorithm reparented the div tag though code tags
were properly nested. A step was added to the spec[1] in order to fix
this, but not implemented in Chromium. This CL implements it.

[1] https://www.w3.org/Bugs/Public/show_bug.cgi?id=24833

Bug: 1217523
Change-Id: I2ce1ada69d37305041468b9c10f59e6238e9a209
---
M AUTHORS
M third_party/blink/renderer/core/html/parser/html_tree_builder.cc
A third_party/blink/web_tests/external/wpt/html/syntax/parsing/adoption_agency_check_the_end_tag_name.html
3 files changed, 28 insertions(+), 0 deletions(-)

To view, visit change 3162497. To unsubscribe, or for help writing mail filters, visit settings.

Keiichiro Nagashima (Gerrit)

unread,

Sep 15, 2021, 11:27:26 AM9/15/21

to blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, loading-rev...@chromium.org, Mason Freed, chromium...@chromium.org

Attention is currently required from: Mason Freed.

View Change

1 comment:

Patchset:
- Patch Set #1:
  Hi! Could you review this patch?
  I think you suggested the fix in [1].
  (I'm not 100% sure because only first three letters are displayed as "mas...@chromium.org")
  [1] https://bugs.chromium.org/p/chromium/issues/detail?id=1217523
  And if it's OK, could you run automated tests for me?
  I have never contributed a patch, so I don't have an access.
  Thanks!

To view, visit change 3162497. To unsubscribe, or for help writing mail filters, visit settings.

Blink WPT Bot (Gerrit)

unread,

Sep 15, 2021, 11:37:46 AM9/15/21

to Keiichiro Nagashima, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, loading-rev...@chromium.org, Mason Freed, chromium...@chromium.org

Attention is currently required from: Mason Freed.

Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/web-platform-tests/wpt/pull/30804.

When this CL lands, the bot will automatically merge the PR on GitHub if the required GitHub checks pass; otherwise, ecosystem-infra@ team will triage the failures and may contact you.

WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md#Automatic-export-process

View Change

To view, visit change 3162497. To unsubscribe, or for help writing mail filters, visit settings.

Mason Freed (Gerrit)

unread,

Sep 15, 2021, 12:21:03 PM9/15/21

to Keiichiro Nagashima, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, loading-rev...@chromium.org, Chromium LUCI CQ, Blink WPT Bot, chromium...@chromium.org

Attention is currently required from: Keiichiro Nagashima.

View Change

1 comment:

Patchset:
- Patch Set #1:
  Hi! Could you review this patch? […]
  Yep, that's me! I'd be happy to review. And I just kicked off the tests.

To view, visit change 3162497. To unsubscribe, or for help writing mail filters, visit settings.

Mason Freed (Gerrit)

unread,

Sep 15, 2021, 12:34:16 PM9/15/21

to Keiichiro Nagashima, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, loading-rev...@chromium.org, Chromium LUCI CQ, Blink WPT Bot, chromium...@chromium.org

Attention is currently required from: Keiichiro Nagashima.

View Change

6 comments:

Commit Message:
- Patch Set #1, Line 10:

  <code some-attribute><div><code><code><code><code></code></code></code></code>
parsed to this:
  <code some-attribute></code>
  <div><code some-attribute><code><code><code><code></code></code></code></code></code></div>

- It would be nice to format this a bit - you can use more lines and indent things if you like.
Patchset:
- Patch Set #1:
  Looks pretty good! Thanks for fixing this bug. Just a few relatively small comments, and then I think this is good to land.
File third_party/blink/renderer/core/html/parser/html_tree_builder.cc:
- Patch Set #1, Line 1567: http://www.whatwg.org/specs/web-apps/current-work/multipage/tree-construction.html#parsing-main-inbody
  I know this is an old comment, but while you're here, can you please correct this link to:
```
  https://html.spec.whatwg.org/multipage/parsing.html#adoption-agency-algorithm
```
- Patch Set #1, Line 1570:
  Let's add a comment here referring to the new spec:
```
  // 2. If the current node is an HTML element whose tag name is subject,
  // and the current node is not in the list of active formatting elements,
  // then pop the current node off the stack of open elements and return.
```
- Patch Set #1, Line 1571: tree_.CurrentElement();
  Do we need to check that !tree_.IsEmpty() before trying to retrieve the CurrentElement()? If so, you can do that in one line:
```
  if (!tree_.IsEmpty() &&
      tree_.CurrentElement()->HasLocalName(token->GetName()) &&
      !tree_.ActiveFormattingElements()->Contains(current_element)) {
```
File third_party/blink/web_tests/external/wpt/html/syntax/parsing/adoption_agency_check_the_end_tag_name.html:
- Patch Set #1:
  Thanks for adding a test!

To view, visit change 3162497. To unsubscribe, or for help writing mail filters, visit settings.

Mason Freed (Gerrit)

unread,

Sep 15, 2021, 12:35:18 PM9/15/21

to Keiichiro Nagashima, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, loading-rev...@chromium.org, Chromium LUCI CQ, Blink WPT Bot, chromium...@chromium.org

Attention is currently required from: Keiichiro Nagashima.

View Change

1 comment:

Commit Message:
- Patch Set #1, Line 21: Bug
  tip: if you change this to "Fixed", the bug will get automatically closed when this CL lands. E.g.
```
  Fixed: 1217523
```

To view, visit change 3162497. To unsubscribe, or for help writing mail filters, visit settings.

Keiichiro Nagashima (Gerrit)

unread,

Sep 15, 2021, 3:40:42 PM9/15/21

to blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, loading-rev...@chromium.org

Attention is currently required from: Keiichiro Nagashima.

Keiichiro Nagashima uploaded patch set #3 to this change.

View Change

Fix incorrect tree construction by the AAA

Prior to this CL, the following code:

  <code some-attribute>
    <div>
      <code>
        <code>
          <code>
            <code>
            </code>
          </code>
        </code>
      </code>
parsed to this:
  <code some-attribute></code>
  <div>
    <code some-attribute>
      <code>
        <code>
          <code>
            <code>
            </code>
          </code>
        </code>
      </code>
    </code>
  </div>

The adoption agency algorithm reparented the div tag though code tags
were properly nested. A step was added to the spec[1] in order to fix
this, but not implemented in Chromium. This CL implements it.

[1] https://www.w3.org/Bugs/Public/show_bug.cgi?id=24833

Fixed: 1217523


Change-Id: I2ce1ada69d37305041468b9c10f59e6238e9a209
---
M AUTHORS
M third_party/blink/renderer/core/html/parser/html_tree_builder.cc
A third_party/blink/web_tests/external/wpt/html/syntax/parsing/adoption_agency_check_the_end_tag_name.html

3 files changed, 32 insertions(+), 1 deletion(-)

To view, visit change 3162497. To unsubscribe, or for help writing mail filters, visit settings.

Keiichiro Nagashima (Gerrit)

unread,

Sep 15, 2021, 3:56:53 PM9/15/21

to blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, loading-rev...@chromium.org, Chromium LUCI CQ, Mason Freed, Blink WPT Bot, chromium...@chromium.org

Attention is currently required from: Mason Freed.

View Change

5 comments:

Commit Message:
- Patch Set #1, Line 10:

  <code some-attribute><div><code><code><code><code></code></code></code></code>
parsed to this:
  <code some-attribute></code>
  <div><code some-attribute><code><code><code><code></code></code></code></code></code></div>

- It would be nice to format this a bit - you can use more lines and indent things if you like.
  Done

- Patch Set #1, Line 21: Bug

- tip: if you change this to "Fixed", the bug will get automatically closed when this CL lands. E.g. […]
  Done
File third_party/blink/renderer/core/html/parser/html_tree_builder.cc:

- Patch Set #1, Line 1567: http://www.whatwg.org/specs/web-apps/current-work/multipage/tree-construction.html#parsing-main-inbody

- I know this is an old comment, but while you're here, can you please correct this link to: […]
  Done

- Patch Set #1, Line 1570:

- Let's add a comment here referring to the new spec: […]
  Done

- Patch Set #1, Line 1571: tree_.CurrentElement();

- Do we need to check that !tree_. […]
  I think we need it, thanks!
  I still have a problem. It seems that some web tests[1] failed due to the lack of an emptiness check. I added the check and ran it locally, but it still failed..
  I'm going to take a look at it in detail.
  [1] https://ci.chromium.org/ui/p/chromium/builders/try/linux_layout_tests_composite_after_paint/54302/overview

To view, visit change 3162497. To unsubscribe, or for help writing mail filters, visit settings.

Mason Freed (Gerrit)

unread,

Sep 15, 2021, 7:05:46 PM9/15/21

to Keiichiro Nagashima, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, loading-rev...@chromium.org, Chromium LUCI CQ, Blink WPT Bot, chromium...@chromium.org

Attention is currently required from: Keiichiro Nagashima.

View Change

2 comments:

Patchset:
- Patch Set #3:
  Thanks for the changes! Let's wait to see if Patchset 3 passes. If not, see my other comment here.
File third_party/blink/renderer/core/html/parser/html_tree_builder.cc:
- Patch Set #3, Line 1574: ) &&
  If you're still getting failures on Patchset 3 (and I'm not sure you are), then you might need to add this here:
```
  && tree_.TopStackItem()->IsElementNode() &&
  
(or something similar)
```

To view, visit change 3162497. To unsubscribe, or for help writing mail filters, visit settings.

Keiichiro Nagashima (Gerrit)

unread,

Sep 16, 2021, 11:12:49 AM9/16/21

to blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, loading-rev...@chromium.org, Mason Freed, Chromium LUCI CQ, Blink WPT Bot, chromium...@chromium.org

Attention is currently required from: Mason Freed.

View Change

3 comments:

Patchset:
- Patch Set #4:
  Thank you for the comment!
  I think this patchset is going to pass.
File third_party/blink/renderer/core/html/parser/html_tree_builder.cc:
- Patch Set #1, Line 1571: tree_.CurrentElement();
  I think we need it, thanks! […]
  Done
File third_party/blink/renderer/core/html/parser/html_tree_builder.cc:

- Patch Set #3, Line 1574: ) &&

- If you're still getting failures on Patchset 3 (and I'm not sure you are), then you might need to ad […]
  Thank you for the suggestion!
  The test passed after I added IsElementNode(), yeah!
  I'm going to upload a patchset.

To view, visit change 3162497. To unsubscribe, or for help writing mail filters, visit settings.

Mason Freed (Gerrit)

unread,

Sep 16, 2021, 11:33:43 AM9/16/21

to Keiichiro Nagashima, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, loading-rev...@chromium.org, Chromium LUCI CQ, Blink WPT Bot, chromium...@chromium.org

Attention is currently required from: Keiichiro Nagashima.

Patch set 4:Commit-Queue +2

View Change

1 comment:

Patchset:
- Patch Set #4:
  Ok, this looks good! Thanks for the changes, and for working on this bug. I'll push the button to land it now.

To view, visit change 3162497. To unsubscribe, or for help writing mail filters, visit settings.

Mason Freed (Gerrit)

unread,

Sep 16, 2021, 11:34:18 AM9/16/21

to Keiichiro Nagashima, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, loading-rev...@chromium.org, Chromium LUCI CQ, Blink WPT Bot, chromium...@chromium.org

Attention is currently required from: Keiichiro Nagashima.

View Change

1 comment:

Patchset:
- Patch Set #4:
  Ok, this looks good! Thanks for the changes, and for working on this bug. […]
  ...also, congratulations on your first patch! 🎉

To view, visit change 3162497. To unsubscribe, or for help writing mail filters, visit settings.

Mason Freed (Gerrit)

unread,

Sep 16, 2021, 11:34:31 AM9/16/21

to Keiichiro Nagashima, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, loading-rev...@chromium.org, Chromium LUCI CQ, Blink WPT Bot, chromium...@chromium.org

Attention is currently required from: Keiichiro Nagashima.

Patch set 4:Code-Review +1

View Change

To view, visit change 3162497. To unsubscribe, or for help writing mail filters, visit settings.

Keiichiro Nagashima (Gerrit)

unread,

Sep 16, 2021, 11:49:29 AM9/16/21

to blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, loading-rev...@chromium.org, Mason Freed, Chromium LUCI CQ, Blink WPT Bot, chromium...@chromium.org

View Change

1 comment:

Patchset:
- Patch Set #4:
  ... […]
  Thank you for your kind support.
  I learned a lot and enjoyed it!

To view, visit change 3162497. To unsubscribe, or for help writing mail filters, visit settings.

Mason Freed (Gerrit)

unread,

Sep 16, 2021, 12:22:38 PM9/16/21

to Keiichiro Nagashima, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, loading-rev...@chromium.org, Chromium LUCI CQ, Blink WPT Bot, chromium...@chromium.org

Attention is currently required from: Keiichiro Nagashima.

View Change

1 comment:

Patchset:
- Patch Set #4:
  Thank you for your kind support. […]
  I'm glad! Come back for more! 😊

To view, visit change 3162497. To unsubscribe, or for help writing mail filters, visit settings.

Chromium LUCI CQ (Gerrit)

unread,

Sep 16, 2021, 1:17:36 PM9/16/21

to Keiichiro Nagashima, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, loading-rev...@chromium.org, Mason Freed, Blink WPT Bot, chromium...@chromium.org

Chromium LUCI CQ submitted this change.

View Change

Approvals:
  Mason Freed: Looks good to me; Commit

Fix incorrect tree construction by the AAA

Prior to this CL, the following code:

  <code some-attribute>
    <div>
      <code>
        <code>
          <code>
            <code>
            </code>
          </code>
        </code>
      </code>
parsed to this:
  <code some-attribute></code>
  <div>
    <code some-attribute>
      <code>
        <code>
          <code>
            <code>
            </code>
          </code>
        </code>
      </code>
    </code>
  </div>

The adoption agency algorithm reparented the div tag though code tags
were properly nested. A step was added to the spec[1] in order to fix
this, but not implemented in Chromium. This CL implements it.

[1] https://www.w3.org/Bugs/Public/show_bug.cgi?id=24833

Fixed: 1217523
Change-Id: I2ce1ada69d37305041468b9c10f59e6238e9a209

Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3162497
Commit-Queue: Mason Freed <mas...@chromium.org>
Reviewed-by: Mason Freed <mas...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#922189}


---
M AUTHORS
M third_party/blink/renderer/core/html/parser/html_tree_builder.cc
A third_party/blink/web_tests/external/wpt/html/syntax/parsing/adoption_agency_check_the_end_tag_name.html
3 files changed, 32 insertions(+), 1 deletion(-)

To view, visit change 3162497. To unsubscribe, or for help writing mail filters, visit settings.

Blink WPT Bot (Gerrit)

unread,

Sep 16, 2021, 1:43:22 PM9/16/21

to Keiichiro Nagashima, Chromium LUCI CQ, blink-rev...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, kinuko...@chromium.org, loading-rev...@chromium.org, Mason Freed, chromium...@chromium.org

The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/30804

View Change

To view, visit change 3162497. To unsubscribe, or for help writing mail filters, visit settings.

Reply all

Reply to author

Forward