This repository has been archived by the owner on Feb 20, 2023. It is now read-only.

For #5295 [A11y] Unable to tap snackbar #6513

Merged

bors merged 1 commit into mozilla-mobile:master from kglazko:Issue5295UnableToTapSnackbar

Nov 15, 2019

Contributor

kglazko commented Nov 8, 2019

Pull Request checklist

Quality: This PR builds and passes detekt/ktlint checks (A pre-push hook is recommended)
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

After merge

Milestone: Make sure issues finished by this pull request are added to the milestone of the version currently in development.

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-io commented Nov 8, 2019 •

edited

Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@ee4e1c8). Click here to learn what that means.
The diff coverage is 0%.

@@            Coverage Diff            @@
##             master    #6513   +/-   ##
=========================================
  Coverage          ?   16.75%           
  Complexity        ?      349           
=========================================
  Files             ?      271           
  Lines             ?    10667           
  Branches          ?     1484           
=========================================
  Hits              ?     1787           
  Misses            ?     8733           
  Partials          ?      147

Impacted Files	Coverage Δ	Complexity Δ
app/src/main/java/org/mozilla/fenix/utils/Undo.kt	`0% <0%> (ø)`	`0 <0> (?)`

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 ee4e1c8...e038e0c. Read the comment docs.

kglazko changed the title ~~Issue 5295 [A11y] Unable to tap snackbar~~ WIP For #5295 [A11y] Unable to tap snackbar

severinrudie reviewed

View reviewed changes

app/src/main/java/org/mozilla/fenix/utils/Undo.kt Outdated

-                      .setAction(undoActionTitle) {
-                          requestedUndo.set(true)
+                  if (isAccessibilityEnabled) {
+                      // CONFIRMATION DIALOG: TO-D0 CHECK IF THERE IS TALK BACK VIA ACCESSIBILITY THEN DO THIS

Contributor

severinrudie Nov 12, 2019

The code looks good, but these blocks have gotten big enough that they're getting a little tough to follow. I think this would be easier if they were broken into helper functions, so we could get:

if (isAccessibilityEnabled) {
  showUndoDialog()
else {
  showUndoSnackbar()
}

kglazko force-pushed the Issue5295UnableToTapSnackbar branch from 3591a9a to 4203614 Compare

November 12, 2019 23:31

kglazko requested a review from severinrudie

November 12, 2019 23:45

severinrudie approved these changes

View reviewed changes

Contributor

severinrudie left a comment

Code looks good to me. I have a few minor nits, but the structure is solid.

app/src/main/java/org/mozilla/fenix/utils/Undo.kt Outdated

+                  val isAccessibilityEnabled = accessibilityEnabled(view)
+                  // Show either Snackbar or dialog based on result
+                  if (isAccessibilityEnabled) showUndoDialog() else showUndoSnackbar()

Contributor

severinrudie Nov 13, 2019

This is a small project style thing, but would you remind reformatting this into:

if (isAccessibilityEnabled) {
  showUndoDialog()
} else {
  showUndoSnackbar()
}

app/src/main/java/org/mozilla/fenix/utils/Undo.kt Outdated

                       }
                   }
+                  // Check if accessibility is enabled
+                  val isAccessibilityEnabled = accessibilityEnabled(view)

Contributor

severinrudie Nov 13, 2019

This line doesn't seem to help readabbility too much. Maybe just call accessibilityEnabled(view) inside the if statement on 96?

app/src/main/java/org/mozilla/fenix/utils/Undo.kt Outdated

+                  // Check if accessibility is enabled
+                  val isAccessibilityEnabled = accessibilityEnabled(view)
+                  // Show either Snackbar or dialog based on result

Contributor

severinrudie Nov 13, 2019

There's a maxim on code comments that says they should explain how, not what. I don't think we need this comment, but it probably would be worth explaining why we're doing this in the first place. Would you mind replacing this with something like // It is difficult to use our Snackbars quickly enough with accessibility settings enabled, so in that case we show a dialog instead?

app/src/main/java/org/mozilla/fenix/utils/Undo.kt Outdated

-                      .setAnchorView(anchorView)
-                      .setAction(undoActionTitle) {
-                          requestedUndo.set(true)
+                  // Launch an alert dialog for undo- for accessibility users

Contributor

severinrudie Nov 13, 2019

This is the kind of comment that could age poorly. If we explain the accessibility problem down where we're conditioning on accessibility mode, hopefully it will be removed if that ever goes out of date. However, it's not unreasonable to think that some other chunk of code could someday call showUndoDialog for a use case that has nothing to do with accessibility. At that point, this comment will have rotted unintentionally. I would remove this one entirely.

Commenting is really hard.

app/src/main/java/org/mozilla/fenix/utils/Undo.kt Outdated

-                  // If user engages with the snackbar, it'll get automatically dismissed.
-                  snackbar.show()
+                  // Launch an indefinite snackbar

Contributor

severinrudie Nov 13, 2019

I think we could probably get rid of this comment

app/src/main/java/org/mozilla/fenix/utils/Undo.kt Outdated

+              fun accessibilityEnabled(view:View): Boolean {
+                  val am = view.context.getSystemService(Context.ACCESSIBILITY_SERVICE) as AccessibilityManager
+                  return am.isEnabled

Contributor

severinrudie Nov 13, 2019

Good helper function, this really improves the clarity.

Normally I think it's good to narrow parameters as much as possible (passing Context is better than View, AccessibilityManager is better than Context). There are a lot of good reasons for doing that, but in this instance the lookup is all the function is really doing. So this is probably good as is! No changes needed.

app/src/main/java/org/mozilla/fenix/utils/Undo.kt Outdated

+                              }
+                          }
+                      // If user engages with the snackbar, it'll get automatically dismissed.

Contributor

severinrudie Nov 13, 2019

I think we could probably get rid of this comment

kglazko force-pushed the Issue5295UnableToTapSnackbar branch 3 times, most recently from 2ccd1f6 to 0b53107 Compare

November 14, 2019 19:25

Contributor Author

kglazko commented Nov 14, 2019

Screenshot GIF

kglazko force-pushed the Issue5295UnableToTapSnackbar branch 4 times, most recently from 5d4ce61 to 5b31dd8 Compare

November 14, 2019 21:13

e038e0c

kglazko force-pushed the Issue5295UnableToTapSnackbar branch from 3d5be32 to e038e0c Compare

November 14, 2019 21:54

kglazko requested review from brampitoyo and removed request for brampitoyo

November 14, 2019 22:09

kglazko changed the title ~~WIP For #5295 [A11y] Unable to tap snackbar~~ For #5295 [A11y] Unable to tap snackbar

Contributor Author

kglazko commented Nov 14, 2019

@lime124 See attached GIF above

Collaborator

lime124 commented Nov 15, 2019

@AmyYLee can you check this out tomorrow? thanks!

lime124 assigned AmyYLee

Collaborator

AmyYLee commented Nov 15, 2019

Screenshot GIF

This looks good to me @kglazko Thanks!

Contributor Author

kglazko commented Nov 15, 2019

bors r+

bors bot pushed a commit that referenced this pull request


          Merge #6513

204ead0

6513: For #5295 [A11y] Unable to tap snackbar r=kglazko a=kglazko



Co-authored-by: Kate Glazko <katglazko@gmail.com>

bors bot commented Nov 15, 2019

Build succeeded

build-debug
test-debug
lint-detekt
lint-ktlint
test-ui
lint-compare-locales
lint-lint

bors bot merged commit e038e0c into mozilla-mobile:master

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet