Closes #18816: Disable TabsTray FAB on accessibility enabled #19170

rocketsroger · 2021-04-21T19:57:54Z

Pull Request checklist

Tests: This PR includes thorough tests or an explanation of why it does not
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:

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

codecov-commenter · 2021-04-21T20:19:42Z

Codecov Report

Merging #19170 (426a58d) into master (cf4847d) will increase coverage by 0.12%.
The diff coverage is 60.00%.

❗ Current head 426a58d differs from pull request most recent head 43d8d72. Consider uploading reports for the commit 43d8d72 to get more accurate results

@@             Coverage Diff              @@
##             master   #19170      +/-   ##
============================================
+ Coverage     34.65%   34.78%   +0.12%     
- Complexity     1639     1643       +4     
============================================
  Files           541      542       +1     
  Lines         21946    21971      +25     
  Branches       3283     3285       +2     
============================================
+ Hits           7606     7642      +36     
+ Misses        13441    13426      -15     
- Partials        899      903       +4

Impacted Files	Coverage Δ	Complexity Δ
...ava/org/mozilla/fenix/tabstray/TabsTrayFragment.kt	`0.00% <0.00%> (ø)`	`0.00 <0.00> (ø)`
.../org/mozilla/fenix/tabstray/NewTabButtonBinding.kt	`71.87% <71.87%> (ø)`	`7.00 <7.00> (?)`
...illa/fenix/tabstray/FloatingActionButtonBinding.kt	`76.31% <90.90%> (+76.31%)`	`7.00 <5.00> (+7.00)`
...settings/creditcards/CreditCardsSettingFragment.kt	`0.00% <0.00%> (-23.34%)`	`0.00% <0.00%> (-7.00%)`
...main/java/org/mozilla/fenix/components/UseCases.kt	`75.00% <0.00%> (-3.95%)`	`1.00% <0.00%> (ø%)`
...rg/mozilla/fenix/settings/account/AccountUiView.kt	`34.14% <0.00%> (-2.44%)`	`4.00% <0.00%> (ø%)`
...src/main/java/org/mozilla/fenix/components/Core.kt	`20.00% <0.00%> (-0.61%)`	`2.00% <0.00%> (-2.00%)`
...pp/src/main/java/org/mozilla/fenix/HomeActivity.kt	`24.31% <0.00%> (ø)`	`28.00% <0.00%> (ø%)`
...in/java/org/mozilla/fenix/perf/NavGraphProvider.kt	`62.50% <0.00%> (ø)`	`2.00% <0.00%> (-1.00%)`
.../mozilla/fenix/search/awesomebar/AwesomeBarView.kt	`0.00% <0.00%> (ø)`	`0.00% <0.00%> (ø%)`
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3b3e4c0...43d8d72. Read the comment docs.

jonalmeida

Looks good! Some minor nits, but nothing blocking.

jonalmeida · 2021-04-21T20:55:47Z

app/src/main/java/org/mozilla/fenix/tabstray/NewTabButtonBinding.kt

