Last Comment Bug 994290 - Clean up attribute storage in AST
: Clean up attribute storage in AST
Status: RESOLVED FIXED
:
Product: Firefox OS
Classification: Client Software
Component: Gaia::L10n (show other bugs)
: unspecified
: All All
P2 normal (vote)
: ---
Assigned To: Zibi Braniecki [:gandalf][:zibi]
:
:
Mentors:
: 936534 (view as bug list)
Depends on: 1009134 1010537 1021942 1021946
Blocks: 994357 1006359
  Show dependency treegraph
 
Reported: 2014-04-09 13:06 PDT by Zibi Braniecki [:gandalf][:zibi]
Modified: 2014-06-10 16:17 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (2.27 KB, patch)
2014-05-21 16:53 PDT, Zibi Braniecki [:gandalf][:zibi]
stas: feedback+
Details | Diff | Splinter Review
pull request (46 bytes, text/x-github-pull-request)
2014-05-21 17:02 PDT, Zibi Braniecki [:gandalf][:zibi]
no flags Details | Review | Splinter Review
patch v2 (5.30 KB, patch)
2014-06-06 14:13 PDT, Zibi Braniecki [:gandalf][:zibi]
stas: review+
Details | Diff | Splinter Review

Description User image Zibi Braniecki [:gandalf][:zibi] 2014-04-09 13:06:37 PDT
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 User image Staś Małolepszy :stas 2014-04-10 11:01:28 PDT
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 User image Staś Małolepszy :stas 2014-04-10 11:21:26 PDT
(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 User image Zibi Braniecki [:gandalf][:zibi] 2014-04-10 17:03:52 PDT
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 User image Staś Małolepszy :stas 2014-05-20 03:59:17 PDT
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 User image Zibi Braniecki [:gandalf][:zibi] 2014-05-21 16:53:41 PDT
Created attachment 8426671 [details] [diff] [review]
patch

I simplified code and special-cased ariaLabel and innerHTML for now.
Comment 6 User image Zibi Braniecki [:gandalf][:zibi] 2014-05-21 17:02:57 PDT
Created attachment 8426678 [details] [review]
pull request
Comment 7 User image Staś Małolepszy :stas 2014-05-22 02:59:39 PDT
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 User image Staś Małolepszy :stas 2014-05-22 03:16:04 PDT
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 User image Zibi Braniecki [:gandalf][:zibi] 2014-06-06 14:13:05 PDT
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.
Comment 10 User image Staś Małolepszy :stas 2014-06-09 06:52:09 PDT
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 User image Zibi Braniecki [:gandalf][:zibi] 2014-06-09 14:53:32 PDT
(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 User image Staś Małolepszy :stas 2014-06-09 15:08:44 PDT
(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 14 User image Zibi Braniecki [:gandalf][:zibi] 2014-06-09 17:05:11 PDT
*** Bug 936534 has been marked as a duplicate of this bug. ***
Comment 15 User image Staś Małolepszy :stas 2014-06-10 00:28:49 PDT
Can you also land in https://github.com/l20n/l20n.js/tree/gaia/ please?
Comment 16 User image Zibi Braniecki [:gandalf][:zibi] 2014-06-10 16:17:13 PDT
Oh, thanks Stas!

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

Note You need to log in before you can comment on or make changes to this bug.