Closed Bug 994290 Opened 11 years ago Closed 10 years ago

Clean up attribute storage in AST

Categories

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

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: zbraniecki, Assigned: zbraniecki)

References

Details

Attachments

(2 files, 1 obsolete file)

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)?
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
Blocks: 1006359
Assignee: nobody → gandalf
Priority: -- → P2
Depends on: 1009134
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)
Attached patch patch (obsolete) (deleted) — Splinter Review
I simplified code and special-cased ariaLabel and innerHTML for now.
Attachment #8426671 - Flags: feedback?(stas)
Flags: needinfo?(gandalf)
Attached file pull request (deleted) —
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.
Depends on: 1021942
Depends on: 1021946
Attached patch patch v2 (deleted) — Splinter Review
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+
(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.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: