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)
Tracking
()
Tracking | Status | |
---|---|---|
firefox116 | --- | fixed |
People
(Reporter: rpl, Assigned: annhermy, Mentored)
References
Details
(Keywords: good-first-bug)
Attachments
(2 files)
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).
Updated•4 years ago
|
Reporter | ||
Updated•4 years ago
|
Hi my name is Leslie and I'm an Outreachy applicant. Can I work on this bug?
Reporter | ||
Comment 2•4 years ago
|
||
(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:
-
Ensure your development environment is working correctly and you can successfully build and run Firefox using the artifact built approach (it is a build mode that allows you to build Firefox faster if you don't need to change any C++ or Rust code)
-
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
) -
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:
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.
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?
Reporter | ||
Comment 4•4 years ago
|
||
(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).
Comment 5•3 years ago
|
||
Hi, I am an outreachy applicant. I would like to work on this bug.If anyone not working on it.
Reporter | ||
Comment 6•3 years ago
|
||
(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).
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.
Reporter | ||
Comment 9•2 years ago
|
||
(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.
Reporter | ||
Comment 10•2 years ago
|
||
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.
Comment 11•2 years ago
•
|
||
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.
Comment 12•2 years ago
|
||
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:
- 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. - 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 sameright
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.
Comment 13•2 years ago
|
||
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)
Comment 14•2 years ago
|
||
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
Comment 15•2 years ago
|
||
(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:
- 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.- 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 sameright
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.
Comment 16•2 years ago
|
||
(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.
Comment 17•2 years ago
|
||
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!
Comment 18•2 years ago
|
||
It's been assigned to you Siya, let me know if you have any questions.
Comment 19•2 years ago
|
||
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!
Comment 20•2 years ago
|
||
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.
Comment 21•2 years ago
|
||
Comment 22•2 years ago
|
||
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
Comment 23•1 year ago
|
||
This good-first-bug hasn't had any activity for 2 months, it is automatically unassigned.
For more information, please visit BugBot documentation.
Assignee | ||
Comment 24•1 year ago
|
||
Updated•1 year ago
|
Comment 25•1 year ago
|
||
Comment 26•1 year ago
|
||
bugherder |
Description
•