1399294 - [a11y] Activity stream topsites does not announce when a top site is pinned

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: Activity Stream, defect, P3)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

A non-visual user has no way of knowing if a top site is pinned except to remember or to open the context menu and hover over, "Pin site" or "Unpin site". This doesn't seem great.

Comment 1

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

There are bigger a11y Talkback issues so let's focus on those.

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

[eng triage recommendation] P2: there are much bigger Talkback issues with top sites but it'd be great to help users use pinned sites.

Comment 3

[Chenxia Liu [:liuche]]

After talking to :eejay, this is probably a small fix and would be very helpful for users.

Only concern is if we need new strings as the Talkback announcement, in which case this needs to be with 58 because of string freeze. If we can reuse strings, then this is important enough to have an uplift. Changing to P1, dependent on strings.

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Bug 1399294: Add contentDescription for pinned top sites.

Since this is intended to be uplifted, I was unable to add a String - I filed
bug 1402115 to correct this.

This String is used for non-visual users as a hint when focusing a top sites
tile that the top sites they focused is pinned. In english, this would be:

    "Pinned site, <top-site-tile-name>"

For non-pinned sites, it will be just the top site tile name.

I chose this existing String because it was originally used in the following
context (according to the l10n note): "This is a snackbar label displayed after
a site is pinned or unpinned." Roughly, I take this to be "The site you have
chosen has be pinned." This somewhat makes sense in the top sites tiles, where
the user has previously pinned this site and we're now notifying them of that.

Review commit: https://reviewboard.mozilla.org/r/182392/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/182392/

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Delphine, is it okay that I'm re-using a String that doesn't have the same context? If so, how do you feel about the String I have chosen [1] to represent this value? Please note that I'm trying to get this into FF57.

[1]: http://searchfox.org/mozilla-central/rev/2ef8bd8a46a02c68ddbb1d5f25fa254dd7be1fbd/mobile/android/base/locales/en-US/android_strings.dtd#621

Comment 6

[Chenxia Liu [:liuche]]

Comment on attachment 8910933 [details]
Bug 1399294: Add contentDescription for pinned top sites.

https://reviewboard.mozilla.org/r/182392/#review187734

Nice, now that we're on 16+ we can use importantForAccessibility!

A quick search seems to show that this is fine when the view is GONE (e.g., TalkBack won't announce it)

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

NI self to follow-up with Delphine and (hopefully! :) land.

Comment 8

[Delphine Lebédel [:delphine - Use needinfo]]

I almost thought this would work - but actually, there are languages where this won't. The context is too different. So unfortunately, we can't re-use a string. So at this point, we'll have to wait for next cycle :/ Sorry!

Comment 9

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Okay, moving to FF58 then. I'm going to prioritize 57 issues right now so finishing this up (i.e. adding the string) will have to wait.

Comment 11

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

To be explicit, the action item here is take the patch, add a new string for the contentDescription (rather than using the existing string), and land it. You should probably talk to UX about what that string should be.

Comment 12

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

All open Activity Stream bugs are moving from the whiteboard tag, "[mobileAS]", to the Firefox for Android component, "Activity Stream", so that I can keep better track of these bugs as the new triage owner; I will send out an email shortly with additional details, caveats, etc.

Comment 13

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

[triage] Bulk edit from title: this is a non-critical issue. Please remove priority if you wish this to be re-triaged.

Comment 14

[BMO Automation]

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.