Closed Bug 1030470 Opened 5 years ago Closed 5 years ago

[AccessFu] Accessfu.properties localization needs to work in Gaia.

Categories

(Core :: Disability Access APIs, defect)

All
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: yzen, Assigned: yzen)

References

Details

(Keywords: intl)

Attachments

(2 files, 8 obsolete files)

Accessfu message bundles are not localized in Gaia. We need a mechanism of having the l10n working on the FxOS device.

Note: consider accessibility service in gaia to potentially address this.
OS: All → Gonk (Firefox OS)
Summary: [AccessFu] Localization needs to work. → [AccessFu] Accessfu.properties localization needs to work in Gaia.
Assignee: nobody → yzenevich
Attached patch 1030470 patch v1 (obsolete) — Splinter Review
First pass.
Attachment #8460419 - Flags: feedback?(eitan)
Depends on: 1042944
Attached patch 1030470 patch v2 (obsolete) — Splinter Review
Changed the name from accessfu-speech to accessfu-actions. Removed enqueue: true for speech presenter's pivot change.
Attachment #8460419 - Attachment is obsolete: true
Attachment #8460419 - Flags: feedback?(eitan)
Attachment #8461776 - Flags: feedback?(eitan)
Comment on attachment 8461776 [details] [diff] [review]
1030470 patch v2

Review of attachment 8461776 [details] [diff] [review]:
-----------------------------------------------------------------

yzen, i have 3 thoughts about this patch
 1. it is hard to read with the jslint :P could you make that a preliminary patch?
 2. i think localization should get out of OutputGenerator entirely, it is confusing to have it both there and in presenter
 3. yeah, sorry i said otherwise before, but i think we need to think of a "protocol" format of the data we throw at gaia
 and gaia should do speech/earcon/haptic and do a native gaia toaster for announcements
<yzen> ya i can do that 
 icaaq is now known as icaaq|afk
<eeejay> i think the fields would be:
 1. output-type: that would be things like vc-change, state-change, text-change, announcement, etc
 and then gaia would determine what earcon it gets, if it gets haptic, etc.
 the special case would be announcement, it would trigger a toaster, which in turn would send another liveregion change event
<yzen> eeejay right so that's the idea
<yzen> im  a little confused what you mean about l10n in OutputGenerator, afaik, there's nothing there any more
<eeejay> hm
<yzen> eeejay ok let me split the patch from linting , it will be easier to read
<eeejay> yzen, also. i have second thoughts about the data format..
 yzen, maybe it is worth having the gaia end as dumb as possible so that we have more control in our own module
 yzen, don't want to ask for review from gaia devs when we want to change the haptic feedback
 yzen, the l10n in the generator is the _addLocalized{State,Role}
 yzen, at the end of the day (this could be done in one patch), we should have 3 presenters: Visual, Android, B2G
 And the Visual could one day go away
<yzen> eeejay right that's an old name, ill update
 eeejay ok ill update the pr with more changes
<eeejay> pr, haha
Attachment #8461776 - Flags: feedback?(eitan)
Attached patch 1030470.1 patch 1 (obsolete) — Splinter Review
Attachment #8461776 - Attachment is obsolete: true
Attachment #8462744 - Flags: review?(eitan)
Attached patch 1030470.2 patch 1 (obsolete) — Splinter Review
Attachment #8462745 - Flags: review?(eitan)
Comment on attachment 8462744 [details] [diff] [review]
1030470.1 patch 1

Review of attachment 8462744 [details] [diff] [review]:
-----------------------------------------------------------------

Overall looks good. I am not a fan of the details field being an array. It then makes retrieving details assume that there is a 0 index, like you do in the Visual presenter. Also, that was a flaw in the Speech presenter that I would like to see go away. Do we ever have more than one?

::: accessible/jsat/Presentation.jsm
@@ +440,3 @@
>    __proto__: Presenter.prototype,
> +  type: 'B2G',
> +  PIVOT_CHANGE_PATTERN: [40],

this is haptic feedback? gotta be a more descriptive name for that.

@@ +450,5 @@
> +      details: [{
> +        eventType: 'vc-change',
> +        data: UtteranceGenerator.genForContext(aContext),
> +        options: {
> +          enqueue: false,

maybe enqueue should implicitly always be false unless it is present/true?

@@ +452,5 @@
> +        data: UtteranceGenerator.genForContext(aContext),
> +        options: {
> +          enqueue: false,
> +          pattern: this.PIVOT_CHANGE_PATTERN,
> +          isKey: aContext.accessible.role === Roles.KEY

how about adding vc change reason as well. We may want to present differently if it is ebt, swipe, or auto-moved.

::: accessible/jsat/Utils.jsm
@@ +188,5 @@
>      this.isContentProcess =
>        Services.appinfo.processType == Services.appinfo.PROCESS_TYPE_CONTENT;
>      return this.isContentProcess;
>    },
> +  sendChromeEvent: function sendChromeEvent(aDetails) {

1. The name is misleading, you are doing more here than simply sending an event with given details.
2. Isn't this only used in AccessFu.jsm? If so, I think it is more appropriate for this to be Output.B2G.

@@ +195,5 @@
> +      details: JSON.stringify(aDetails)
> +    };
> +    if (this.win.shell) {
> +      this.win.shell.sendChromeEvent(details);
> +    } else {

Assuming this is for supporting desktop browser, should be a comment here with that.

@@ +206,5 @@
> +  },
> +
> +  localize: function localize(aOutput) {
> +    if (!Array.isArray(aOutput)) {
> +      aOutput = [aOutput];

don't re-assign arguments.

@@ +219,2 @@
>    get stringBundle() {
> +    let bundle = Services.strings.createBundle(

You stopped caching the bundle. Is that intentional?

@@ +241,5 @@
> +          if (count) {
> +            str = PluralForm.get(count, str);
> +            str = str.replace('#1', count);
> +          }
> +          return str;

What if there is both a 'count' and an 'args' field? Looks like the count will clobber the args string. Should they co-exist. Also, I'll sleep better if str is initialized as an empty string.

::: dom/locales/en-US/chrome/accessibility/AccessFu.properties
@@ +184,5 @@
>  quicknav_Separator   = Separators
>  quicknav_Table       = Tables
>  quicknav_Checkbox    = Check boxes
>  # Shortened role names for braille
> +menubarAbbr        =       menu bar

Do we really need to copy all the roles here even when they are not abbreviated? Can't we programmatically fall back on the non-abbreviated role name when an abbreviated one does not exist?
Attachment #8462744 - Flags: review?(eitan)
Comment on attachment 8462745 [details] [diff] [review]
1030470.2 patch 1

Review of attachment 8462745 [details] [diff] [review]:
-----------------------------------------------------------------

Auto-approved jslinted files.
Attachment #8462745 - Flags: review?(eitan) → review+
(In reply to Eitan Isaacson [:eeejay] from comment #6)
> Comment on attachment 8462744 [details] [diff] [review]
> 1030470.1 patch 1
> 
> Review of attachment 8462744 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Overall looks good. I am not a fan of the details field being an array. It
> then makes retrieving details assume that there is a 0 index, like you do in
> the Visual presenter. Also, that was a flaw in the Speech presenter that I
> would like to see go away. Do we ever have more than one?

As discussed in IRC, we'll normalize with Android and will get rid of Visual presenter as soon as possible.

> 
> ::: accessible/jsat/Presentation.jsm
> @@ +440,3 @@
> >    __proto__: Presenter.prototype,
> > +  type: 'B2G',
> > +  PIVOT_CHANGE_PATTERN: [40],
> 
> this is haptic feedback? gotta be a more descriptive name for that.

Added comment.

> 
> @@ +450,5 @@
> > +      details: [{
> > +        eventType: 'vc-change',
> > +        data: UtteranceGenerator.genForContext(aContext),
> > +        options: {
> > +          enqueue: false,
> 
> maybe enqueue should implicitly always be false unless it is present/true?

Done.

> 
> @@ +452,5 @@
> > +        data: UtteranceGenerator.genForContext(aContext),
> > +        options: {
> > +          enqueue: false,
> > +          pattern: this.PIVOT_CHANGE_PATTERN,
> > +          isKey: aContext.accessible.role === Roles.KEY
> 
> how about adding vc change reason as well. We may want to present
> differently if it is ebt, swipe, or auto-moved.

Done.

> 
> ::: accessible/jsat/Utils.jsm
> @@ +188,5 @@
> >      this.isContentProcess =
> >        Services.appinfo.processType == Services.appinfo.PROCESS_TYPE_CONTENT;
> >      return this.isContentProcess;
> >    },
> > +  sendChromeEvent: function sendChromeEvent(aDetails) {
> 
> 1. The name is misleading, you are doing more here than simply sending an
> event with given details.
> 2. Isn't this only used in AccessFu.jsm? If so, I think it is more
> appropriate for this to be Output.B2G.

Done.

> 
> @@ +195,5 @@
> > +      details: JSON.stringify(aDetails)
> > +    };
> > +    if (this.win.shell) {
> > +      this.win.shell.sendChromeEvent(details);
> > +    } else {
> 
> Assuming this is for supporting desktop browser, should be a comment here
> with that.

Done.

> 
> @@ +206,5 @@
> > +  },
> > +
> > +  localize: function localize(aOutput) {
> > +    if (!Array.isArray(aOutput)) {
> > +      aOutput = [aOutput];
> 
> don't re-assign arguments.

Fixed.

> 
> @@ +219,2 @@
> >    get stringBundle() {
> > +    let bundle = Services.strings.createBundle(
> 
> You stopped caching the bundle. Is that intentional?

From what I know , unless you delete the property in the getter it is only evaluated once?

> 
> @@ +241,5 @@
> > +          if (count) {
> > +            str = PluralForm.get(count, str);
> > +            str = str.replace('#1', count);
> > +          }
> > +          return str;
> 
> What if there is both a 'count' and an 'args' field? Looks like the count
> will clobber the args string. Should they co-exist. 

Not entirely sure.. Args are used to expand %S placeholders, and Plural form expands #1. It also looks like there's never a case where args and count are used together..

> Also, I'll sleep better
> if str is initialized as an empty string.

Fixed.

> 
> ::: dom/locales/en-US/chrome/accessibility/AccessFu.properties
> @@ +184,5 @@
> >  quicknav_Separator   = Separators
> >  quicknav_Table       = Tables
> >  quicknav_Checkbox    = Check boxes
> >  # Shortened role names for braille
> > +menubarAbbr        =       menu bar
> 
> Do we really need to copy all the roles here even when they are not
> abbreviated? Can't we programmatically fall back on the non-abbreviated role
> name when an abbreviated one does not exist?

We'd have to localize it as part of the logic and have the same code both in gecko and gaia. I think it's OK to have Abbr version for all roles but if you definitely prefer the other approach I can change that?
Attached patch 1030470.1 patch 2 (obsolete) — Splinter Review
Comments addressed
Attachment #8462744 - Attachment is obsolete: true
Attachment #8462844 - Flags: review?(eitan)
Attached patch 1030470.2 patch 2 (obsolete) — Splinter Review
Carrying over r+ from Eitan.
Attachment #8462745 - Attachment is obsolete: true
Attached patch 1030470.1 patch v3 (obsolete) — Splinter Review
Comments addressed.
Attachment #8462844 - Attachment is obsolete: true
Attachment #8462844 - Flags: review?(eitan)
Attached patch 1030470.2 patch v3 (obsolete) — Splinter Review
Carrying over the r+
Attachment #8462845 - Attachment is obsolete: true
Attachment #8463372 - Flags: review?(eitan)
Comment on attachment 8463372 [details] [diff] [review]
1030470.1 patch v3

Review of attachment 8463372 [details] [diff] [review]:
-----------------------------------------------------------------

This looks great. I have no further feedback! We should wait for the gaia bits to land and settle before you land this.
Attachment #8463372 - Flags: review?(eitan) → review+
Carrying over r+ from eeejay.
Attachment #8463372 - Attachment is obsolete: true
Carrying over r+ from eeejay.
Attachment #8463373 - Attachment is obsolete: true
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Should keep it open until it lands in m-c
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/d6059e806022
https://hg.mozilla.org/mozilla-central/rev/860146d2669f
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.