l10n.js should use setAttribute for setting attributes of HTML elements

RESOLVED DUPLICATE of bug 994290

Status

Firefox OS
Gaia::L10n
P4
normal
RESOLVED DUPLICATE of bug 994290
4 years ago
4 years ago

People

(Reporter: stas, Assigned: gandalf)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

4 years ago
Curerntly, l10n.js uses the element[property] syntax.  The proper approach is to use setAttribute().

To minimize the need for rebasing, let's fix this once bug 914414 lands.
(Assignee)

Updated

4 years ago
Assignee: nobody → gandalf
(Assignee)

Comment 1

4 years ago
Created attachment 8404190 [details] [diff] [review]
patch

This is a small patch.

A bigger one will go in bug 994290.
Attachment #8404190 - Flags: review?(stas)
(Reporter)

Comment 2

4 years ago
Before I review this, I'd like to see a patch against the Gaia repo, with relevant tests fixed.  For instance, in apps/gallery/test/unit/l10n/l10n_test.js you'll find:

      assert.equal(elem.prop, 'this is another property');
      assert.equal(elem.getAttribute('aria-label'), 'label via ARIA');

This should be unified to getAttribute, I believe.
(Assignee)

Comment 3

4 years ago
Created attachment 8404501 [details] [diff] [review]
patch, gaia part
Attachment #8404501 - Flags: review?(stas)
(Reporter)

Comment 4

4 years ago
Comment on attachment 8404190 [details] [diff] [review]
patch

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

r=me with a question.

Should we also change the line above the patch, which currently looks like the following?

  element[key.substr(0, pos)][key.substr(pos + 1)] = attr;

This could use setProperty for 'style', but not for 'dataset'.  It would also need a camelCase function to convert names of properties.

My take is 'no', and judging from the patch, you're against it as well, but I just want to make sure.
Attachment #8404190 - Flags: review?(stas) → review+
(Reporter)

Comment 5

4 years ago
Comment on attachment 8404501 [details] [diff] [review]
patch, gaia part

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

Thanks. Not many changes, cool.  Can you create a pull request for Gaia?
Attachment #8404501 - Flags: review?(stas) → review+
(Reporter)

Comment 6

4 years ago
Comment on attachment 8404190 [details] [diff] [review]
patch

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

Actually, I'm afraid this is an r-.  I just realized that this will break all the foo.innerHTML we do in Gaia to insert HTML element in the translations.  Let's first fix bug 994357, get rid of innerHTML in the properties files, and revisit this bug.
Attachment #8404190 - Flags: review+ → review-
(Reporter)

Updated

4 years ago
Depends on: 994357
(Assignee)

Comment 7

4 years ago
(In reply to Staś Małolepszy :stas from comment #4)
> Should we also change the line above the patch, which currently looks like
> the following?
> 
>   element[key.substr(0, pos)][key.substr(pos + 1)] = attr;

No. That's bug 994290.
(Reporter)

Updated

4 years ago
Component: Gaia → Gaia::L10n
OS: Linux → All
Hardware: x86_64 → All
(Reporter)

Updated

4 years ago
Blocks: 999779
(Assignee)

Updated

4 years ago
Blocks: 1006359
(Assignee)

Updated

4 years ago
Priority: -- → P4
(Assignee)

Comment 8

4 years ago
I just landed a patch for bug 994290 which basically fixes this. 

The only remaining piece is innerHTML exception which is going to be fixed by bug 994357.

I'm marking this bug as a DUPE of 994290.
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 994290
You need to log in before you can comment on or make changes to this bug.