1121710 - Fix MenuBar class for correct handling of menus

Status: RESOLVED WONTFIX

(Testing :: Firefox UI Tests, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

This is not a blocker for security tests in Q1.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Backported the skip patch for the unit test to all the other branches as:
https://github.com/mozilla/firefox-ui-tests/commit/8e8fcbcf9c680f77bd5bae9dea1f2ae06c6ffe38

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 5

[Tareq Khandaker]

Attachment: Github PR (usage of ids)

The attachment has a link to the Github pull request.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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

Comment 8

[Tareq Khandaker]

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

Comment 9

[Tareq Khandaker]

If those two files are alright as is, I will ask for r?

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(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.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 13

[Tareq Khandaker]

Attachment: Github PR (refactor and generalization)

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.

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 15

[Tareq Khandaker]

Comment on attachment 8682198 [details] [review]
Github PR (refactor and generalization)

Please review.

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 18

[Tareq Khandaker]

Hi Henrik. I don't think I have the time to work on this bug anymore. Sorry for not letting you know earlier.

Comment 19

[Aditya Motwani (:AdityaM)]

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!

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The firefox-ui harness and tests can be found here:
https://dxr.mozilla.org/mozilla-central/source/testing/firefox-ui

Any Firefox puppeteer related code is here:
https://dxr.mozilla.org/mozilla-central/source/testing/marionette/puppeteer/firefox/

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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