+import org.mozilla.fenix.tabstray.syncedtabs.SyncedTabsInteractor
+import org.mozilla.fenix.utils.Settings
+
+class NewTabButtonBinding(


nit: Since we're making this solely for accessibility, maybe we can name it as such?

Maybe, AccessibleNewTabButtonBinding? That might catch the right-person's eye if they're confused.

Sure, I'll update.

jonalmeida · 2021-04-21T20:58:20Z

app/src/main/java/org/mozilla/fenix/tabstray/NewTabButtonBinding.kt

+        if (!settings.accessibilityServicesEnabled) {
+            newTabButton.visibility = View.GONE
+            return
+        }


Could we lift this up to the caller (start) and put it above the store.flowScoped line? I think that makes it clearer that this will only be performed on start vs. a "setFab" that doesn't always set.

Good idea. Since TabsTrayFragment will call start every time there's really no reason for both buttons to observe the store changes. Thanks

Agreed. I'll update. Thanks

jonalmeida · 2021-04-21T21:02:00Z

app/src/main/java/org/mozilla/fenix/tabstray/NewTabButtonBinding.kt

+    }
+
+    private fun setFab(selectedPage: Page, syncing: Boolean) {
+        /* Do not show fab when accessibility service is enabled */


This binding is coupled with the FloatingActionButtonBinding, so let's reference in this comment here, or better as a kdoc for the class, to take a look at the other one?

Related: I've been using the symbol matching in KDocs to ensure that related classes can be found through Android Studio and other symbolic referencing tools, like rename, safe-delete, etc.

You can reference the class in the KDocs with [FloatingActionButtonBinding] and you should be able to control-click on it to go to that class from the docs.

Sure, I'll move this comment on both class to the top in KDocs. They will reference each other 👍

jonalmeida · 2021-04-21T21:02:31Z

app/src/main/java/org/mozilla/fenix/tabstray/FloatingActionButtonBinding.kt

+        if (settings.accessibilityServicesEnabled) {
+            actionButton.hide()
+            return
+        }


See below comment about lifting this up to the calling method.

jonalmeida · 2021-04-21T21:03:13Z

app/src/test/java/org/mozilla/fenix/tabstray/FloatingActionButtonBindingTest.kt

@@ -0,0 +1,170 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public


Thank you for going out of the way to write tests for this! ✨ ✨ ✨

jonalmeida · 2021-04-21T21:09:53Z

app/src/main/java/org/mozilla/fenix/tabstray/TabsTrayFragment.kt

@@ -312,7 +327,7 @@ class TabsTrayFragment : AppCompatDialogFragment(), TabsTrayInteractor {
            },
            operation = { },
            elevation = ELEVATION,
-            anchorView = new_tab_button
+            anchorView = if (new_tab_button.isVisible) new_tab_button else null


…abled

rocketsroger added the needs:review PRs that need to be reviewed label Apr 21, 2021

rocketsroger requested review from jonalmeida and codrut-topliceanu April 21, 2021 19:57

rocketsroger requested review from a team as code owners April 21, 2021 19:57

jonalmeida approved these changes Apr 21, 2021

View reviewed changes

Closes mozilla-mobile#18816: Disable TabsTray FAB on accessibility en…

43d8d72

…abled

rocketsroger force-pushed the FB_18816 branch from 426a58d to 43d8d72 Compare April 22, 2021 15:24

rocketsroger closed this Apr 22, 2021

rocketsroger reopened this Apr 22, 2021

rocketsroger merged commit cd37f39 into mozilla-mobile:master Apr 22, 2021

rocketsroger deleted the FB_18816 branch April 22, 2021 16:48

rocketsroger removed the needs:review PRs that need to be reviewed label Apr 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Closes #18816: Disable TabsTray FAB on accessibility enabled #19170

Closes #18816: Disable TabsTray FAB on accessibility enabled #19170

rocketsroger commented Apr 21, 2021 •

edited by gabrielluong

Loading

codecov-commenter commented Apr 21, 2021 •

edited

Loading

jonalmeida left a comment

jonalmeida Apr 21, 2021

rocketsroger Apr 21, 2021

jonalmeida Apr 21, 2021

rocketsroger Apr 21, 2021

rocketsroger Apr 22, 2021

jonalmeida Apr 21, 2021

rocketsroger Apr 22, 2021

jonalmeida Apr 21, 2021

jonalmeida Apr 21, 2021

jonalmeida Apr 21, 2021

		@@ -0,0 +1,170 @@
		/* This Source Code Form is subject to the terms of the Mozilla Public

Closes #18816: Disable TabsTray FAB on accessibility enabled #19170

Closes #18816: Disable TabsTray FAB on accessibility enabled #19170

Conversation

rocketsroger commented Apr 21, 2021 • edited by gabrielluong Loading

Pull Request checklist

To download an APK when reviewing a PR:

codecov-commenter commented Apr 21, 2021 • edited Loading

Codecov Report

jonalmeida left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rocketsroger commented Apr 21, 2021 •

edited by gabrielluong

Loading

codecov-commenter commented Apr 21, 2021 •

edited

Loading