Closed Bug 1703495 Opened 3 years ago Closed 11 months ago

Add explicit test coverage for the expected alignment of the about:addons options menu attached to the gear button

Categories

(Toolkit :: UI Widgets, task, P5)

Product:
Toolkit
Component:
UI Widgets
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
116 Branch
Tracking Flags:
Tracking Status
firefox116 --- fixed

People

(Reporter: rpl, Assigned: annhermy, Mentored)

References

Details

(Keywords: good-first-bug)

Attachments

(2 files)

Bug 1703495 - Add explicit test coverage for the expected alignment of the about:addons options menu attached to the gear button. r=mstriemer
48 bytes, text/x-phabricator-request
Details | Review
Bug 1703495 - Add explicit test coverage for the expected alignment of the about:addons options menu attached to the gear button r=hjones
48 bytes, text/x-phabricator-request
Details | Review

In Bug 1703487 ntim is fixing the alignment of this about:addons options menu, which was going to be a bit off when the about:addons page width is making the gear menu to be near the end of the right side of the page (or the left side in rtl mode).

Given that the original regressions from Bug 1699917 wasn't making any test to fail, the expected alignment of this options menu isn't covered by any test.

It would be good to add one to be able to catch this kind of regression (if I'm not mistaken we do cover something similar for other UI element of the about:addons page, and so it seems reasonable to cover also this one more explicitly).

Severity: -- → N/A
Keywords: good-first-bug
Priority: -- → P5
Mentor: lgreco

Hi my name is Leslie and I'm an Outreachy applicant. Can I work on this bug?

Flags: needinfo?(lgreco)

(In reply to Leslie from comment #1)

Hi my name is Leslie and I'm an Outreachy applicant. Can I work on this bug?

Hi Leslie,
sure thing, the bug is up for grab.

If this is the first time you are looking into contributing a change to Firefox Add-ons Manager internals (and/or Firefox in general) you may want to take a look to our wiki page at https://wiki.mozilla.org/WebExtensions/Contribution_Onramp (e.g. "how to setup your development environment" and "Add new tests" can help you to get some of the initial pointers needed to work on this bug).

As first steps I would suggest to:

I often suggest contributor to try to make some small changes to an existing tests and see what happens if you run it again, and to do not worry if it breaks, especially because "breaking the test and try to read and understand the errors is an important step of your path to understand it"!

and then to feel free to ask us questions that would let you understanding more of it piece after piece.

Flags: needinfo?(lgreco)

Okay, I set up my development environment and I've successfully built Firefox using the artifact build approach.
I was also successful in running some Add-ons Manager mochitests.
I am a little green when it comes to testing, so I was not too sure what to do with the third task. I did break the test by removing one of the awaits inside the async function. It prevented the pop up from closing and I believe the test was looking for it, so when it timed out, the test did not pass.
Am I on the right track?

Flags: needinfo?(lgreco)

(In reply to Leslie from comment #3)

Okay, I set up my development environment and I've successfully built Firefox using the artifact build approach.
I was also successful in running some Add-ons Manager mochitests.
I am a little green when it comes to testing, so I was not too sure what to do with the third task. I did break the test by removing one of the awaits inside the async function. It prevented the pop up from closing and I believe the test was looking for it, so when it timed out, the test did not pass.
Am I on the right track?

yep, based on what you the described I think you are on the right track.

now, the goal on this issue is to be sure that we do have a test that fails (possibly with an error message that makes it clear why, so that it makes it easier for us to figure out what is going on if there is a regression and the test starts to fail again) and so we need to:

  • determine how to assert that the alignment is right, and add a new assertion in an existing tests (if there is one where the assertion could fit without making the test unclear or harder to maintain in the long run) or a new test
  • then verify that the test pass successfully, but it breaks and log the expected assertion error if locally you try to temporarily revert the fix landed from Bug 1703487 and run the same test again

To determine how to assert the alignment you may take a look to the code nearby the fix landed from Bug 1703487 (https://hg.mozilla.org/mozilla-central/rev/70ef4799e3cd), you can look to the surrounding code locally and/or on searchfox:

In particular, the call to window.windowUtils.getBoundsWithoutFlushing seems pretty relevant (you may also experiment a bit interactively by opening the developer tools on an about:addons tab, select some node in the inspector and then call window.windowUtils.getBoundsWithoutFlushing($0) to see what the method will return you, in the developer tools $0 is a reference to the DOM node currently selected in the inspector panel).

window.windowUtils.getBroundWithoutFlushing is a method not available in a regular webpage (is part of the Firefox internals), but as part of the WebAPI there is getBoundingClientRect which may be useful to look on MDN to get a better picture (and/or more specific questions based on the initial observations):

You may also start to have more questions about particular parts of the test case you tried to change (e.g. browser_page_options_install_addon.js) that you are not yet understanding or you are not 100% sure if you are understanding correctly, feel free to ask me some questions about it (don't worry if you are not sure if the question is a good one, it would still help me to guess what may be more useful for me to describe you in a bit more details, without risking to be "flooding" you with too many details all at ones ;-))

(Feel also free to drop me an email if you have questions that you feel would be useful for you to work on this but not that strictly related to this particular issue and I'll reply over email with more details).

Flags: needinfo?(lgreco)

Hi, I am an outreachy applicant. I would like to work on this bug.If anyone not working on it.

Flags: needinfo?(lgreco)

(In reply to Anshuman Singh from comment #5)

Hi, I am an outreachy applicant. I would like to work on this bug.If anyone not working on it.

yes, no one is currently working on it (the previous comments are from almost one year ago).

Based on a quick look on searchfox.org (a website that indexes Firefox source code, we use it a lot to navigate the code, e.g. as part of planning changes or investigate issues) the details from comment 2 and comment 4 are still relevant and should help you to dig into the code related to the change needed and to plan the additions to the tests.

Let us know if you have further doubts or questions that we can help you with (e.g. by detail your questions and/or doubts with enough context in a new bugzilla comment and needinfo me on it).

Flags: needinfo?(lgreco)

Hello! I am an outreachy applicant and would like to work on this, I'll go through the comments thread above and see if I have any doubts, although it looks like I know what is to be done. Thanks for the opportunity.

(In reply to Luca Greco [:rpl] [:luca] [:lgreco] from comment #2)

  • Take a look to an existing test that is opening the same popup menu that we should cover explicitly with a test assertion that checks the alignment of that popup:
    • https://searchfox.org/mozilla-central/rev/46a67b8656ac12b5c180e47bc4055f713d73983b/toolkit/mozapps/extensions/test/browser/browser_page_options_install_addon.js#92-128
      Hey! So I've been trying to figure this out for a while now, and from what I can understand, this is a test that checks if the install button in the about::addons pop-up menu is present or not. And what it essentially does is open the pop-up menu, if the install button is shown there it shows us the message "The install button is shown" and if the install button is hidden a message saying "The install button is now hidden" is sent back. Is this correct? And from comment #4 what I understand is we need a test that asserts the perfect alignment of the the options menu with respect to the gear button, and this is either to be added as a new test or is to be included in an already existing test? I did look at the code which was added to align the pop-up, I'm not too sure if I understand it yet. But for now can you please confirm if I'm on the right track? If I am, could you please guide me as to in which file particularly will the test be added, I'm unable to figure this out.
Flags: needinfo?(lgreco)

(In reply to Siya from comment #8)

Hey! So I've been trying to figure this out for a while now, and from what I can understand, this is a test that checks if the install button in the about::addons pop-up menu is present or not. And what it essentially does is open the pop-up menu, if the install button is shown there it shows us the message "The install button is shown" and if the install button is hidden a message saying "The install button is now hidden" is sent back. Is this correct? And from comment #4 what I understand is we need a test that asserts the perfect alignment of the the options menu with respect to the gear button, and this is either to be added as a new test or is to be included in an already existing test? I did look at the code which was added to align the pop-up, I'm not too sure if I understand it yet. But for now can you please confirm if I'm on the right track? If I am, could you please guide me as to in which file particularly will the test be added, I'm unable to figure this out.

Hi Siya,
The custom element that takes care of the alignment have been recently refactored out of aboutaddons.js and moved in the set of reusable gecko widgets at toolkit/content/widgets/panel-list.js as part of Bug 1765635 (the setAlign function, the one where ntim fix was applied, looks still very much the one we used to have in aboutaddons.js and so technically the test coverage for that corner case seems to be still missing), and so it seems that now a unit test targeting the panel-list reusable widget may be a better fit for adding explicit test coverage for the expected alignment.

Covering the scenario in a unit test may be actually even easier than testing it out in an about:addons integration test, in particular because to be be able to hit the same scenario where there used to be a regression fixed in Bug 1703487:

  • in a unit test we could instead just place the anchor element in the position we need to make sure the panel popup has to be right aligned to the anchored element
  • whereas to trigger the same in an integration test for the aboutaddons page, we would need to resize the Firefox window first, to make sure the gear button in the aboutaddons page gets too near to the right of the aboutaddons page for the popup menu to be aligned on the left of the gear button and forces it to be right aligned to it (and also wait for the window to be actually resized, because resizing a Firefox window is going to happen asynchronously).

Let's see what Mark thinks about covering this with a unit test for panel-list.

Flags: needinfo?(lgreco)

Hi Mark,
Would you mind to help us to determine if a unit test for the toolkit/content/widgets/panel-list.js to cover that corner case now that the custom element have been refactored out of aboutaddons.js and moved into a reusable component?

As I mention briefly in comment 9, covering the scenario fixed by Bug 1703487 as an integration test for aboutaddons.js would require the new test case to be resizing the browser window to make sure the panel popup will have to be right aligned instead of left aligned, and so at a first glance it seems that a unit test may be a better fit.

Flags: needinfo?(mstriemer)

Thanks Luca,

This code has indeed moved to our shared widgets, so I'm moving the ticket and taking the mentor slot.

A new test file can be created by running ./mach addtest toolkit/content/tests/widgets/test_panel_list_anchoring.html --suite=mochitest-chrome. You can then run the test with ./mach test test_panel_list_anchoring.html.

The test_panel_list_accessibility.html test likely has some examples of how to get started with a test that has some buttons and opens the menu. I'd recommend using similar markup and testing that the menu is aligned correctly in the top left, top right, bottom left, and bottom right corners of the screen. The button can be moved by setting something like anchorButton.style.position = "fixed"; anchorButton.style.right = "5px"; in the test. Alternatively, you could create multiple buttons where you'd expect them to be and verify that triggering the menu from different buttons puts the menu in the correct spot.

You can check the position of the menu and buttons by using Element.getBoundingClientRect(). The left-aligned button and the menu should have the same left value, and the right-aligned button and menu should have the same right value when the menu is open. Similarly, the menu's top and button's bottom should match when the button is at the top, and the reverse when the button is at the bottom. There's more info in comment 4 about this, though it may link to code that has since moved.

Mentor: lgreco → mstriemer
Component: Add-ons Manager → XUL Widgets
Flags: needinfo?(mstriemer)

Hi Mark,
So from what I undertand, I will keep the markup the same as is, and could do any one of the two things mentioned in:

  1. let there be just one button, after this I could have a loop and through each iteration I can move the button to the top, left, right and bottom, with somethings like anchorButton.style.position = "fixed"; anchorButton.style.right = "5px"; as you mentioned. And according to the then position of the button I will check for their alignment with the triggered menu.
  2. make 1 button each for top, right, left, bottom and then check check for their alignment with the triggered menu.
    Am I thinking correct?

(In reply to Mark Striemer [:mstriemer] from comment #11)

The left-aligned button and the menu should have the same left value, and the right-aligned button and menu should have the same right value when the menu is open. Similarly, the menu's top and button's bottom should match when the button is at the top, and the reverse when the button is at the bottom. There's more info in comment 4 about this, though it may link to code that has since moved.

A little silly, but are we supposed to check this with something a simple as anchorButton.style.right == panelList.style.right (in case of a right aligned button) and something of this sort for button in the other positions as well? I've never really written a test before, any help would be appreciated.

Flags: needinfo?(mstriemer)

hello I am interested in this can i work on it please
(I am an outreachy applicant and new to this though but with guidance , i can do it)

Flags: needinfo?(lgreco)

Ensure you can successfully run some Add-ons Manager mochitests (e.g. mach mochitest toolkit/mozapps/extensions/test/browser/browser_page_options_install_addon.js) how do i run a test for the above condition on my pc

(In reply to Siya from comment #12)

Hi Mark,
So from what I undertand, I will keep the markup the same as is, and could do any one of the two things mentioned in:

  1. let there be just one button, after this I could have a loop and through each iteration I can move the button to the top, left, right and bottom, with somethings like anchorButton.style.position = "fixed"; anchorButton.style.right = "5px"; as you mentioned. And according to the then position of the button I will check for their alignment with the triggered menu.
  2. make 1 button each for top, right, left, bottom and then check check for their alignment with the triggered menu.
    Am I thinking correct?

That is correct, I think either approach would work well, so whatever seems easiest or has the clearest implementation.

(In reply to Mark Striemer [:mstriemer] from comment #11)

The left-aligned button and the menu should have the same left value, and the right-aligned button and menu should have the same right value when the menu is open. Similarly, the menu's top and button's bottom should match when the button is at the top, and the reverse when the button is at the bottom. There's more info in comment 4 about this, though it may link to code that has since moved.

A little silly, but are we supposed to check this with something a simple as anchorButton.style.right == panelList.style.right (in case of a right aligned button) and something of this sort for button in the other positions as well? I've never really written a test before, any help would be appreciated.

The Element.getBoundingClientRect() method (linked above) returns an object with keys like top, right, left, bottom. These values can be used to get the exact position of an element. Something like let { bottom, left } = anchorButton.getBoundingClientRect() will give you the bottom and left edges of the anchor button

Siya, I see you already have bug 1761282 assigned to you. Since there are so many Outreachy applicants at the moment we'd like to keep only one assigned in-progress bug per applicant. So for while that work is still in progress I'd like to leave this open to other contributors.

Flags: needinfo?(mstriemer)

(In reply to briannapatricia7 from comment #14)

Ensure you can successfully run some Add-ons Manager mochitests (e.g. mach mochitest toolkit/mozapps/extensions/test/browser/browser_page_options_install_addon.js) how do i run a test for the above condition on my pc

If you've gone through the build setup process and the ./mach run command opens a Firefox browser (this would be your locally built Firefox), then you should be able to run an existing panel-list test with ./mach test test_panel_list_accessibility.html

If that does not work for you then you will need to resolve your local build setup. You can follow that guide or look at the quick reference. The quick reference also links to the #introduction Matrix channel where you can ask for help with any problems you may encounter.

Flags: needinfo?(lgreco)

Hi Mark, will it be okay if I start looking into this bug back again? I'm unassigned right now and seems like the other person is not working on it. Please do let me know. Thanks!

Flags: needinfo?(mstriemer)

It's been assigned to you Siya, let me know if you have any questions.

Assignee: nobody → siya066btit21
Flags: needinfo?(mstriemer)

Hi Mark, so I have written the unit test and when I try to run it certain tests fail with the following message: https://pastebin.mozilla.org/obSZ8kjE
and this is the code I've written so far: https://pastebin.mozilla.org/op9h7bng I have used test_panel_list_accessibility.html and comment #4 primarily as reference. I'm also new to writing unit tests so I'm not really sure where am I going wrong. I'd really appreciate some help. Thank you!

Flags: needinfo?(mstriemer)

Hi Mark, there were a lot of bad and pretty obvious mistakes in the code that I'd written previously, I was able to figure them out and am finally submitting a patch, the test cases are failing, so the alignment is obviously going wrong, one thing that I could think of is to simply add the values from the failed messages to menu's left, right, top, bottom property. Please tell me if this approach is correct.
Also, since this is a unit test, should I also include it in toolkit/content/tests/widget/chrome.ini, and if yes, are there are any specific conditions under which the test runs or does not run?
Sincere apologies for the silly mistakes in my previous code, I should've thought about them a bit more.

Hi Siya, sorry I missed your needinfo before

I left some comments on your patch on phabricator, it's definitely moving in the right direction! Thanks again

Flags: needinfo?(mstriemer)

This good-first-bug hasn't had any activity for 2 months, it is automatically unassigned.
For more information, please visit BugBot documentation.

Assignee: siya066btit21 → nobody
Assignee: nobody → annhermy
Status: NEW → ASSIGNED
Pushed by hjones@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d208b65e0f14
Add explicit test coverage for the expected alignment of the about:addons options menu attached to the gear button r=hjones

https://hg.mozilla.org/mozilla-central/rev/d208b65e0f14

Status: ASSIGNED → RESOLVED
Closed: 11 months ago
status-firefox116: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 116 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: