Conversation
Review NotesOverall the approach is sound — using keyboard navigation to work around shadow DOM complexity is a reasonable workaround. A couple of things to address: Unused import: After removing search_engine_dropdown(), the Dropdown import on line 16 of page_object_prefs.py is now dead code. Ruff will flag this. Linting not done: The PR checklist notes linting/formatting is unchecked — per project conventions this must pass before merging. Running 'ruff check .' will catch the unused import above. Algorithm assumption: select_default_search_engine_by_key assumes the dropdown always starts focused at the topmost option when opened. If Firefox focuses the currently selected item instead, pressing DOWN i times won't consistently land on the expected index — the loop might exit early on the wrong iteration or miss the target. Worth verifying this assumption holds for all dropdown states (e.g. when DuckDuckGo is already selected before the method runs). |
|
Good cleanup overall -- replacing the shadow DOM Dropdown interaction with keyboard navigation is a solid approach, and the test stabilizations look reasonable. One bug flagged inline; two minor nits: (1) missing return type annotation on legacy_search_engine_matches, (2) linting is unchecked in the PR checklist -- the docstring rewraps look fine but worth verifying with ruff format before merge. |
Relevant Links
Bugzilla: Link
Description of Code / Doc Changes
Process Changes Required
Mark the relevant boxes, delete irrelevant lines.
Screenshots or Explanations
The shadow DOM in the new search engine dropdown is more difficult to work with than the previous, so using the keyboard is preferred
Comments or Future Work
Workflow Checklist
Thank you!