mozilla-mobile / fenix

⚠️ Fenix (Firefox for Android) moved to a new repository. It is now developed and maintained as part of: https://github.com/mozilla-mobile/firefox-android
https://github.com/mozilla-mobile/firefox-android
Mozilla Public License 2.0
6.47k stars 1.27k forks source link

Bug 1812616 - Enable clear button on the Password and Host fields #28676

Closed titooan closed 1 year ago

titooan commented 1 year ago

Two bugs were fixed in this PR, because they relate to the same screen and I could update the related UI test at once, avoiding further conflicts that could appear if I had opened two separate PRs.

This PR fixes the behavior of the clear button on both the Password and the Host fields: the buttons should be visible and enabled as soon as the field contains a valid value.

Bug id Screenshot before fix Screenshot after fix
1812616 - Password field Screenshot of the clear button disabled on Password field Screenshot of the button enabled on Password field
1812613 - Host field Screenshot of the clear button invisible on Host field Screenshot of the clear button enabled and visible on Host field

Closes 1812616

Pull Request checklist

QA

To download an APK when reviewing a PR (after all CI tasks finished running):

  1. Click on Checks at the top of the PR page.
  2. Click on the firefoxci-taskcluster group on the left to expand all tasks.
  3. Click on the build-debug task.
  4. Click on View task in Taskcluster in the new DETAILS section.
  5. The APK links should be on the right side of the screen, named for each CPU architecture.

GitHub Automation

Used by GitHub Actions.

gabrielluong commented 1 year ago

@titooan Just a consideration for your PR title and commit message to do Bug 1812616, Bug 1812613 - Enable clear button on the Password and Host fields to ensure that it's linked in Bugzilla. This will match the regex used by our Bugzilla integration which parses these GitHub PRs so that it will automatically attach your PR to the provided bugs in Bugzilla. If they aren't linked or the commit message doesn't have the second Bug in the commit message, then the Bugzilla issue won't close. Alternatively, you could flag one as duplicate and change the description of the other one. Do note the commit message currently doesn't have 1812613 so we just want to make sure that gets closed one way or another.

This is the commit with the Bugzilla regex if you were interested to see how it works https://github.com/mozilla-bteam/bmo/commit/8509bbaa68c31c00621832728bd799b724e94751#diff-7c417a2dd5c9150d5d680d5506ea703dae0b1933954dcdec522ef71bc00f6b77R23

gabrielluong commented 1 year ago

When you land this, could you make sure to also set in Bugzilla the QA flags https://mozilla-hub.atlassian.net/wiki/spaces/Mobile/pages/113443418/Android+Bugzilla+GitHub+workflow#How-to-ask-QA-to-verify-a-bug-fix

titooan commented 1 year ago

@gabrielluong

I'm a bit confused with one point:

If they aren't linked or the commit message doesn't have the second Bug in the commit message, then the Bugzilla issue won't close. [...] Do note the commit message currently doesn't have 1812613 so we just want to make sure that gets closed one way or another.

Actually the commit message is:

Bug 1812616 - Enable clear button on the Password field even after it has been clicked once Bug 1812613 - Show clear button on the Host field when its content is valid

Does it mean that the regex can detect only one Bugzilla link per commit message? (I'm not sure I understand well the Bugzilla regexp)

Also, I changed the title of the PR to follow your advice, but it seems that the bug 1812613 is still not linked to this PR. Should I trigger the Bugzilla integration again somehow? 

Thanks a lot for your helpful comments!

gabrielluong commented 1 year ago

@gabrielluong

I'm a bit confused with one point:

If they aren't linked or the commit message doesn't have the second Bug in the commit message, then the Bugzilla issue won't close. [...] Do note the commit message currently doesn't have 1812613 so we just want to make sure that gets closed one way or another.

Actually the commit message is:

Bug 1812616 - Enable clear button on the Password field even after it has been clicked once Bug 1812613 - Show clear button on the Host field when its content is valid

Does it mean that the regex can detect only one Bugzilla link per commit message? (I'm not sure I understand well the Bugzilla regexp)

Also, I changed the title of the PR to follow your advice, but it seems that the bug 1812613 is still not linked to this PR. Should I trigger the Bugzilla integration again somehow? 

Thanks a lot for your helpful comments!

Oh I see the second bug is in the commit description. I would still do the following format Bug 1812616, Bug 1812613 - for better readability and just having the bug numbers right up front. The bugzilla regex probably won't parse the description (but I am not completely sure).

titooan commented 1 year ago

I closed the Bug 1812613 and tagged it as duplicate of the Bug 1812616. Then I edited the description of the latter so that it describes both issues.

After talking with @gabrielluong it seems to be the better way of:

I also edited the commit message to follow the right format ("Bug XX, Bug XX" instead having the second bug in the description of the commit). This triggered the CI again and the flacky test passed succesfully this time (FYI @kycn)