Fix traversal errors in ElementListIterator::Retreat. [chromium/src : main]

[David Baron (Gerrit)]

Attention needed from Joey Arhar

New activity on the change

Open in Gerrit

Related details

Attention is currently required from:

• Joey Arhar

Submit Requirements:

• Code-Coverage
• Code-Owners
• Code-Review
• Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: chromium/src

Gerrit-Branch: main

Gerrit-Change-Id: I797409bbb4e25dac172b8dc9770b0598840958c1

Gerrit-Change-Number: 7997300

Gerrit-PatchSet: 2

Gerrit-Owner: David Baron <dba...@chromium.org>

Gerrit-Reviewer: David Baron <dba...@chromium.org>

Gerrit-Reviewer: Joey Arhar <jar...@chromium.org>

Gerrit-CC: (Julie)Jeongeun Kim <je_jul...@chromium.org>

Gerrit-CC: Akihiro Ota <akihi...@chromium.org>

Gerrit-CC: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-CC: Kentaro Hara <har...@chromium.org>

Gerrit-CC: Kevin Babbitt <kbab...@microsoft.com>

Gerrit-CC: Menard, Alexis <alexis...@intel.com>

Gerrit-CC: Nektarios Paisios <nek...@chromium.org>

Gerrit-CC: Raphael Kubo da Costa <ku...@igalia.com>

Gerrit-CC: chromium...@chromium.org

Gerrit-Attention: Joey Arhar <jar...@chromium.org>

Gerrit-Comment-Date: Thu, 25 Jun 2026 15:44:00 +0000

Gerrit-HasComments: No

Gerrit-Has-Labels: No

[David Baron (Gerrit)]

Attention needed from Joey Arhar

David Baron added 1 comment

Commit Message

Line 45, Patchset 2 (Latest):Bug: 406566432

David Baron . unresolved

Is there a `<select>`-related bug number I should add here?

Open in Gerrit

Related details

Attention is currently required from:

• Joey Arhar

Submit Requirements:

• Code-Coverage
• Code-Owners
• Code-Review

• No-Unresolved-Comments

• Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: chromium/src

Gerrit-Branch: main

Gerrit-Change-Id: I797409bbb4e25dac172b8dc9770b0598840958c1

Gerrit-Change-Number: 7997300

Gerrit-PatchSet: 2

Gerrit-Owner: David Baron <dba...@chromium.org>

Gerrit-Reviewer: David Baron <dba...@chromium.org>

Gerrit-Reviewer: Joey Arhar <jar...@chromium.org>

Gerrit-CC: (Julie)Jeongeun Kim <je_jul...@chromium.org>

Gerrit-CC: Akihiro Ota <akihi...@chromium.org>

Gerrit-CC: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-CC: Kentaro Hara <har...@chromium.org>

Gerrit-CC: Kevin Babbitt <kbab...@microsoft.com>

Gerrit-CC: Menard, Alexis <alexis...@intel.com>

Gerrit-CC: Nektarios Paisios <nek...@chromium.org>

Gerrit-CC: Raphael Kubo da Costa <ku...@igalia.com>

Gerrit-CC: chromium...@chromium.org

Gerrit-Attention: Joey Arhar <jar...@chromium.org>

Gerrit-Comment-Date: Thu, 25 Jun 2026 15:46:39 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No

[Joey Arhar (Gerrit)]

Attention needed from David Baron

Joey Arhar voted and added 1 comment

Votes added by Joey Arhar

Code-Review

+1

1 comment

Commit Message

Line 45, Patchset 2 (Latest):Bug: 406566432

David Baron . resolved

Is there a `<select>`-related bug number I should add here?

Joey Arhar

nah i think its fine to just use the menu elements tracking bug

Open in Gerrit

Related details

Attention is currently required from:

• David Baron

Submit Requirements:

• Code-Coverage
• Code-Owners

• Code-Review
• Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: chromium/src

Gerrit-Branch: main

Gerrit-Change-Id: I797409bbb4e25dac172b8dc9770b0598840958c1

Gerrit-Change-Number: 7997300

Gerrit-PatchSet: 2

Gerrit-Owner: David Baron <dba...@chromium.org>

Gerrit-Reviewer: David Baron <dba...@chromium.org>

Gerrit-Reviewer: Joey Arhar <jar...@chromium.org>

Gerrit-CC: (Julie)Jeongeun Kim <je_jul...@chromium.org>

Gerrit-CC: Akihiro Ota <akihi...@chromium.org>

Gerrit-CC: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-CC: Kentaro Hara <har...@chromium.org>

Gerrit-CC: Kevin Babbitt <kbab...@microsoft.com>

Gerrit-CC: Menard, Alexis <alexis...@intel.com>

Gerrit-CC: Nektarios Paisios <nek...@chromium.org>

Gerrit-CC: Raphael Kubo da Costa <ku...@igalia.com>

Gerrit-CC: chromium...@chromium.org

Gerrit-Attention: David Baron <dba...@chromium.org>

Gerrit-Comment-Date: Thu, 25 Jun 2026 17:55:36 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: Yes

Comment-In-Reply-To: David Baron <dba...@chromium.org>

[David Baron (Gerrit)]

David Baron voted Commit-Queue+2

Commit-Queue

+2

Open in Gerrit

Related details

Attention set is empty

Gerrit-Comment-Date: Thu, 25 Jun 2026 17:57:37 +0000

Gerrit-HasComments: No

Gerrit-Has-Labels: Yes

[Chromium LUCI CQ (Gerrit)]

Chromium LUCI CQ submitted the change

Change information

Commit message:

Fix traversal errors in ElementListIterator::Retreat.

This fixes multiple traversal errors in ElementListIterator::Retreat,
which is used when iterating backwards:

* Prior to this change, multiple places in the function used
  ElementTraversal::Previous.  This was incorrect because (given that we
  are doing the reverse of depth-first pre-order traversal)
  ElementTraversal::Previous can take multiple steps from parent to
  child in a single call, since when it moves to a previous sibling it
  wants to start with the last child's last child's last child (all the
  way down).  In this code this is incorrect because each step needs to
  call owner_.ShouldIgnoreDescendantsForElementTraversals.  This is
  tested by the added tests OptionListTest.RetreatExcluded and
  OptionListTest.RetreatExcludedAtEnd.

* Additionally, the use of ElementTraversal::PreviousAbsoluteSibling was
  incorrect, because, in the case where it needs to traverse from child
  to parent, it goes to the parent's next sibling *without* entering
  that sibling's descendants.  This is tested by the added test
  OptionListTest.RetreatNesting2.

* The condition for when to call PreviousAbsoluteSibling was also
  incorrect since it was checking whether to traverse descendants of the
  element that (when going backwards) has already had its descendants
  traversed, not the one(s) that we should be considering.  This is
  tested by the added test OptionListTest.RetreatNesting3.

The first of these bugs is tested extensively by the work-in-progress
change to update the structure of menu elements to use DOM nesting.  The
second and third were found only by code inspection while rewriting the
code to fix the first.

This also updates the DCHECK_EQ() to fire much closer to the bug occurs:
we check the ownership when we set current_ rather than when we traverse
away from it, and we check it in both directions rather than only when
iterating backwards.

Bug: 406566432

Change-Id: I797409bbb4e25dac172b8dc9770b0598840958c1

Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7997300

Commit-Queue: David Baron <dba...@chromium.org>

Reviewed-by: Joey Arhar <jar...@chromium.org>

Cr-Commit-Position: refs/heads/main@{#1652556}

Files:

• M third_party/blink/renderer/core/html/forms/option_list.cc
• M third_party/blink/renderer/core/html/forms/option_list_test.cc

Change size: M

Delta: 2 files changed, 211 insertions(+), 18 deletions(-)

Branch: refs/heads/main

Submit Requirements:

• Code-Review: +1 by Joey Arhar

Open in Gerrit

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: merged

Gerrit-Project: chromium/src

Gerrit-Branch: main

Gerrit-Change-Id: I797409bbb4e25dac172b8dc9770b0598840958c1

Gerrit-Change-Number: 7997300

Gerrit-PatchSet: 3

Gerrit-Owner: David Baron <dba...@chromium.org>

Gerrit-Reviewer: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Reviewer: David Baron <dba...@chromium.org>

Gerrit-Reviewer: Joey Arhar <jar...@chromium.org>

Gerrit-CC: (Julie)Jeongeun Kim <je_jul...@chromium.org>

Gerrit-CC: Akihiro Ota <akihi...@chromium.org>