Allow <img/> in Fluent DOM Overlays

RESOLVED FIXED

Status

()

enhancement
P3
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: gandalf, Assigned: gandalf)

Tracking

(Blocks 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 obsolete attachment)

In order to fully transition Preferences Chrome to Fluent we'd like to enable <image/> element to be used in DOM Overlays.

There are five strings in the patch in bug 1424682 which would use <image/> element with dom overlays.

Currently, we only have a whitelist of HTML elements which can be introduced by localization [0] and we also only allow localizable elements to be in DOM Overlays.

That means that in HTML this works:

```
<p data-l10n-id="key1">
  <a href="http://www.mozilla.org"></a>
  <a href="http://www.firefox.com"></a>
</p>

===

key1 =
  Visit <a>Mozilla</a> and read more about <a>Firefox</a>.
```

but this in XUL doesn't:

```
<description data-l10n-id="extension-controlled-enable">
  <image src="chrome://mozapps/skin/extensions/extensionGeneric-16.svg"/>
  <image src="chrome://browser/skin/menu.svg"/>
</description>

===

extension-controlled-enable = To enable the extension go to <image/> Add-ons in the <image/> menu.
```

We'd like to enable the <image/> element in XUL to be available for DOM Overlays.

[0] https://github.com/projectfluent/fluent.js/blob/master/fluent-dom/src/overlay.js#L10
(Assignee)

Comment 1

a year ago
:bz - you're listed as a peer of XPToolkit. Is there a list of XUL elements that we could whitelist similar to localizable HTML elements? If not, are you ok with us whitelisting <image/>?

:freddyb - in the context of the security review of Fluent, can you advise here? Is it ok for us to whitelist <image/>?
Flags: needinfo?(fbraun)
Flags: needinfo?(bzbarsky)
(Assignee)

Updated

a year ago
Blocks: 1424682
Priority: -- → P3
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 4

a year ago
mozreview-review
Comment on attachment 8948062 [details]
Bug 1435464 - Allow <html:img/> in Fluent DOM Overlays.

https://reviewboard.mozilla.org/r/217698/#review223468

I'm technically a peer for xptoolkit (though we should reevalute that), but I don't know why this list is limited to the things its limited to and what the implications of adding things to it are, sorry....
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 5

a year ago
I think it falls into your bucket :mossop since you reviewed the whole Fluent and DOM Overlays in particular. :)
Flags: needinfo?(dtownsend)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #1)
> :freddyb - in the context of the security review of Fluent, can you advise
> here? Is it ok for us to whitelist <image/>?

My XUL-fu isn't very strong to fully understand the risks here.
Are we making sure to only allow 'src' attributes on image elements?
Flags: needinfo?(fbraun)
(Assignee)

Comment 7

a year ago
We only allow image elements to be placed in DOM Fragments. I don't believe localizers will be able to set the `src` attribute (NI on stas on that).

Those are the only settable attributes on any XUL element: https://searchfox.org/mozilla-central/source/intl/l10n/DOMLocalization.jsm#59

:freddy - if that's the case, do you have any reason to oppose this feature?
Flags: needinfo?(stas)
Flags: needinfo?(fbraun)
Looks good, go ahead.
Flags: needinfo?(fbraun)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #7)
> We only allow image elements to be placed in DOM Fragments. I don't believe
> localizers will be able to set the `src` attribute (NI on stas on that).

That's correct.
Flags: needinfo?(stas)

Comment 10

a year ago
mozreview-review
Comment on attachment 8948062 [details]
Bug 1435464 - Allow <html:img/> in Fluent DOM Overlays.

https://reviewboard.mozilla.org/r/217698/#review223542

Zibi, were you able to test this successfully? 


The markup overlay code in Fluent uses HTML's <template> element behind the scenes. This means that everything it parses gets parsed as HTML elements. The namespace check should always return the HTML5 namespace for any elements found in the translation. Perhaps there's a way to use "<xul:image>" instead? My early morning attempts have been so far unsuccessful.

It also just so happens that "<image>" is parsed as a regular HTML <img> element. Which is also not currently allowed for the HTML namespace. Perhaps the solution for bug 1424682 is to use <html:img> instead of <image>?
(In reply to Staś Małolepszy :stas from comment #10)

> The namespace check should always return the HTML5 namespace for any
> elements found in the translation.

To clarify:

If I'm reading the code right, whitelisting <image> for the XUL namespace won't work.

isElementLocalizable uses namespaceURI of child elements found in the translation to determine if they are localizable for this namespace. However, <template> parses everything as HTML and the namespace is always http://www.w3.org/1999/xhtml.
oh, that's a bummer.

Taking off NI from :mossop because it may be that there's no value in adding XUL elements at all.

Will test it further.
Flags: needinfo?(dtownsend)
Morphing this bug into support for HTML <img/> in DOM Overlays - still without 'src' of course.
Summary: Allow <image/> in Fluent DOM Overlays → Allow <img/> in Fluent DOM Overlays
(Assignee)

Updated

a year ago
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Blocks: 1435912
No longer blocks: 1424682
Comment on attachment 8948062 [details]
Bug 1435464 - Allow <html:img/> in Fluent DOM Overlays.

Removing the review request as this isn't something that we want to do.
Attachment #8948062 - Flags: review?(stas)
Let's close this bug as WONTFIX. We don't want to white-list <img>. Instead, we'd like to allow developers to put any elements in the source and make it possible to overlay them with translations.

The design work is taking place in https://github.com/projectfluent/fluent.js/issues/128 and https://github.com/projectfluent/fluent.js/issues/144.
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → WONTFIX
(Assignee)

Updated

a year ago
Attachment #8948062 - Attachment is obsolete: true
We discussed it with stas on IRC and decided to reopen to track a blocker to bug 1435912.

Instead of whitelisting <img/> we'd like to come up with v2 of DOM Overlays API which will support developer provided elements, so this bug will likely be solved with that, but depending on timing we may look for intermediate solutions.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
(Assignee)

Updated

a year ago
Blocks: 1415730
No longer blocks: 1435912
(Assignee)

Updated

a year ago
Depends on: 1453480
This has been verified as fixed by bug 1453480
Status: REOPENED → RESOLVED
Last Resolved: a year agoa year ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.