Closed
Bug 872277
Opened 13 years ago
Closed 12 years ago
Main localization is injected after other 'ready' callbacks
Categories
(L20n :: HTML Bindings, defect, P1)
Tracking
(Not tracked)
RESOLVED
FIXED
1.0
People
(Reporter: zbraniecki, Assigned: zbraniecki)
References
Details
Attachments
(1 file, 1 obsolete file)
|
3.37 KB,
patch
|
stas
:
review+
|
Details | Diff | Splinter Review |
The current heuristics in html bindings is really sweet, but the problem we encounter is that any event listener on 'ready' that is registered from the main JS code will get registered before the crucial callback that localized the document.
Example:
<html>
<head>
<script type="application/l20n" src="./locales/en-US/index.lol"></script>
<script type="text/javascript">
document.l10n.addEventListener('ready', function() {
// do some intensive stuff like pool SQL
});
</script>
</head>
<body>
<h1 data-l10n-id="foo"></h1>
</body>
</html>
The node localization will be called after the SQL polling.
The ideal solution would be to have a way to inject our callback as the first one.
|
Assignee | |
Comment 1•13 years ago
|
||
|
||
Updated•13 years ago
|
Attachment #749540 -
Attachment is patch: true
Attachment #749540 -
Attachment mime type: text/x-patch → text/plain
|
|
||
Comment 2•13 years ago
|
||
Comment on attachment 749540 [details] [diff] [review]
POC
Review of attachment 749540 [details] [diff] [review]:
-----------------------------------------------------------------
::: lib/l20n/events.js
@@ +29,4 @@
> return this;
> }
>
> + EventEmitter.prototype.injectEventListener = function ee_inject(type, listener) {
I'd like to keep the EventEmitter's interface as close to other implementations as possible. Ideally, we'd be able to swap our events.js for node's if we wanted to.
We shouldn't tamper the order of callbacks. We shouldn't assume we know anything about the order they're in.
I have two alternative solutions:
1. don't call ctx.localize in a 'ready' listener in the bindings, but outsdie of it. ctx.localize will then bind a callback with _retr and the change from bug 868677 [1] will make it execute before other 'ready' listeners
2. emit an internal-use only 'pre-ready' event whose purpose is exactly to cater to this bug's use case (and other similar ones).
[1] https://github.com/zbraniecki/l20n.js/commit/a89efbf159e0aed0709a6691424583accf6a21d4#L1L468
|
|
||
Comment 3•12 years ago
|
||
G, this is waiting for your input about comment 2.
Flags: needinfo?(gandalf)
|
|
Assignee | |
Comment 4•12 years ago
|
||
(In reply to Staś Małolepszy :stas (needinfo along with cc, please) from comment #2)
> I have two alternative solutions:
>
> 1. don't call ctx.localize in a 'ready' listener in the bindings, but
> outsdie of it. ctx.localize will then bind a callback with _retr and the
> change from bug 868677 [1] will make it execute before other 'ready'
> listeners
So basically - move body of localizeBodyHandler inside collectNodes?
This will make localize set the eventlistener on localizeHandler before onload fires.
Sounds good to me, but there's still implicit assumption that no code will register eventemitter between we add ctx to document.l10n and when collectNodes is ready.
> 2. emit an internal-use only 'pre-ready' event whose purpose is exactly to
> cater to this bug's use case (and other similar ones).
This sounds like more bullet-proof solution. The problems I see with this are:
- people can register for pre-ready and then it's similar issue as with ready
- we leak HTML bindings needs onto context API
Flags: needinfo?(gandalf)
|
|
Assignee | |
Comment 5•12 years ago
|
||
(In reply to Zbigniew Braniecki [:gandalf] from comment #4)
> (In reply to Staś Małolepszy :stas (needinfo along with cc, please) from
> comment #2)
> > I have two alternative solutions:
> >
> > 1. don't call ctx.localize in a 'ready' listener in the bindings, but
> > outsdie of it. ctx.localize will then bind a callback with _retr and the
> > change from bug 868677 [1] will make it execute before other 'ready'
> > listeners
>
> So basically - move body of localizeBodyHandler inside collectNodes?
>
> This will make localize set the eventlistener on localizeHandler before
> onload fires.
>
> Sounds good to me, but there's still implicit assumption that no code will
> register eventemitter between we add ctx to document.l10n and when
> collectNodes is ready.
Actually I'm wrong here. RetranslationManager fires before EE fires ready, so I want to use this path.
Assignee: nobody → gandalf
Priority: -- → P2
Target Milestone: --- → 1.0
|
|
Assignee | |
Updated•12 years ago
|
Priority: P2 → P1
|
|
Assignee | |
Comment 6•12 years ago
|
||
A patch that moves us to use localize.
Attachment #749540 -
Attachment is obsolete: true
Attachment #755631 -
Flags: review?(stas)
|
|
||
Comment 7•12 years ago
|
||
Comment on attachment 755631 [details] [diff] [review]
patch
Review of attachment 755631 [details] [diff] [review]:
-----------------------------------------------------------------
r=me.
::: bindings/l20n/html.js
@@ +50,5 @@
> + // this function is fired right when we have document.body available
> + //
> + // We collect the nodes and then we check if the l10n context is ready.
> + // If it is ready, we create localize block for it, if not
> + // we set an event listener on context and add it when it's ready.
The comment is out of date.
@@ +57,3 @@
> localizeHandler = ctx.localize(nodes.ids, function(l10n) {
> if (!nodes) {
> nodes = getNodes(document.body);
I wonder if we still need to nullify nodes after the callback has run.
Maybe a better approach would be to store the array of ids passed to localize() (and amend it when localizeNode() is called), and expose it on the l10n object?
Attachment #755631 -
Flags: review?(stas) → review+
|
|
||
Comment 8•12 years ago
|
||
(In reply to Staś Małolepszy :stas from comment #7)
> I wonder if we still need to nullify nodes after the callback has run.
>
> Maybe a better approach would be to store the array of ids passed to
> localize() (and amend it when localizeNode() is called), and expose it on
> the l10n object?
Bug 879212.
|
|
||
Comment 9•12 years ago
|
||
Gandalf, I took your patch, removed the outdated comment and landed:
https://github.com/l20n/l20n.js/commit/2a0c59f094252d7fbf8cefefc0c91484179d16b0
This also adds the change from bug 875292 to bindings/l20n/gaia, which had previously only landed in bindings/l10n/html.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•