1097827 - Move manual setting of aria-label in JS code to use l10nId

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Gaia, defect)

Description

[Zibi Braniecki [:zbraniecki][:gandalf]]

We currently have about 40 cases with code like this:

```
node.setAttribute('aria-label', navigator.mozL10n.get('foo');

foo = Aria Value
```

This code should be replaced with:

```
node.setAttribute('data-l10n-id', 'foo');

foo.ariaLabel = AriaValue
```

You can find this code using the following grep:

grep -I --exclude *.html -r "'aria-label'" ./apps/

Comment 1

[Zibi Braniecki [:zbraniecki][:gandalf]]

Flod, just CC'ing you to put this on your tracker in case someone will take it.

With landing in bug 1091113 this should not result in any UI regressions.

Comment 2

[Guilherme Uhelski]

Can I take this one?

Comment 3

[Zibi Braniecki [:zbraniecki][:gandalf]]

Sure! Please, limit your first patch to one app. Depending on how comfortable you feel, take something small (callscreen or calendar) or big (system) :)

Comment 4

[Guilherme Uhelski]

Cool! Just to recap, I will make a per app patch and once they are all good I can squash them all in one big PR, right?

Comment 5

[Zibi Braniecki [:zbraniecki][:gandalf]]

I think I'd prefer you to make a per app patch, and get each landed separately. It'll be easier to track, isolate regressions etc.

Comment 6

[Guilherme Uhelski]

Great, I will start working on them and let you know how it goes.

Comment 7

[Guilherme Uhelski]

My first take on the calendar app: https://github.com/guhelski/gaia/commit/af7d858e60372dc18bfa59cf825fdf6ab247bea4

Some questions that are still blurry to me:

1) https://github.com/guhelski/gaia/commit/af7d858e60372dc18bfa59cf825fdf6ab247bea4#diff-fa69b42c7066e08948996ef0e2f0e977R128 Should we remove the arial-label attribute or the new data-l10n-id instead? Since we are setting a ariaLabel attribute, I suppose it will be turned into a aria-label and that's what needs to be removed (stays untouched).

2) https://github.com/guhelski/gaia/commit/af7d858e60372dc18bfa59cf825fdf6ab247bea4#diff-07af39e247d3c6b3efbc82d5a556d17bR495 We are only testing if the new attribute is passed to setAttribute, shouldn't we test if data-l10n-id actually turns it (with ariaLabel) into a aria-label?

3) https://github.com/guhelski/gaia/commit/af7d858e60372dc18bfa59cf825fdf6ab247bea4#diff-99723fc6fce28c8b011fff987dcb4a1aR160 What happens when we don't have a locale variable to set ariaLabel?

Sorry to bomb you with so many questions :)

Comment 8

[Zibi Braniecki [:zbraniecki][:gandalf]]

(In reply to Guilherme Uhelski from comment #7)
> My first take on the calendar app:
> https://github.com/guhelski/gaia/commit/
> af7d858e60372dc18bfa59cf825fdf6ab247bea4
> 
> Some questions that are still blurry to me:
> 
> 1)
> https://github.com/guhelski/gaia/commit/
> af7d858e60372dc18bfa59cf825fdf6ab247bea4#diff-
> fa69b42c7066e08948996ef0e2f0e977R128 Should we remove the arial-label
> attribute or the new data-l10n-id instead? Since we are setting a ariaLabel
> attribute, I suppose it will be turned into a aria-label and that's what
> needs to be removed (stays untouched).

Yeah, this is a manual setting of aria-label, not a localization. Keep it as is.

> 2)
> https://github.com/guhelski/gaia/commit/
> af7d858e60372dc18bfa59cf825fdf6ab247bea4#diff-
> 07af39e247d3c6b3efbc82d5a556d17bR495 We are only testing if the new
> attribute is passed to setAttribute, shouldn't we test if data-l10n-id
> actually turns it (with ariaLabel) into a aria-label?

Yes. The test should be as isolated as possible, so you only want to test if data-l10n-id has been set to 'busy'. This test didn't even do that, it just tested if setAttribute has been called with 'aria-label', so it is enough to switch it to test if it has been called with 'data-l10n-id'.

> 3)
> https://github.com/guhelski/gaia/commit/
> af7d858e60372dc18bfa59cf825fdf6ab247bea4#diff-
> 99723fc6fce28c8b011fff987dcb4a1aR160 What happens when we don't have a
> locale variable to set ariaLabel?

I don't think I understand this question.

Good start! Sorry for a late response, I've been traveling. From now on my reviews and feedback will be faster :)

Comment 9

[Zibi Braniecki [:zbraniecki][:gandalf]]

Hey Guilherme,

Do you plan to work on that bug?

Comment 10

[Guilherme Uhelski]

Hey Zibi.

Sorry for the very late reply, I have been traveling too and access to a computer is limited. Yes, I plan to work on the bug, I will be back to full speed on the 4th and I will try to advance a little on the patches till there.

Thanks for the answers!

Comment 11

[Guilherme Uhelski]

I'm back and with questions again... Looks like there was a big refactor on the calendar app views, month_child.js is gone and month_day.js is the new kid on the block. I've tried to replicate my changes to month_day.js but they are not working.

The changes to month_day.js: https://github.com/guhelski/gaia/commit/6bac3faacf24fad32167d1be02fac277d76c4b3e

The tests were gone too so I tried to add some: https://github.com/guhelski/gaia/commit/21201b3c5c48f05063af2e61a3c02af5d0f1cfc6

The good news is that the tests pass before my changes, so they apparently look good. For some reason setAttribute('data-l10n-id'... is not turning into an aria-label attribute, my suspicion is that the refactored  HTML snippet is not ready to do that: https://github.com/guhelski/gaia/blob/6bac3faacf24fad32167d1be02fac277d76c4b3e/apps/calendar/js/views/month_day.js#L46-L50

Makes any sense?

Comment 12

[Zibi Braniecki [:zbraniecki][:gandalf]]

(In reply to Guilherme Uhelski from comment #11)
> I'm back and with questions again... Looks like there was a big refactor on
> the calendar app views, month_child.js is gone and month_day.js is the new
> kid on the block. I've tried to replicate my changes to month_day.js but
> they are not working.
> 
> The changes to month_day.js:
> https://github.com/guhelski/gaia/commit/
> 6bac3faacf24fad32167d1be02fac277d76c4b3e

This should work. How do you verify it doesn't?


> The tests were gone too so I tried to add some:
> https://github.com/guhelski/gaia/commit/
> 21201b3c5c48f05063af2e61a3c02af5d0f1cfc6

This will not work because we localize nodes asynchronously. Instead, you need to test if data-l10n-id/data-l10n-args are properly set:

https://developer.mozilla.org/en-US/Firefox_OS/Developing_Gaia/localization_code_best_practices#Testing

Comment 13

[theultimateaim]

Hi, I saw this bug is assigned to nobody. Can I contribute to fixing it?

Comment 14

[Zibi Braniecki [:zbraniecki][:gandalf]]

Sorry, this bug is assigned to Guilherme.

Comment 15

[Guilherme Uhelski]

Zibi.

Looks like it does work indeed, as you noted, my test was the culprit. I'm testing if the attributes are set now: https://github.com/guhelski/gaia/commit/f045ba02e8594c391dd0689358f5e5486c3dc100

Do you think this test is enough?

Comment 16

[Zibi Braniecki [:zbraniecki][:gandalf]]

If you want to be deliberate you can use mozL10n.getAttributes and deepEqual. See: https://developer.mozilla.org/en-US/Firefox_OS/Developing_Gaia/localization_code_best_practices#Testing

Comment 17

[Guilherme Uhelski]

https://github.com/guhelski/gaia/blob/bug-1097827-calendar/apps/calendar/js/views/month_day.js#L74-L75
https://github.com/guhelski/gaia/blob/bug-1097827-calendar/apps/calendar/test/unit/views/month_day_test.js#L80-L116

Better?

Comment 18

[Zibi Braniecki [:zbraniecki][:gandalf]]

Yes :)

In the former link you can use navigator.mozL10n.setAttributes(holder, 'busy', { n: count });

Comment 19

[Guilherme Uhelski]

Fixed: https://github.com/guhelski/gaia/blob/bug-1097827-calendar/apps/calendar/js/views/month_day.js#L74

You mentioned about a per app patch, do you think it's ready for the calendar app?

Comment 20

[Zibi Braniecki [:zbraniecki][:gandalf]]

Yup. Create a pull request please, and set r? on me.

Comment 21

[GitHub Autolander Bot]

Attachment: [gaia] guhelski:bug-1097827-calendar > mozilla-b2g:master

Comment 22

[Zibi Braniecki [:zbraniecki][:gandalf]]

We're down to 26 cases in ./apps.

Comment 23

[Zibi Braniecki [:zbraniecki][:gandalf]]

We're down to two cases in ./apps !

Comment 24

[Santiago Saavedra]

Sorry, I have been off for awhile (laptop in repair; other issues). I'm not sure what should I do at the current state of things (in master).

Currently, in apps/calendar/js/views/month_day.js there is a

> holder.setAttribute('aria-label', navigator.mozL10n.get('busy', {
>   n: count
> }));

That I I think needs be like that in order to account for plurals.

Same thing in pdfjs, we need the mozL10n.get('thumb_page_canvas', { page: id } /*, ... */) in order to inject the {{page}} parameter.

At least, I haven't found any way to inject params if set as a DOM parameter in the HTML, according to https://developer.mozilla.org/en-US/Add-ons/SDK/Tutorials/l10n .

I would really love some input here.

Comment 25

[Santiago Saavedra]

I just stumbled upon https://developer.mozilla.org/en-US/Apps/Build/Localization/Getting_started_with_app_localization

Disregard previous comment.

Comment 26

[GitHub Autolander Bot]

Attachment: [gaia] ssaavedra:bugfix-1097827-change-arialabel-l10nid > mozilla-b2g:master

Comment 27

[Santiago Saavedra]

Comment on attachment 8698385 [details] [review]
[gaia] ssaavedra:bugfix-1097827-change-arialabel-l10nid > mozilla-b2g:master

With this patch I think calendar and pdfjs should now use the proper declarative way for mozL10n.

Comment 28

[Yura Zenevich [:yzen] (please use "needinfo")]

(In reply to Santiago Saavedra from comment #27)
> Comment on attachment 8698385 [details] [review]
> [gaia] ssaavedra:bugfix-1097827-change-arialabel-l10nid > mozilla-b2g:master
> 
> With this patch I think calendar and pdfjs should now use the proper
> declarative way for mozL10n.

So this is a good start. I suggest pdf.js related changes should go upstream so we could then grab a packaged version from them.

I have a feeling there are more cases where get use get method and otherwise shouldn't. Just a quick search for 'l10n.get' yields things like:
https://github.com/mozilla-b2g/gaia/blob/master/apps/bluetooth/js/deviceList.js#L97-L98

Note however some cases that you find are OK (non-DOM related)

Also see if the other out of date PR can be helpful as I don't think it ever got landed

Comment 29

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8698385 [details] [review]
[gaia] ssaavedra:bugfix-1097827-change-arialabel-l10nid > mozilla-b2g:master

Removing the r? for now. Also if you have any questions, simply request needinfo? Otherwise there's a high chance that I will miss your comment.

Comment 30

[Yura Zenevich [:yzen] (please use "needinfo")]

Hi Santiago, I'm back from the holidays, let me know if I can help to move this forward. Thanks!

Comment 31

[BMO Automation]

Firefox OS is not being worked on