Clean up attribute storage in AST

RESOLVED FIXED

Status

Firefox OS
Gaia::L10n
P2
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: gandalf, Assigned: gandalf)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

3 years ago
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).
Blocks: 994357
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%
(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)?
(Assignee)

Comment 3

3 years ago
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
(Assignee)

Updated

3 years ago
Blocks: 1006359
(Assignee)

Updated

3 years ago
Assignee: nobody → gandalf
Priority: -- → P2
(Assignee)

Updated

3 years ago
Depends on: 1009134
(Assignee)

Updated

3 years ago
Depends on: 1010537
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.)
Flags: needinfo?(gandalf)
(Assignee)

Comment 5

3 years ago
Created attachment 8426671 [details] [diff] [review]
patch

I simplified code and special-cased ariaLabel and innerHTML for now.
Attachment #8426671 - Flags: feedback?(stas)
Flags: needinfo?(gandalf)
(Assignee)

Comment 6

3 years ago
Created attachment 8426678 [details] [review]
pull request
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.
Attachment #8426671 - Flags: feedback?(stas) → feedback+
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.
(Assignee)

Updated

3 years ago
Depends on: 1021942
(Assignee)

Updated

3 years ago
Depends on: 1021946
(Assignee)

Comment 9

3 years ago
Created attachment 8436089 [details] [diff] [review]
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.
Attachment #8426671 - Attachment is obsolete: true
Attachment #8436089 - Flags: review?(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.
Attachment #8436089 - Flags: review?(stas) → review+
(Assignee)

Comment 11

3 years ago
(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?
(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.
(Assignee)

Comment 13

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

Filed bug 1022989.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
(Assignee)

Updated

3 years ago
Duplicate of this bug: 936534
Can you also land in https://github.com/l20n/l20n.js/tree/gaia/ please?
(Assignee)

Comment 16

3 years ago
Oh, thanks Stas!

L20n.js: https://github.com/l20n/l20n.js/commit/ceec667904ab1cf8e30347e3e232c43373cb2038
You need to log in before you can comment on or make changes to this bug.