Closed Bug 1820284 Opened 3 years ago Closed 3 years ago

Use moz-button-group in the "Add new address" form autofill dialog in about:preferences

Categories

(Toolkit :: UI Widgets, task)

Product:
Toolkit
Component:
UI Widgets
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
113 Branch
Tracking Flags:
Tracking Status
firefox113 --- fixed

People

(Reporter: hjones, Assigned: imlata1111, Mentored)

References

(Blocks 1 open bug)

Details

(Keywords: good-first-bug, Whiteboard: [fidefe-reusable-components][lang=html][lang=js])

Attachments

(4 files, 2 obsolete files)

Add New Address Dialog.png
104.89 KB, image/png
Details
Navigate to adress autofill dialog
4.10 MB, video/quicktime
Details
Screenshot after changes
27.49 KB, image/png
Details
Bug 1820284 - Use moz-button-group in the Add new address r=hjones!
48 bytes, text/x-phabricator-request
Details | Review

The add/edit address dialog for form autofill contains a pair of buttons that don't get reordered based on platform (the order stays the same for Mac/Windows). We could easily provide reordering and ensure consistent styling by wrapping the buttons in a moz-button-group element.

To help Mozilla out with this bug, follow these steps:

  1. Comment here on the bug that you want to volunteer to help. This will tell others that you're working on the next steps.
  2. Download and build the Firefox source code
  3. Start working on this bug.
    • To find the dialog in Firefox, navigate to about:preferences#privacy and scroll to the "Forms and Autofill" section, or search for "autofill". You will may need to click the checkbox to enable "Autofill addresses". Once it's enabled, you should be able to click the "Saved addresses" button, then click on the "Add" button in the next dialog to surface the "Add new address" dialog.
    • The code For the buttons can be found here.
    • You'll need to replace the buttons with a moz-button-group, then double check to see if there's any CSS that needs to be removed (anything that changes the size or spacing of the buttons). You may need to add a script tag to make the moz-button-group element available in the dialog. You can see an example of similar work in Bug 1802377.
    • If you have any problems with this bug, please comment on this bug and set the needinfo flag for me. Also, you can find me and my teammates on the #reusable-components channel on Element/Matrix most hours of most days.
  4. Build your change with mach build and verify your changes locally. You can also test your change by running some of the formautofill tests (in particular the browser_editAddressDialog.js tests). More information on running tests can be found here. Also check your changes for adherence to our style guidelines by using mach lint
  5. Submit the patch (including an automated test, if applicable) for review. Mark me as a reviewer so I'll get an email to come look at your code.
  6. After a series of reviews and changes to your patch, I'll mark it for checkin or push it to autoland. Your code will soon be shipping to Firefox users worldwide!
