Skip to content

Conversation

@Sangie50
Copy link
Collaborator

Relevant Links

Bugzilla: Bug 1976523
TestRail: C134460

Description of Code / Doc Changes

I have not merged the latest version of the main branch into this PR because it is currently failing its own tests (specifically, the geolocation and FxA tests). This branch is based on the last stable commit before those unrelated failures were introduced.

Once main is fixed, I can rebase or merge the latest changes into this PR if needed.


  • Creates a new automated test for opening a bookmark in a new tab (Bug 1976272).
  • Adds the correct element ID for the "Open in New Tab" context menu item to context_menu.components.json.
  • Implements an explicit wait (WebDriverWait) in the test to fix a race condition and ensure reliability.

Process Changes Required

Mark the relevant boxes:

  • Adds a dependency (rerun pipenv install)
  • Changes the BasePage
  • Changes or creates a BOM/POM (name the object model): context_menu.components.json
  • Changes CI flow
  • Changes scheduled Beta or DevEdition
  • Changes Git hooks or Github settings
  • Changes L10n harness

Screenshots or Explanations

N/A

Comments or Future Work

None

Workflow Checklist

  • Please request reviewers
  • If this is an unblocker, please post in Slack.
  • If asked to address comments, please resolve conversations.
  • If asked to change code, please re-request review from the person who wanted changes.

Thank you!

@Tracy-Walker Tracy-Walker self-requested a review August 21, 2025 13:22
Copy link
Collaborator

@Tracy-Walker Tracy-Walker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This picked up some l10n_CM testing stuff, which caused some failures and merge conflict. If you merge main again. Then address the failure in your test, on Mac, in checking for number of tabs. The fact your test passed on Windows and Linux indicates a possible race condition. I think it may be happening from line 34 and 35 of your tests. Add a wait in between those two lines. "tabs.wait_for_num_tabs(2)" to ensure the new tab, of line 34, is opened before the next test step.

Lastly, when you add a new selector, like you have in modules/data/context_menu.components.json, please add the appropriate entry into the SELECTOR_INFO.md (In this case, insert the new selector documentation after line 1776).

@Tracy-Walker
Copy link
Collaborator

This picked up some l10n_CM testing stuff, which caused some failures and merge conflict. If you merge main again. Then address the failure in your test, on Mac, in checking for number of tabs. The fact your test passed on Windows and Linux indicates a possible race condition. I think it may be happening from line 34 and 35 of your tests. Add a wait in between those two lines. "tabs.wait_for_num_tabs(2)" to ensure the new tab, of line 34, is opened before the next test step.

Nevermind my theory on there being a race condition. Don't implement that suggested fix. Instead see next review comment inline....

@Tracy-Walker
Copy link
Collaborator

Tracy-Walker commented Aug 25, 2025

Tried to merge main and push but git wanted to create a new PR. my git-fu is weak. :(. So we now have PR 755 for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants