Internet Explorer 10 compatibility

RESOLVED FIXED in 1.0

Status

L20n
JS Library
P2
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Matthias Mullie, Assigned: stas)

Tracking

Details

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
Created attachment 745680 [details] [diff] [review]
ie.patch

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:20.0) Gecko/20100101 Firefox/20.0
Build ID: 20130326150557

Steps to reproduce:

Use l20n on IE


Actual results:

It didn't work:
* IE does not have HTMLDocument
* IE does not support XMLHttpRequest::overrideMimeType
* IE is not terribly happy with unneeded trailing commas


Expected results:

It should've worked.

Patch is attached.
Target Milestone: --- → 1.0
confirming
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attachment #745680 - Flags: review?(stas)
Attachment #745680 - Attachment is patch: true
(Assignee)

Comment 2

5 years ago
Comment on attachment 745680 [details] [diff] [review]
ie.patch

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

Bug 866723 made another series of renames and the patch has bitrotten.  I'm sorry :(  I have a few other patches waiting for reviews;  when they land, I'll rebase this patch.

Everything looks good, but please see my comment below about HTMLDocument.

::: lib/l20n/bindings/html/io.js
@@ +43,4 @@
>          // XXX should this fail more horribly?
>          return '';
>        }
> +    }

Can we just check if (xhr.overrideMimeType) here?

::: lib/l20n/bindings/html.js
@@ +102,4 @@
>      } else {
>        document.addEventListener('readystatechange', onDocumentBodyReady);
>      }
> +    if (typeof HTMLDocument != 'undefined') {

I had to research this a little bit, which is why I review the patch only now.

It seems that HTMLDocument and HTMLElement (we'll want that here too, eventually) are part of DOM 2 HTML spec, and are no longer present in DOM 3 Core (nor in DOM 4).

Instead, DOM 3 Core gives us Document and Element:

    http://www.w3.org/TR/DOM-Level-3-Core/core.html#ID-1590626202

Further more, HTML 5.1 defines Document and HTMLElement:

    http://www.w3.org/html/wg/drafts/html/master/dom.html#the-document-object
    http://www.w3.org/html/wg/drafts/html/master/dom.html#elements-in-the-dom

Document and HTMLElement (and Element) are supported by IE10, but I have no way to test other versions right now.  Matthias, do you know more about this?

Given the compatibility issues and also good practices, maybe it would advisable to attach l10n to `document` in all browsers, without the above check?

Gandalf, what do you think?
Attachment #745680 - Flags: review?(stas) → review+
I'd settle on Document and HTMLElement as per HTML 5.1 spec
(Assignee)

Comment 4

5 years ago
(In reply to Staś Małolepszy :stas (needinfo along with cc, please) from comment #2)

> Document and HTMLElement (and Element) are supported by IE10, but I have no
> way to test other versions right now.  Matthias, do you know more about this?

I checked IE 9 and it supports both Document and HTMLElement too.

(In reply to Zbigniew Braniecki [:gandalf] from comment #3)
> I'd settle on Document and HTMLElement as per HTML 5.1 spec

Cool.

Matthias, would you like to resubmit the patch changing HTMLDocument to Document for all browser?  Thanks!
(Assignee)

Comment 5

5 years ago
Let's fix bug 868677 first and then fix IE in a single landing.
Depends on: 868677
(Assignee)

Comment 6

5 years ago
Setting priority to P2 but only for IE 10.
Assignee: nobody → stas
Priority: -- → P2
Summary: Internet Explorer compatibility → Internet Explorer 10 compatibility
(Reporter)

Comment 7

5 years ago
On IE10 (Win8):
Document.prototype.__defineGetter__ -> undefined
Same thing for HTMLElement/Element.

Alternative (cross-browser) could be:
    Object.defineProperty(Document.prototype, 'l10n', {
      get: function() {
        return ctx;
      }
    });

This does work on IE10 too, just tested.
(Assignee)

Comment 8

5 years ago
In bug 868677 we want to stop extending the prototypes of host objects, like Document and HTMLELement.  It's generally considered bad practice and has a significant performance impact.

I think we'll settle on "document.l10n =" (like you did in your patch) for all browser.  Instead HTMLElement.prototype.localize we might go for document.l10n.localizeNode.
(Assignee)

Comment 9

5 years ago
Sorry it took so long.  A bunch of renames made rebasing this patch a more tedious task.  Landed on master:

https://github.com/l20n/l20n.js/commit/b68d34e1418503565c051d55294761e14a8a9135

Thanks, Matthias!

(In reply to Staś Małolepszy :stas from comment #8)
> I think we'll settle on "document.l10n =" (like you did in your patch) for
> all browser.  Instead HTMLElement.prototype.localize we might go for
> document.l10n.localizeNode.

That's exactly what we do now.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.