Whiteboard: [ → [lang=html][lang=js]

I am looking into this bug and working on the next steps.

Great! I've gone ahead and set you as the assignee for now while you work on it. If you have any questions or run into issues you can request information from me using the form below, or reach out to me and my teammates in the #reusable-components channel.

Assignee: nobody → imlata1111
Status: NEW → ASSIGNED

Sure

Hey! I am not able to find the above dialog as I can't find any option about:preferences#privacy in the settings. Please help me finding this.

Hey, sorry I should have given clearer instructions. If you navigate to about:preferences you can click on the "Privacy & Security" link from the navigation panel on the left hand side of the page. That will take you to about:preferences#privacy, and you should be able to scroll down to find the "Forms and Autofill" section. Alternatively you can put about:preferences#privacy into the address bar, hit enter, and it should take you to where you need to go.

I've recorded/attached a video of me navigating to the dialog, hopefully that helps!

Whiteboard: [lang=html][lang=js] → [fidefe-reusable-components][lang=html][lang=js]

That's indeed very helpful. Thankyou!

Hey! According to your instructions and video, "Form and autofill" should be after "Logins and password" and before "History" but my firefox does not contain it. I am attaching a screenshot please look into it.

Hey! According to your instructions and video, "Form and autofill" should be after "Logins and password" and before "History" but my firefox does not contain it. I am attaching a screenshot please look into it.

I'm sorry but I am not able to attach screenshot here. I am tagging you on the public channel of "reusable component".

Hey! I have made the changes "We could easily provide reordering and ensure consistent styling by wrapping the buttons in a moz-button-group element." It now look like this on my device(Windows). Is it done or I need to make other changes too, I am not getting how should it look like finally? Can you provide some design or idea please?

Flags: needinfo?(hjones)

Nice I'm happy to see you were able to get everything working locally! The "consistent styling" is something that is provided by the moz-button-group element itself - it applies it's own styles to the buttons it wraps, so even just using it may be enough in this case.

Here's the CSS it's applying to the buttons for reference: https://searchfox.org/mozilla-central/source/toolkit/content/widgets/moz-button-group/moz-button-group.css#11-13

In addition to positioning the buttons using flexbox, the button group is also removing any extra margins that may have been added, which helps standardize how our buttons look.

One thing I see in your screenshot is that the button group seems to be wrapping both the buttons and the text, when we just want it to wrap these buttons: https://searchfox.org/mozilla-central/source/browser/extensions/formautofill/content/editAddress.xhtml#80-81

I think if you just move the <span> out of the button group you should be good. It might be easier if you submit a patch with the work you've done so far and I can add comments to the review. Instructions on how to submit a patch can be found here: https://firefox-source-docs.mozilla.org/devtools/contributing/making-prs.html

Let me know if you have any other questions!

Flags: needinfo?(hjones)

Thankyou so much! I am submitting the patch for easier review.

Hey! I have submitted a patch with the work I have done till. I have also moved the <span> out of the button group as you said. Please review it and suggest the required changes if any.
https://phabricator.services.mozilla.com/D171914

Hi Lata, I just added some comments, but your patch is currently submitted as WIP so I'm not sure you will be notified.

After you make the suggested changes, you'll want to re-submit the patch and possibly change the commit message so it's no longer WIP.

If you run this in your terminal:

hg commit --amend

It will pop up a shell where you can change the commit message. You should change it to something like this:

Bug 1820284 - Use moz-button-group in the Add new address r=hjones!

You'll want to remove the "WIP" from before the bug number, as well as the [devtools] reference since this isn't actually a devtools patch. You'll also need to tag me as the reviewer with r=hjones, and I think that will get your patch out of WIP. You may also be able to make these edits through Phabricator from the right hand side menu where it says "Edit revision".

Then you can run moz-phab submit again.

Let me know if you have issues with any of this.

Flags: needinfo?(imlata1111)
Attachment #9321713 - Attachment description: WIP: Bug 1820284 -[devtools] Use moz-button-group in the Add new address :hjones! → Bug 1820284 - Use moz-button-group in the Add new address r=hjones!

Hey! Thankyou for your suggestions. I have made the required changes and submitted my code again. Please review.

Flags: needinfo?(imlata1111)
Pushed by hjones@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/9fe73bfccdfa Use moz-button-group in the Add new address r=hjones,credential-management-reviewers

Backed out changeset 9fe73bfccdfa (Bug 1820284) for bc failures on browser_editAddressDialog.js.
Backout link
Push with failures <--> bc4
Failure Log

Flags: needinfo?(imlata1111)

Hey! What exactly caused this and how can I further make correction in this?(In reply to Marian-Vasile Laza from comment #18)

Backed out changeset 9fe73bfccdfa (Bug 1820284) for bc failures on browser_editAddressDialog.js.
Backout link
Push with failures <--> bc4
Failure Log

Flags: needinfo?(imlata1111)

Sorry but I can't help you with code fixes in your code, I can only point out from where the fail originated, thanks.

So what this means is the change got backed out because it started causing some of our automated tests to fail. This is my bad - I should have done a test run on our automation servers before landing your code change.

In any case, the best next step is for you to try to reproduce this test failure locally. You can check out your patch again by running

hg pull central && hg up central
moz-phab patch D171914 --apply-to here

Once you've done that, try running

./mach test browser_editAddressDialog.js

You might get a bunch of FAIL notices, which will be expected and helpful. If you can reproduce the test failure locally, then I would recommend taking a look at the code in the test file here: https://searchfox.org/mozilla-central/source/browser/extensions/formautofill/test/browser/browser_editAddressDialog.js

The moz-button-group changes the position of buttons based on the users operating system, so one effect of your code change is the buttons may be in a different order on Windows and Linux then they used to be. Taking a quick look at those tests, it looks like we define an array of keypress actions that are used to fill out and submit the form. I expect what you'll need to do to get these tests passing again is change this sequence slightly - it will need to be different for Windows/Linux vs Mac since the buttons for submitting or cancelling submission of the form will be in different places. You may be able to detect operating system by checking navigator.platform or AppConstants.platform - I would recommend looking for other places in test code where we're already doing this to get a sense of how that will work.

I haven't actually tried all of that locally, so these are just my best guesses on what's happening to get you started. Give all that a try and let me know if/when you get stuck.

(In reply to Hanna Jones [:hjones] from comment #21)

So what this means is the change got backed out because it started causing some of our automated tests to fail. This is my bad - I should have done a test run on our automation servers before landing your code change.

In any case, the best next step is for you to try to reproduce this test failure locally. You can check out your patch again by running

hg pull central && hg up central
moz-phab patch D171914 --apply-to here

Once you've done that, try running

./mach test browser_editAddressDialog.js

You might get a bunch of FAIL notices, which will be expected and helpful. If you can reproduce the test failure locally, then I would recommend taking a look at the code in the test file here: https://searchfox.org/mozilla-central/source/browser/extensions/formautofill/test/browser/browser_editAddressDialog.js

The moz-button-group changes the position of buttons based on the users operating system, so one effect of your code change is the buttons may be in a different order on Windows and Linux then they used to be. Taking a quick look at those tests, it looks like we define an array of keypress actions that are used to fill out and submit the form. I expect what you'll need to do to get these tests passing again is change this sequence slightly - it will need to be different for Windows/Linux vs Mac since the buttons for submitting or cancelling submission of the form will be in different places. You may be able to detect operating system by checking navigator.platform or AppConstants.platform - I would recommend looking for other places in test code where we're already doing this to get a sense of how that will work.

I haven't actually tried all of that locally, so these are just my best guesses on what's happening to get you started. Give all that a try and let me know if/when you get stuck.

Hey! Since, Now it shows "Backout merged to central: https://hg.mozilla.org/mozilla-central/rev/09992798c02f" do I still have to do the process you suggested?

Yes that just means the commit that reverted your code changes got merged, so your code changes that cause the test failure are no longer on central. You should still try to reproduce the test failure and fix it locally. Once you have a fix you can update your patch, I'll give it another review, then we can re-land your code changes.

Okay! I'm on it.

Hey! I have made the changes as you suggested to get all the test pass. But I think two different commit has been made. Is that fine or should I squash them into a single commit?

Please squash them into a single commit, you can use hg histedit to do this. There's some more information on how to do that here.

Once you've squashed the commits just run moz-phab submit again. Thanks!

Hey! I have merged my changes into the prevoius "accepted" commit. https://phabricator.services.mozilla.com/D171914
Please review.

Attachment #9322827 - Attachment is obsolete: true
Attachment #9322839 - Attachment is obsolete: true
Pushed by hjones@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a74295853296 Use moz-button-group in the Add new address r=hjones,credential-management-reviewers,sgalich

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

Status: ASSIGNED → RESOLVED
Closed: 3 years ago
status-firefox113: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 113 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: