Make arrow focus changes for menuitems loop. [chromium/src : main]

0 views

Skip to first unread message

David Baron (Gerrit)

unread,

Jan 9, 2026, 3:42:05 PM (3 days ago) Jan 9

to David Baron, Mason Freed, AyeAye, chromium...@chromium.org, blink-revie...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org

Attention needed from Mason Freed

David Baron added 2 comments

Commit Message

Line 9, Patchset 3 (Latest):This makes right/left arrows on the menubar and up/down arrows on the

David Baron . unresolved

Does this seem like the right behavior?

File third_party/blink/renderer/core/html/menu_item_list.cc

Line 177, Patchset 2: // Directional focus in menu item lists always loops, whether in a menubar

David Baron . unresolved

This seems like one of those cases where there's probably a clever way to make this code 5-10 lines shorter, but I don't currently see it...

Open in Gerrit

Related details

Attention is currently required from:

Mason Freed

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

satisfied_requirement

unsatisfied_requirement

open

diffy

Mason Freed (Gerrit)

unread,

1:24 PM (5 hours ago) 1:24 PM

to David Baron, AyeAye, chromium...@chromium.org, blink-revie...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org

Attention needed from David Baron

Mason Freed voted and added 5 comments

Votes added by Mason Freed

Code-Review

5 comments

Patchset-level comments

File-level comment, Patchset 5 (Latest):

Mason Freed . resolved

LGTM!

Commit Message

Line 9, Patchset 3:This makes right/left arrows on the menubar and up/down arrows on the

David Baron . unresolved

Does this seem like the right behavior?

Mason Freed

Quick note that this was removed from the commit description - likely good to put it back.

Now to your question. I do think looping sounds like the right behavior, I think that's what Erin's UX doc concluded (correct me if I'm wrong). For a11y, it does mean we need to expose posinset and setsize, so that a user knows "when" they've looped back around.

File third_party/blink/renderer/core/html/menu_item_list.cc

Line 180, Patchset 5 (Latest):

      if (forward) {
        menu_item_list_iterator = begin();
      } else {
        menu_item_list_iterator = last();
      }

Mason Freed . unresolved

This could be ternary, saving 4 lines.

Line 187, Patchset 5 (Latest): HTMLMenuItemElement* this_item = &*menu_item_list_iterator;

Mason Freed . unresolved

This seems like perhaps you should add a (non-explicit) `operator HTMLMenuItemElement*()` for `MenuItemListIterator`?

Line 189, Patchset 5 (Latest):

    if (first_item_tested) {
      if (this_item == first_item_tested) {
        // We've tested all the items and none is focusable.
        return nullptr;
      }
    } else {
      first_item_tested = this_item;
    }
    if (this_item->IsFocusable()) {
      return this_item;
    }

Mason Freed . unresolved

Not much different, so take it or leave it, but:

```
if (item == first_item_tested) {
  return nullptr;
}
if (!first_item_tested) {
  first_item_tested = item;
}
if (item->IsFocusable()) {
  return item;
}
```

Open in Gerrit

Related details

Attention is currently required from:

David Baron

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

satisfied_requirement

unsatisfied_requirement

open

diffy

Reply all

Reply to author

Forward

0 new messages