994290 - Clean up attribute storage in AST

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::L10n, defect, P2)

Description

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

The current approach to storing nested attributes of entities in unnecessarily complex and potentially confusing.

See: https://github.com/mozilla-b2g/gaia/blob/fd2d573db38fa4ff5e607e62a35a23cb15c025d7/shared/js/l10n.js#L652-L680

We should be able to clean up the AST by allowing for nested attributes and update the DOM bindings code to properly recognize special ones (style/dataset).

Comment 1

[Staś Małolepszy :stas]

I did a quick grep all Gaia apps and found only one place where 'dataset' was used:

apps/sms/locales/sms.en-US.properties
43:composeMessage.dataset.placeholder = Message

It might be worth investigating why we need the placeholder as a data attr on composeMessage and whether there are other, simpler ways, of achieving the same result.

'style' isn't used anywhere in the source repo, but the French locale uses it:

fr/apps/browser/browser.properties
9:history.style.maxWidth=20%
45:top-sites-tab.style.maxWidth=15%
60:bookmarks.style.maxWidth=27%

Comment 2

[Staś Małolepszy :stas]

(In reply to Staś Małolepszy :stas from comment #1)

> It might be worth investigating why we need the placeholder as a data attr
> on composeMessage and whether there are other, simpler ways, of achieving
> the same result.

So this was bug 883813.  It seems like the problem was that the placeholder is set on a div, not an input, and inserted into the page via CSS's content: attr(data-placeholder) property.

I wonder if we could just use composeMessage.dataPlaceholder or composeMessage.data-placeholder instead?  And maybe unify it with aria-label (bug 920252)?

Comment 3

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

So, if I understand correctly we have three types of use cases that exceed the attribute model:

 - style.*
 - data.*
 - innerHTML

My understanding is that we should aim for:

 - style.* should not be part of localization. It's a security issue if a localizer can modify position of elements, display etc. I'd suggest that we work on a global solution to make text fit on the screen (there was a bug for that?). We have responsive l10n down the path, and auto-font-sizing.

 - data.* should also not be part of localization. I'd much rather prefer the developer to use JS to pull the right localization code than for the code to assume that data-* is localizable. (esp. in the context of planned move to MutationObservers - bug 992473)

 - innerHTML is so scary. We should use bug 994357 to tackle that.

I'd suggest the following action chain:

1) I'll file a bug to stop using data.* in SMS
2) I'll submit a patch there
3) I'll file a bug to stop using style.* in French Browser
4) We'll remove support for that here
5) We tackle innerHTML in bug 994357
6) We switch to setAttribute everywhere.

The only challenge I can see here is if the changes made by the French Community are required because I do not have a replacement in place for them.

Also, as a side note. I would like attributes to be "nested" in the sense that an atttribute may be a Hash Value, not Entity value, but that's outside of the scope of this bug since the goal is for string branching, not DOM attribute nesting.

CC'ing Vivien

Comment 4

[Staś Małolepszy :stas]

Do we want the following to be invalid syntax?

  id.foo.bar = Foo

Currently, that parses as an entity with id "id.foo" with an attribute "bar".  (Unless foo is actually dataset or style, but we're phasing these quirks out.)

Intuitively, the above line should mean: entity 'id', with an attribute 'foo' which in turn has its own attribute 'bar'.  But since we don't want nested attributes, I suggest this be invalid syntax.

(In L20n master, the syntax doesn't allow to define nested attrs;  referencing them (foo::bar::baz) is also invalid.)

Comment 5

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

Attachment: patch

I simplified code and special-cased ariaLabel and innerHTML for now.

Comment 6

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

Attachment: pull request

Comment 7

[Staś Małolepszy :stas]

Comment on attachment 8426671 [details] [diff] [review]
patch

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

f=me, thanks.  We'll need tests for the parser part, please, and you'll need to change a few tests in apps/sharedtest/test/unit/l10n/l10n_test.js to test the new behavior (throwing the error).  Or at least remove the ones with dataset and style.

::: lib/l20n/parser.js
@@ +101,2 @@
>    } else {
>      attr = null;

Do we even need this else clause here?  It seems that attr == undefined is good enough for setEntityValue.

Comment 8

[Staś Małolepszy :stas]

Comment on attachment 8426671 [details] [diff] [review]
patch

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

::: bindings/l20n/dom.js
@@ +93,2 @@
>        } else {
> +        element.setAttribute(key, attr);

I noticed a few apps use .textContent, not sure why, tbh.  Should we special case it here as well?  setAttribute('textContent') doesn't work.

Other attributes in use: placeholder, title, label.  I'm not sure about the last one.  It's in the Video app, but I didn't find it used in code.

Comment 9

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

Attachment: patch v2

Updated patch. I filed two bugs for removing the textContent uses, and I'm removing the obsolete entity for label use in this patch.

Stas: I removed the tests that don't make sense anymore and added the attribute error test for parser.

Comment 10

[Staś Małolepszy :stas]

Comment on attachment 8436089 [details] [diff] [review]
patch v2

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

r=me, removing code makes me happy.

I wonder if we should maybe be a bit louder when an error about this is thrown.  Right now, we emit it on the ctx, which does nothing with DEBUG=0.  This might make it harder for developers to understand why their code isn't working.  Just a thought;  this is good to land as-is.

Comment 11

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

(In reply to Staś Małolepszy :stas from comment #10)
> I wonder if we should maybe be a bit louder when an error about this is
> thrown.  Right now, we emit it on the ctx, which does nothing with DEBUG=0. 
> This might make it harder for developers to understand why their code isn't
> working. 

How would you like to do that? I think it's a more generic problem - if a developer makes a mistake in his .properties code, we do not help him discover that, right?

Comment 12

[Staś Małolepszy :stas]

(In reply to Zibi Braniecki [:gandalf] from comment #11)

> How would you like to do that? I think it's a more generic problem - if a
> developer makes a mistake in his .properties code, we do not help him
> discover that, right?

Yes, you're right.  It's a generic problem.  I think it's relevant here because we're effectively changing the allowed syntax of the properties files.  However, nested attributes were so sparingly used the I think it's okay to discuss this after this bug is fixed.

Comment 13

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

Commit: https://github.com/mozilla-b2g/gaia/commit/021cc02f12b20d0c76e6e67f5a97883cd23b46db
Merge: https://github.com/mozilla-b2g/gaia/commit/954b77e12ef1d1cfdf8c9827af2842e5a525450c

Filed bug 1022989.

Comment 15

[Staś Małolepszy :stas]

Can you also land in https://github.com/l20n/l20n.js/tree/gaia/ please?

Comment 16

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

Oh, thanks Stas!

L20n.js: https://github.com/l20n/l20n.js/commit/ceec667904ab1cf8e30347e3e232c43373cb2038