Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

Allow toolbar menu buttons to determine their state asynchronously (or provide alternative) #4915

Closed
severinrudie opened this issue Oct 31, 2019 · 2 comments
Closed

Allow toolbar menu buttons to determine their state asynchronously (or provide alternative) #4915

severinrudie opened this issue Oct 31, 2019 · 2 comments
Labels
<menu> Component: concept-menu, browser-menu, browser-menu2
Projects
A-C: Android Components Sprint Planning A-C: Concept-Menu

Comments

@severinrudie
Copy link
Contributor

severinrudie commented Oct 31, 2019 •
edited by data-sync-user
Loading

Use case

As part of mozilla-mobile/fenix#4281, determining whether or not the current site is bookmarked needs to happen within TwoStateButton#isInPrimaryState. Because of how TwoStateButton works, this call must be synchronous. Unfortunately checking if the site is bookmarked is very slow (10-50MS in my tests).

The best fix would be for bookmark checks to be faster (and I've notified the perf team), but I also don't think it is unreasonable to expect that menu items may need to make slow calls to determine their state in the future. It would be good to have a canonical way of handling this case.

Fenix workaround

mozilla-mobile/fenix@572e2f6

┆Issue is synchronized with this Jira Task
@severinrudie severinrudie added the <menu> Component: concept-menu, browser-menu, browser-menu2 label Oct 31, 2019
@severinrudie severinrudie changed the title Allow toolbar menu buttons to determine their state asynchronously Allow toolbar menu buttons to determine their state asynchronously (or provide alternative) Oct 31, 2019
severinrudie added a commit to severinrudie/fenix that referenced this issue Oct 31, 2019
@severinrudie
For 4281: updated menu to show if url is bookmarked
4700ad8 
This sloppy workaround is required because TwoStateButton requires that `isInPrimaryState` be a synchronous call, and checking whether or not the current site is bookmarked is quite slow (10-50 MS, in my tests).  After days of work and many attempted solutions, this was the least abhorrent among them.

mozilla-mobile/android-components#4915 was opened against AC to evaluate potentially supporting async `isInPrimaryState` functions.
mozilla-mobile#6370 was opened against Fenix to investigate the unexpectedly slow call to `BookmarkStorage`.
severinrudie added a commit to severinrudie/fenix that referenced this issue Nov 1, 2019
@severinrudie
For 4281: updated menu to show if url is bookmarked
572e2f6 
This sloppy workaround is required because TwoStateButton requires that `isInPrimaryState` be a synchronous call, and checking whether or not the current site is bookmarked is quite slow (10-50 MS, in my tests).  After days of work and many attempted solutions, this was the least abhorrent among them.

mozilla-mobile/android-components#4915 was opened against AC to evaluate potentially supporting async `isInPrimaryState` functions.
mozilla-mobile#6370 was opened against Fenix to investigate the unexpectedly slow call to `BookmarkStorage`.
severinrudie added a commit to severinrudie/fenix that referenced this issue Nov 5, 2019
@severinrudie
For 4281: updated menu to show if url is bookmarked
c7b4957 
This sloppy workaround is required because TwoStateButton requires that `isInPrimaryState` be a synchronous call, and checking whether or not the current site is bookmarked is quite slow (10-50 MS, in my tests).  After days of work and many attempted solutions, this was the least abhorrent among them.

mozilla-mobile/android-components#4915 was opened against AC to evaluate potentially supporting async `isInPrimaryState` functions.
mozilla-mobile#6370 was opened against Fenix to investigate the unexpectedly slow call to `BookmarkStorage`.
@csadilek csadilek mentioned this issue Nov 6, 2019
Merged
severinrudie added a commit to severinrudie/fenix that referenced this issue Nov 6, 2019
@severinrudie
For 4281: updated menu to show if url is bookmarked
96bc04e 
This sloppy workaround is required because TwoStateButton requires that `isInPrimaryState` be a synchronous call, and checking whether or not the current site is bookmarked is quite slow (10-50 MS, in my tests).  After days of work and many attempted solutions, this was the least abhorrent among them.

mozilla-mobile/android-components#4915 was opened against AC to evaluate potentially supporting async `isInPrimaryState` functions.
mozilla-mobile#6370 was opened against Fenix to investigate the unexpectedly slow call to `BookmarkStorage`.
@severinrudie
Copy link
Contributor Author

It's also worth noting that, since all the logic is driven by the component, the current toolbar API doesn't play particularly well with LibState. One possible solution to this could be to have the toolbar handle its presentation details, but to expose fun update(state: ToolbarState) and allow client code to drive the logic. This could also help pave the way for a future Fenix issue that will require updating the toolbar menu while it is open.

severinrudie added a commit to severinrudie/fenix that referenced this issue Nov 7, 2019
@severinrudie
For 4281: updated menu to show if url is bookmarked
f050528 
This sloppy workaround is required because TwoStateButton requires that `isInPrimaryState` be a synchronous call, and checking whether or not the current site is bookmarked is quite slow (10-50 MS, in my tests).  After days of work and many attempted solutions, this was the least abhorrent among them.

mozilla-mobile/android-components#4915 was opened against AC to evaluate potentially supporting async `isInPrimaryState` functions.
mozilla-mobile#6370 was opened against Fenix to investigate the unexpectedly slow call to `BookmarkStorage`.
severinrudie added a commit to severinrudie/fenix that referenced this issue Nov 8, 2019
@severinrudie
For 4281: updated menu to show if url is bookmarked
5096537 
This sloppy workaround is required because TwoStateButton requires that `isInPrimaryState` be a synchronous call, and checking whether or not the current site is bookmarked is quite slow (10-50 MS, in my tests).  After days of work and many attempted solutions, this was the least abhorrent among them.

mozilla-mobile/android-components#4915 was opened against AC to evaluate potentially supporting async `isInPrimaryState` functions.
mozilla-mobile#6370 was opened against Fenix to investigate the unexpectedly slow call to `BookmarkStorage`.
severinrudie added a commit to severinrudie/fenix that referenced this issue Nov 11, 2019
@severinrudie
For 4281: updated menu to show if url is bookmarked
247c5fe 
This sloppy workaround is required because TwoStateButton requires that `isInPrimaryState` be a synchronous call, and checking whether or not the current site is bookmarked is quite slow (10-50 MS, in my tests).  After days of work and many attempted solutions, this was the least abhorrent among them.

mozilla-mobile/android-components#4915 was opened against AC to evaluate potentially supporting async `isInPrimaryState` functions.
mozilla-mobile#6370 was opened against Fenix to investigate the unexpectedly slow call to `BookmarkStorage`.
severinrudie added a commit to severinrudie/fenix that referenced this issue Nov 11, 2019
@severinrudie
For 4281: updated menu to show if url is bookmarked
92448fe 
This sloppy workaround is required because TwoStateButton requires that `isInPrimaryState` be a synchronous call, and checking whether or not the current site is bookmarked is quite slow (10-50 MS, in my tests).  After days of work and many attempted solutions, this was the least abhorrent among them.

mozilla-mobile/android-components#4915 was opened against AC to evaluate potentially supporting async `isInPrimaryState` functions.
mozilla-mobile#6370 was opened against Fenix to investigate the unexpectedly slow call to `BookmarkStorage`.
severinrudie pushed a commit to mozilla-mobile/fenix that referenced this issue Nov 12, 2019
4281 remove qab (#6310)
6909a76 
* For #4281: small ToolbarMenu refactor

This makes it easier to see how items are ordered in the menuItems list

* For 4281: add QAB buttons to menu

* For 4281: removed menu back button per mocks

I double checked with UX, and we'll be relying on the hardware back button for its functionality

* For 4281: add content descriptions for bookmarking

* For 4281: updated BrowserToolbarController for new functionality

* For 4281: provided simple dependencies to browser controller

More complex changes will be in a following commit, for review readability

* For 4281: move toolbar controller dependencies up to BaseBrowserFragment

The functionality they control is being moved into the toolbar menu, which is shared by both normal tabs and custom ones

* For 4281: removed (now unused) code related to QAB

* For 4281: fix test compilation after QAB removal

Tests still need to be expanded to include added functionality

* For 4281: updated menu to show if url is bookmarked

This sloppy workaround is required because TwoStateButton requires that `isInPrimaryState` be a synchronous call, and checking whether or not the current site is bookmarked is quite slow (10-50 MS, in my tests).  After days of work and many attempted solutions, this was the least abhorrent among them.

mozilla-mobile/android-components#4915 was opened against AC to evaluate potentially supporting async `isInPrimaryState` functions.
#6370 was opened against Fenix to investigate the unexpectedly slow call to `BookmarkStorage`.

* For 4281: update reader mode switch

* For 4281: selectively show/hide menu items

* For 4281: add reader mode appearance

* For 4281: update bookmark button when it is clicked

* For 4281: removed unused QAB code

* For 4281: removed QAB robot, updated UI tests

* For 4281: removed QuickActionSheet metrics

Since this behavior now lives in the toolbar, it is tracked via Event.BrowserMenuItemTapped

* For 4281: fixed lint errors

* For 4281: add new strings for buttons added to menu

This is necessary because the location change (from QAB to toolbar menu) could affect the grammar in some languages

* For 4281: remove outdated TODOs

* For 4281: removed QAB container

* For 4281: removed back button reference from UI test

This button no longer exists

* For 4821: Fixes a visual defect (extra padding on top of toolbar)

* For 4281: update copy on reader mode

* For 4281: fixed review nits
mozilla-l10n-automation-bot pushed a commit to mozilla-l10n-automation-bot/android-l10n that referenced this issue Nov 12, 2019
4281 remove qab (#6310)
7feb5f8 
* For #4281: small ToolbarMenu refactor

This makes it easier to see how items are ordered in the menuItems list

* For 4281: add QAB buttons to menu

* For 4281: removed menu back button per mocks

I double checked with UX, and we'll be relying on the hardware back button for its functionality

* For 4281: add content descriptions for bookmarking

* For 4281: updated BrowserToolbarController for new functionality

* For 4281: provided simple dependencies to browser controller

More complex changes will be in a following commit, for review readability

* For 4281: move toolbar controller dependencies up to BaseBrowserFragment

The functionality they control is being moved into the toolbar menu, which is shared by both normal tabs and custom ones

* For 4281: removed (now unused) code related to QAB

* For 4281: fix test compilation after QAB removal

Tests still need to be expanded to include added functionality

* For 4281: updated menu to show if url is bookmarked

This sloppy workaround is required because TwoStateButton requires that `isInPrimaryState` be a synchronous call, and checking whether or not the current site is bookmarked is quite slow (10-50 MS, in my tests).  After days of work and many attempted solutions, this was the least abhorrent among them.

mozilla-mobile/android-components#4915 was opened against AC to evaluate potentially supporting async `isInPrimaryState` functions.
mozilla-mobile/fenix#6370 was opened against Fenix to investigate the unexpectedly slow call to `BookmarkStorage`.

* For 4281: update reader mode switch

* For 4281: selectively show/hide menu items

* For 4281: add reader mode appearance

* For 4281: update bookmark button when it is clicked

* For 4281: removed unused QAB code

* For 4281: removed QAB robot, updated UI tests

* For 4281: removed QuickActionSheet metrics

Since this behavior now lives in the toolbar, it is tracked via Event.BrowserMenuItemTapped

* For 4281: fixed lint errors

* For 4281: add new strings for buttons added to menu

This is necessary because the location change (from QAB to toolbar menu) could affect the grammar in some languages

* For 4281: remove outdated TODOs

* For 4281: removed QAB container

* For 4281: removed back button reference from UI test

This button no longer exists

* For 4821: Fixes a visual defect (extra padding on top of toolbar)

* For 4281: update copy on reader mode

* For 4281: fixed review nits

X-Channel-Revision: [master] mozilla-mobile/android-components@ba3f75e
X-Channel-Converted-Revision: [master] mozilla-mobile/fenix@6909a76
X-Channel-Revision: [master] mozilla-mobile/firefox-tv@e8ccd59
X-Channel-Revision: [master] MozillaReality/FirefoxReality@ebc151c
X-Channel-Revision: [master] mozilla-lockwise/lockwise-android@e070cfc
mozilla-l10n-automation-bot pushed a commit to mozilla-l10n-automation-bot/android-l10n that referenced this issue Nov 12, 2019
4281 remove qab (#6310)
8dfb66b 
* For #4281: small ToolbarMenu refactor

This makes it easier to see how items are ordered in the menuItems list

* For 4281: add QAB buttons to menu

* For 4281: removed menu back button per mocks

I double checked with UX, and we'll be relying on the hardware back button for its functionality

* For 4281: add content descriptions for bookmarking

* For 4281: updated BrowserToolbarController for new functionality

* For 4281: provided simple dependencies to browser controller

More complex changes will be in a following commit, for review readability

* For 4281: move toolbar controller dependencies up to BaseBrowserFragment

The functionality they control is being moved into the toolbar menu, which is shared by both normal tabs and custom ones

* For 4281: removed (now unused) code related to QAB

* For 4281: fix test compilation after QAB removal

Tests still need to be expanded to include added functionality

* For 4281: updated menu to show if url is bookmarked

This sloppy workaround is required because TwoStateButton requires that `isInPrimaryState` be a synchronous call, and checking whether or not the current site is bookmarked is quite slow (10-50 MS, in my tests).  After days of work and many attempted solutions, this was the least abhorrent among them.

mozilla-mobile/android-components#4915 was opened against AC to evaluate potentially supporting async `isInPrimaryState` functions.
mozilla-mobile/fenix#6370 was opened against Fenix to investigate the unexpectedly slow call to `BookmarkStorage`.

* For 4281: update reader mode switch

* For 4281: selectively show/hide menu items

* For 4281: add reader mode appearance

* For 4281: update bookmark button when it is clicked

* For 4281: removed unused QAB code

* For 4281: removed QAB robot, updated UI tests

* For 4281: removed QuickActionSheet metrics

Since this behavior now lives in the toolbar, it is tracked via Event.BrowserMenuItemTapped

* For 4281: fixed lint errors

* For 4281: add new strings for buttons added to menu

This is necessary because the location change (from QAB to toolbar menu) could affect the grammar in some languages

* For 4281: remove outdated TODOs

* For 4281: removed QAB container

* For 4281: removed back button reference from UI test

This button no longer exists

* For 4821: Fixes a visual defect (extra padding on top of toolbar)

* For 4281: update copy on reader mode

* For 4281: fixed review nits

X-Channel-Revision: [master] mozilla-mobile/android-components@ba3f75e
X-Channel-Converted-Revision: [master] mozilla-mobile/fenix@6909a76
X-Channel-Revision: [master] mozilla-mobile/firefox-tv@e8ccd59
X-Channel-Revision: [master] MozillaReality/FirefoxReality@ebc151c
X-Channel-Revision: [master] mozilla-lockwise/lockwise-android@e070cfc
mozilla-l10n-automation-bot pushed a commit to mozilla-l10n-automation-bot/android-l10n that referenced this issue Nov 12, 2019
4281 remove qab (#6310)
83603eb 
* For #4281: small ToolbarMenu refactor

This makes it easier to see how items are ordered in the menuItems list

* For 4281: add QAB buttons to menu

* For 4281: removed menu back button per mocks

I double checked with UX, and we'll be relying on the hardware back button for its functionality

* For 4281: add content descriptions for bookmarking

* For 4281: updated BrowserToolbarController for new functionality

* For 4281: provided simple dependencies to browser controller

More complex changes will be in a following commit, for review readability

* For 4281: move toolbar controller dependencies up to BaseBrowserFragment

The functionality they control is being moved into the toolbar menu, which is shared by both normal tabs and custom ones

* For 4281: removed (now unused) code related to QAB

* For 4281: fix test compilation after QAB removal

Tests still need to be expanded to include added functionality

* For 4281: updated menu to show if url is bookmarked

This sloppy workaround is required because TwoStateButton requires that `isInPrimaryState` be a synchronous call, and checking whether or not the current site is bookmarked is quite slow (10-50 MS, in my tests).  After days of work and many attempted solutions, this was the least abhorrent among them.

mozilla-mobile/android-components#4915 was opened against AC to evaluate potentially supporting async `isInPrimaryState` functions.
mozilla-mobile/fenix#6370 was opened against Fenix to investigate the unexpectedly slow call to `BookmarkStorage`.

* For 4281: update reader mode switch

* For 4281: selectively show/hide menu items

* For 4281: add reader mode appearance

* For 4281: update bookmark button when it is clicked

* For 4281: removed unused QAB code

* For 4281: removed QAB robot, updated UI tests

* For 4281: removed QuickActionSheet metrics

Since this behavior now lives in the toolbar, it is tracked via Event.BrowserMenuItemTapped

* For 4281: fixed lint errors

* For 4281: add new strings for buttons added to menu

This is necessary because the location change (from QAB to toolbar menu) could affect the grammar in some languages

* For 4281: remove outdated TODOs

* For 4281: removed QAB container

* For 4281: removed back button reference from UI test

This button no longer exists

* For 4821: Fixes a visual defect (extra padding on top of toolbar)

* For 4281: update copy on reader mode

* For 4281: fixed review nits

X-Channel-Revision: [master] mozilla-mobile/android-components@ba3f75e
X-Channel-Converted-Revision: [master] mozilla-mobile/fenix@6909a76
X-Channel-Revision: [master] mozilla-mobile/firefox-tv@e8ccd59
X-Channel-Revision: [master] MozillaReality/FirefoxReality@ebc151c
X-Channel-Revision: [master] mozilla-lockwise/lockwise-android@e070cfc
mozilla-l10n-automation-bot pushed a commit to mozilla-l10n-automation-bot/android-l10n that referenced this issue Nov 12, 2019
4281 remove qab (#6310)
ff435ce 
* For #4281: small ToolbarMenu refactor

This makes it easier to see how items are ordered in the menuItems list

* For 4281: add QAB buttons to menu

* For 4281: removed menu back button per mocks

I double checked with UX, and we'll be relying on the hardware back button for its functionality

* For 4281: add content descriptions for bookmarking

* For 4281: updated BrowserToolbarController for new functionality

* For 4281: provided simple dependencies to browser controller

More complex changes will be in a following commit, for review readability

* For 4281: move toolbar controller dependencies up to BaseBrowserFragment

The functionality they control is being moved into the toolbar menu, which is shared by both normal tabs and custom ones

* For 4281: removed (now unused) code related to QAB

* For 4281: fix test compilation after QAB removal

Tests still need to be expanded to include added functionality

* For 4281: updated menu to show if url is bookmarked

This sloppy workaround is required because TwoStateButton requires that `isInPrimaryState` be a synchronous call, and checking whether or not the current site is bookmarked is quite slow (10-50 MS, in my tests).  After days of work and many attempted solutions, this was the least abhorrent among them.

mozilla-mobile/android-components#4915 was opened against AC to evaluate potentially supporting async `isInPrimaryState` functions.
mozilla-mobile/fenix#6370 was opened against Fenix to investigate the unexpectedly slow call to `BookmarkStorage`.

* For 4281: update reader mode switch

* For 4281: selectively show/hide menu items

* For 4281: add reader mode appearance

* For 4281: update bookmark button when it is clicked

* For 4281: removed unused QAB code

* For 4281: removed QAB robot, updated UI tests

* For 4281: removed QuickActionSheet metrics

Since this behavior now lives in the toolbar, it is tracked via Event.BrowserMenuItemTapped

* For 4281: fixed lint errors

* For 4281: add new strings for buttons added to menu

This is necessary because the location change (from QAB to toolbar menu) could affect the grammar in some languages

* For 4281: remove outdated TODOs

* For 4281: removed QAB container

* For 4281: removed back button reference from UI test

This button no longer exists

* For 4821: Fixes a visual defect (extra padding on top of toolbar)

* For 4281: update copy on reader mode

* For 4281: fixed review nits

X-Channel-Revision: [master] mozilla-mobile/android-components@ba3f75e
X-Channel-Converted-Revision: [master] mozilla-mobile/fenix@6909a76
X-Channel-Revision: [master] mozilla-mobile/firefox-tv@e8ccd59
X-Channel-Revision: [master] MozillaReality/FirefoxReality@ebc151c
X-Channel-Revision: [master] mozilla-lockwise/lockwise-android@e070cfc
mozilla-l10n-automation-bot pushed a commit to mozilla-l10n-automation-bot/android-l10n that referenced this issue Nov 12, 2019
4281 remove qab (#6310)
6c0d742 
* For #4281: small ToolbarMenu refactor

This makes it easier to see how items are ordered in the menuItems list

* For 4281: add QAB buttons to menu

* For 4281: removed menu back button per mocks

I double checked with UX, and we'll be relying on the hardware back button for its functionality

* For 4281: add content descriptions for bookmarking

* For 4281: updated BrowserToolbarController for new functionality

* For 4281: provided simple dependencies to browser controller

More complex changes will be in a following commit, for review readability

* For 4281: move toolbar controller dependencies up to BaseBrowserFragment

The functionality they control is being moved into the toolbar menu, which is shared by both normal tabs and custom ones

* For 4281: removed (now unused) code related to QAB

* For 4281: fix test compilation after QAB removal

Tests still need to be expanded to include added functionality

* For 4281: updated menu to show if url is bookmarked

This sloppy workaround is required because TwoStateButton requires that `isInPrimaryState` be a synchronous call, and checking whether or not the current site is bookmarked is quite slow (10-50 MS, in my tests).  After days of work and many attempted solutions, this was the least abhorrent among them.

mozilla-mobile/android-components#4915 was opened against AC to evaluate potentially supporting async `isInPrimaryState` functions.
mozilla-mobile/fenix#6370 was opened against Fenix to investigate the unexpectedly slow call to `BookmarkStorage`.

* For 4281: update reader mode switch

* For 4281: selectively show/hide menu items

* For 4281: add reader mode appearance

* For 4281: update bookmark button when it is clicked

* For 4281: removed unused QAB code

* For 4281: removed QAB robot, updated UI tests

* For 4281: removed QuickActionSheet metrics

Since this behavior now lives in the toolbar, it is tracked via Event.BrowserMenuItemTapped

* For 4281: fixed lint errors

* For 4281: add new strings for buttons added to menu

This is necessary because the location change (from QAB to toolbar menu) could affect the grammar in some languages

* For 4281: remove outdated TODOs

* For 4281: removed QAB container

* For 4281: removed back button reference from UI test

This button no longer exists

* For 4821: Fixes a visual defect (extra padding on top of toolbar)

* For 4281: update copy on reader mode

* For 4281: fixed review nits

X-Channel-Revision: [master] mozilla-mobile/android-components@ba3f75e
X-Channel-Converted-Revision: [master] mozilla-mobile/fenix@6909a76
X-Channel-Revision: [master] mozilla-mobile/firefox-tv@e8ccd59
X-Channel-Revision: [master] MozillaReality/FirefoxReality@ebc151c
X-Channel-Revision: [master] mozilla-lockwise/lockwise-android@e070cfc
@NotWoods NotWoods added this to In Progress in A-C: Concept-Menu Jul 23, 2020
@NotWoods
Copy link
Contributor

Closing because this is offered in menu2 (#5373)

@NotWoods NotWoods added this to 🏁 Done in A-C: Android Components Sprint Planning via automation Jul 24, 2020
@NotWoods NotWoods moved this from In Progress to Done in A-C: Concept-Menu Jul 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
<menu> Component: concept-menu, browser-menu, browser-menu2
Projects
No open projects
A-C: Android Components Sprint Planning
  
🏁 Done
A-C: Concept-Menu
  
Done
Milestone
No milestone
Development

No branches or pull requests
2 participants
@NotWoods @severinrudie