For #11404 : "Open all..." option

[Taknok]

Add the "Open all..." option as described in #11404 (similar to "Open all" present on firefox desktop).

This option open recursively all link in a bookmark folder, if there is no link in the folder, display a toast message with the error. All strings a retrieved from "l10n-mozilla firefox-desktop". Once links are opened, the tabs tray is shown.

Folder structure for this exemple:

How the option is shown in overflow menu:

When opening all:

When a folder is empty:

Pull Request checklist

• [x] Tests: This PR includes thorough tests or an explanation of why it does not
• [x] Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not
• [ ] Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features. In addition, it includes a screenshot of a successful accessibility scan to ensure no new defects are added to the product.

To download an APK when reviewing a PR:

1. click on Show All Checks,
2. click Details next to "Taskcluster (pull_request)" after it appears and then finishes with a green checkmark,
3. click on the "Fenix - assemble" task, then click "Run Artifacts".
4. the APK links should be on the left side of the screen, named for each CPU architecture

Close #11404

[firefoxci-taskcluster[bot]]

No Taskcluster jobs started for this pull request

The `allowPullRequests` configuration for this repository (in `.taskcluster.yml` on the
default branch) does not allow starting tasks for this pull request.

[Taknok]

Can a reviewer tell me if this is a correct way to introduce new strings ? As both chats fenix an l10n in chat.mozilla.org couldn't tell me if I should push it 1st on l10n or fenix project.

[Mugurell]

Can a reviewer tell me if this is a correct way to introduce new strings ? As both chats fenix an l10n in chat.mozilla.org couldn't tell me if I should push it 1st on l10n or fenix project.

All new string resources are to be added only in Fenix values/strings.xml - only to the default strings file. Localization is done automatically by a bot that will sync resources with Pontoon, we should not manually update localized strings in Fenix.

[Taknok]

Ok, so I should only set in values/strings.xml wait this string is imported in l10n, copy values on Pontoon from desktop to fenix and wait auto import here ?

[Mugurell]

Ok, so I should only set in values/strings.xml wait this string is imported in l10n, copy values on Pontoon from desktop to fenix and wait auto import here ?

Once the PR is merged with the changes in values/strings.xml the work on Fenix is done. If you want to help with translations on Pontoon you can certainly do that and would be highly appreciated but that work is outside of the scope of the Fenix ticket.

[gabrielluong]

We probably want to check with Product/UX before proceeding.

[Taknok]

I thought that the interface modification was not that important, but ok. Should I contact them, fill an issue/a form or they will pass on their own ?

[amedyne]

@topotropic or @vesta0 Any thoughts on this? @Taknok FYI. tagging them here for input.

[Taknok]

According to this documentation an engineer should contact the designer (desk or slack).

[Taknok]

Force-push to rebase PR onto main branch.

Any update about this UX review ?

[amedyne]

Talked to product managers about this PR, the request is to implement telemetry as part of this change. @gabrielluong or @Mugurell here's the Jira ticket: https://mozilla-hub.atlassian.net/browse/FNXV2-17873 for the change. Do you think you can take on this or mentor/guide @Taknok to implement?

[Taknok]

Thank you for your quick response. I cannot access the jira ticket, thus I cannot see what is asked. What should be implemented in the telemetry ? Create a special entry ? Count all tabs opened ? Just register one count even if multiples are opened ?

[amedyne]

Thank you for your quick response. I cannot access the jira ticket, thus I cannot see what is asked. What should be implemented in the telemetry ? Create a special entry ? Count all tabs opened ? Just register one count even if multiples are opened ?

The request is to register one count if a user selects the "Open All Bookmarks" action.

[Taknok]

Can I link "Open all" from the overflow menu (this PR) to the event Event.OpenedBookmarksInNewTabs which is currently used when a user selects multiple bookmark items and clicks open (which currently does not work with folders, this is why this issue and PR were created).

To sum up: Event Event.OpenedBookmarksInNewTabs would be triggered when a user opens a folder tree dots menu and click "Open all" and/or when user select multiple items (no folders), click tree dots menu from selection bar and click "open in a new bookmark".

[eliserichards]

I think we should make a new Event for open all, just so we can differentiate. What do you think @Taknok?

[eliserichards]

e.g. Event.OpenAllBookmarksInNewTabs

[Taknok]

Here is the metric with Glean.

[Taknok]

Force-push to rebase PR onto main branch.

Any update about this ?

[Taknok]

Force-push to rebase PR onto main branch. Update tests due to deletion of closeTabViaXButton.

Any update about this ?

[mergify[bot]]

This pull request has conflicts when rebasing. Could you fix it @Taknok? 🙏

[Taknok]

Fixed & force-push to rebase PR onto main branch.

[mergify[bot]]

This pull request has conflicts when rebasing. Could you fix it @Taknok? 🙏

[Taknok]

Should I wait the implementation of api and resolution in lib.rs or goes for bookmarks_get_tree for now, and evolve to new api later ?

I'm afraid that bookmarks_get_tree could slow down a lot the display of the 3 dots menu when there is a lot of elements in a folder.

[Mugurell]

Should I wait the implementation of api and resolution in lib.rs or goes for bookmarks_get_tree for now, and evolve to new api later ?

I'm afraid that bookmarks_get_tree could slow down a lot the display of the 3 dots menu when there is a lot of elements in a folder.

We can give it a try and see. We are already using the getTree method with the recursive parameter set to true in HomeActivity so it would probably be okay to use it here also. This would unblock showing the new menu option(s) only when they are appropriate - when the bookmark folder actually has bookmarks that can be opened and simplify the code a bit but avoiding that callback for a snackbar. (One small thing regarding the patch: please try to squash the commits to standalone ones, that may have sense separately. (maybe even one commit here would be best?). This will help ensure a clean history and easy navigation for future readers - having all related changes in one commit.)

Thank you for pushing through with this!

[Taknok]

We are already using the getTree method with the recursive parameter set to true in HomeActivity so it would probably be okay to use it here also.

Ok, I will try and pushed if convinced.

This would unblock showing the new menu option(s) only when they are appropriate - when the bookmark folder actually has bookmarks that can be opened and simplify the code a bit but avoiding that callback for a snackbar.

Ok.

One small thing regarding the patch: please try to squash the commits to standalone ones, [...]

I'm not sure to understand what you expect. Can you precise a quick example ? I tried todo micro commit as requested in contribution.md. Do you want something like : commit implementation, commit tests & commit androidtests ?

[Mugurell]

I'm not sure to understand what you expect. Can you precise a quick example ? I tried todo micro commit as requested in contribution.md. Do you want something like : commit implementation, commit tests & commit androidtests ?

I'd say a commit should help solve a particular a particular flow, a particular unit of app functionality. Looking through the list of current commits I see some that can be merged with the others to ensure cohesiveness such that a particular commit helps solve an entire particular scenario. For example:

• "Add string to display in overflow menu" can be merged with the one where this string is used.
• "Create core function" can be merged with the code changes that will actually call this new method.
• the work for telemetry can be similarly be a single commit, indeed separate from the functionality of opening the bookmark folder.
• tests should be part of the same commit which contains the code they test.
• static analysis fixups and addressing review comments don't need a separate commit. Ideally these should be merged with the initial commits that need those changes such that those commits will look right in the first place.

Nothing major, I know finding the right balance is hard and again appreciate your investment in this. It's great to separate unrelated functionality or break big changes in smaller units but having a commit for an entire piece of functionality also helps the reviewer and later code archaeologists understand how a feature works by having all it's needed changes in one commit, easy to reason about.

[Mugurell]

Thank you for pushing through @Taknok ! Tested it a bit manually and it works beautifully, seems like just some tests need to be updated and then do a quick last review and look into merging.

Also adding Oana as a reviewer for the ui tests.

[Taknok]

Alert message is implemented ! An alert message is displayed when more than 15 tabs are loaded. Now all requested modifications are implemented. I let you review PR.

By the way, there is no warning message for collections, even with a lot of tabs ;)

[mergify[bot]]

This pull request has conflicts when rebasing. Could you fix it @Taknok? 🙏

[Taknok]

Really T.T I merged 30min ago, and a PR 10min ago conflict T.T