Closed Bug 1121710 Opened 10 years ago Closed 5 years ago

Fix MenuBar class for correct handling of menus

Categories

(Testing :: Firefox UI Tests, defect)

Product:
Testing
Component:
Firefox UI Tests
Version:
Version 2
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED WONTFIX

People

(Reporter: whimboo, Unassigned, Mentored)

References

Details

(Whiteboard: [lang=py])

Attachments

(2 files)

Github PR (usage of ids)
53 bytes, text/x-github-pull-request
whimboo
: review+
Details | Review
Github PR (refactor and generalization)
53 bytes, text/x-github-pull-request
whimboo
: feedback+
Details | Review
To ensure that the menu class is working correctly a couple of fixes will be necessary: * Accessing menus should never work by using the label because this will be broken in localized builds of Firefox. Instead the id of the menu element has to be used. If no id is available another l10n independent property has to be found, which should be reachable via a Selector. * Support of multi-level menus is necessary to also reach elements in sub-levels >1. For all that it might be good to have a look at our current Mozmill Menu API implementation. Not all is perfect but we might be able to get a good chunk of logic and maybe code from there.
This is not a blocker for security tests in Q1.
No longer blocks: 1119715
The test_menubar.py unit test has been disabled due to failures for localized builds: https://github.com/mozilla/firefox-ui-tests/commit/356d9d9ac88c00dbf36fc56af2fecd87b0e9615b (master) I will backport it to all other branches with the other to backport changesets soon.
Backported the skip patch for the unit test to all the other branches as: https://github.com/mozilla/firefox-ui-tests/commit/8e8fcbcf9c680f77bd5bae9dea1f2ae06c6ffe38
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Similar to other modules we already have converted from JS (Mozmill) we can take maybe lots from Mozmill here. As reference its code: https://github.com/mozilla/mozmill/blob/master/mozmill/mozmill/extension/resource/driver/controller.js#L74 The menu class should be able to handle the main menu, and any kind of context menu.
QA Contact: hskupin
The attachment has a link to the Github pull request.
Assignee: nobody → tareqakhandaker
Attachment #8679489 - Flags: review+
Comment on attachment 8679489 [details] [review] Github PR (usage of ids) Small update... to ask for a review you want to set r? and use my id in the requestee field. I will have a look at it now.
Attachment #8679489 - Flags: review+ → review?(hskupin)
Comment on attachment 8679489 [details] [review] Github PR (usage of ids) I had a look at the PR and you definitely got it started the correct way. Having no hard-coded strings anymore is great. But there is way more to do for this bug to be complete. I will remove the r? request for now. Tareq, what we could do is to make the changes step by step. That way we do not end-up in massive PRs. How does that sound to you? If that is ok for you, lets update the current PR and make use of the menu class in all of the modules and tests which are using menus like https://github.com/mozilla/firefox-ui-tests/blob/mozilla-central/firefox_puppeteer/ui/browser/window.py#L115
Attachment #8679489 - Flags: review?(hskupin)
I am alright with going step by step. I updated the PR with the modules and tests that are using menu. I just did a grep for "menu" in the project and looked to see if making a change made sense. There were two files that I did not touch: 1) I think the opener here can be deleted since it uses the menu anyway: https://github.com/mozilla/firefox-ui-tests/blob/mozilla-central/firefox_puppeteer/tests/test_about_window.py#L64 2) This one is a button and not a menu from what I understand: https://github.com/mozilla/firefox-ui-tests/blob/mozilla-central/firefox_puppeteer/tests/test_places.py#L50
If those two files are alright as is, I will ask for r?
Flags: needinfo?(hskupin)
(In reply to Tareq Khandaker from comment #8) > I am alright with going step by step. I updated the PR with the modules and > tests that are using menu. I just did a grep for "menu" in the project and > looked to see if making a change made sense. Great! So lets try to get the current PR finished so we can move over to the next piece. > 1) I think the opener here can be deleted since it uses the menu anyway: > https://github.com/mozilla/firefox-ui-tests/blob/mozilla-central/ > firefox_puppeteer/tests/test_about_window.py#L64 Yes, that his a test and the opening strategy for menu gets forwarded. The extra opener makes use of the menu too and we should update the code to use the menu bar instead of the specific element. > 2) This one is a button and not a menu from what I understand: > https://github.com/mozilla/firefox-ui-tests/blob/mozilla-central/ > firefox_puppeteer/tests/test_places.py#L50 That is the button in the navbar, correct. It is not related to the menu. Once you finished the file in 1) and setup r? I will have a closer look at the PR.
Flags: needinfo?(hskupin)
Attachment #8679489 - Flags: review?(hskupin)
Comment on attachment 8679489 [details] [review] Github PR (usage of ids) Looks great. I merged this PR to mozilla-central as: https://github.com/mozilla/firefox-ui-tests/commit/b1ea646a38f8373364aceac895083990bdff7e80
Attachment #8679489 - Flags: review?(hskupin)
Attachment #8679489 - Flags: review+
Attachment #8679489 - Flags: checkin+
As Tareq and I discussed on IRC the next part on that bug will be to refactor the menu classes and allow to also use them for contextual menus, and not only for the menubar.
Status: REOPENED → ASSIGNED
Whiteboard: [lib]
The second PR for this bug covers refactoring the Menu class to support additional menus (contentAreaContextMenu and main-menubar) and making the MenuElement class into separate class in the same menu.py file that it is currently.
Attachment #8679489 - Attachment is obsolete: true
Comment on attachment 8679489 [details] [review] Github PR (usage of ids) This attachment is not obsolete. It was for a different PR and already got merged.
Attachment #8679489 - Attachment description: github-pr-for-1121710.txt → Github PR (usage of ids)
Attachment #8679489 - Attachment is obsolete: false
Attachment #8682198 - Attachment description: second_github_pr_1121710.txt → Github PR (refactor and generalization)
Comment on attachment 8682198 [details] [review] Github PR (refactor and generalization) Please review.
Attachment #8682198 - Flags: review?(hskupin)
Comment on attachment 8682198 [details] [review] Github PR (refactor and generalization) Great work so far! The classes look promising but there are still two missing things which need to be fixed. Also the additional tests which I have pointed out on the PR should be added. Switching review request to feedback for now. But once updated you can ask for review again via this attachment.
Attachment #8682198 - Flags: review?(hskupin) → feedback+
Hi Tareq! I want to ask if you have an update for us regarding this bug. Please let me know if you don't have the time anymore to work on it or if there are other issues. Thanks.
Flags: needinfo?(tareqakhandaker)
Hi Henrik. I don't think I have the time to work on this bug anymore. Sorry for not letting you know earlier.
Flags: needinfo?(tareqakhandaker)
Assignee: tareqakhandaker → nobody
Status: ASSIGNED → NEW
Product: Mozilla QA → Testing
Mentor: hskupin
Whiteboard: [lang=py]
Hey Whimboo! I would like to work on this. I'm not sure what remains to be done for this bug. As Firefox-UI-Tests has been merged with the Mozilla-Central Branch, Where is the concerned file in the new repo? Thanks!

We are going to remove the package over on bug 1573383.

Status: NEW → RESOLVED
Closed: 10 years ago → 5 years ago
Resolution: --- → WONTFIX
See Also: → 1573383
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: