Internet Explorer support meta-issue

[csillag]

I am creating this issue to coordinate and track IE-related activity.

Tasks for "level 1" support (no dtm, no position or fuzzy strategy):

• Work around missing evaluate API
• Work around missing get URL api (I don't remember the name, @gergely-ujvari probably does)
• Modify all Annotator plugins to work fine without DTM
• Investigate the issues related to the bookmarklet
• Fix the XPath selector creation / reattachment strategy to work reliably (See https://github.com/hypothesis/annotator/issues/59 and https://github.com/hypothesis/annotator/issues/60)
• Implement conditional loading of DTM, based on request parameters - Add option to allow loading H without d-t-m #1233
• Clean up some non-fatal IE warnings
  • IE css style warning #1225
  • Warnings for standalone page under IE #1226

Tasks for "level 2" support (including dtm, position and fuzzy strategies)

• Approach 1 (using d-t-m's master branch):
  • Port d-t-m to use Rangy, eliminating some of the exceptions - see https://github.com/hypothesis/dom-text-mapper/issues/40
  • Make upstream Rangy developers add the required workaround to clean up the remaining exceptions
  • Verify if the performance is acceptable (according to early test, Rangy comes with a big performance penalty)
• Approach 2 (using d-t-m's shoot branch, which includes support for dynamic web pages): This approach is currently blocked, further discussions pending
  • Port d-t-m's shoot branch to Rangy, eliminating all the exceptions
  • Finish the performance optimization work on this branch of d-t-m
  • Port recent dtm fixes to the shoot branch (where applicable)
  • Port recent annotator fixes to the shoot branch (where applicable)
  • Rebase H's shoot branch to latest develop
  • Switch to the shoot branch

[csillag]

Good news here: all "Level 1" tasks are finished. (See list above.)
So now we are good to go with IE.

(But see the limitations described in the OP.)

[gergely-ujvari]

I'll recap here the recent news.

Rangy has not corrected the opened bug (https://github.com/hypothesis/dom-text-mapper/issues/40)
So I've made tests and workarounds with the main branch of the d-t-m, to try to fix/workaround the errors manually.

Results:

• the selection.addRange() call is implemented that way under IE, that if you try to select a node, which is a child/descendant of an invisible node, then IE throws an exception for that: SCRIPT16389: Unspecified error. (this has been sent with a fiddle to the IE guys)
• Other browsers just silently handle this
• Luckily, this exception can be cought and handled. It is fine for us, not to add this node to the range (d-t-m works fine with this)
• Of course, new errors have emerged.
• The selection.removeAllRanges() call is implemented that way under IE, that sometimes if gives back the following error: Could not complete the operation due to error 800a025e.
• Sadly this call cannot be ignored or worked around easily because the error emerges when there is a range to remove.
• After a huge digging I found two possible causes for this error:
  • Microsoft .NET framework should be updated (updated it, didn't work)
  • IE throws this error when a js library injects a hidden IFRAME dynamically to the page and trying to mess with ranges around that injected IFRAME
• This second is exactly what we do for the h. Our sidebar is an injected IFRAME.
• But confusingly, the first range throwing this exception is a range inside the heatmap.

Possible next steps:

• I'll try to identify which ranges would cause IE to throw this exception and hopefully they will not be meaningful ranges to the d-t-m.
• I'll set up a dokku instance (if everybody agrees) which will throw this error under IE, and let's show them the Microsoft IE guys, maybe they'll know to handle this (or maybe they'll release an error patch)
• Postpone this issue until we have the d-t-m conversation, because if we would use another approach to have the corpus-node mapping (either by d-t-m shoot, or a brand new approach) then hopefully these problems will fly away.

What do you think?

[gergely-ujvari]

Adding a bit more info.
As @csillag suggested the newer implementations of the d-t-m (observer, shoot) contain a configurable solution for ignoring parts of the DOM from scanning. So if we can deny scan our injected parts in the DOM then maybe we can workaround this recent IE problem.

As a test, I've backported the relevant functions and made a test with this. (Ignoring the "div.annotator-notice", "div.annotator-frame", "div.annotator-adder" elements and their descendants from the scan)

The good news: The IE exception for the removeAllRanges call is now gone! 🍺
The bad news: We have new exceptions but now, inside the d-t-m, and hopefully they are not browser relevant. So I'll ask a quick help from @csillag and hopefully we can release a better IE compatible version of d-t-m and H.

The current exception is now inside d-t-m, collectPositions(), pathInfo is not always available.

[csillag]

On 2014-06-17 11:41, gergely-ujvari wrote:

I'll set up a dokku instance (if everybody agrees) which will throw
this error under IE, and let's show them the Microsoft IE guys, maybe
they'll know to handle this (or maybe they'll release an error patch)

If we want to ask for external help (for example, IE engineers), we
should build a minimal test case on jsfiddle, showcasing the problem.

[gergely-ujvari]

2014.06.17. 13:24 keltezéssel, Kristof Csillag írta:

On 2014-06-17 11:41, gergely-ujvari wrote:

I'll set up a dokku instance (if everybody agrees) which will throw
this error under IE, and let's show them the Microsoft IE guys, maybe
they'll know to handle this (or maybe they'll release an error patch)

If we want to ask for external help (for example, IE engineers), we
should build a minimal test case on jsfiddle, showcasing the problem.
That is not that easy in this case, but maybe we can workaround this.

—
Reply to this email directly or view it on GitHub
#1234 (comment).

[csillag]

On 2014-06-17 11:41, gergely-ujvari wrote:

if we would use another approach to have the corpus-node mapping
(either by d-t-m shoot, or a brand new approach) then hopefully these
problems will fly away.

To rephrase this: the way the shoot branch of d-t-m uses the selection
API is much more robust, and it will not trigger any of these errors.
(And it also survives dynamic webpages, unlike the master branch.)

That's why I would prefer to migrate to it (after finishing the pending
performance optimization of it.)

[csillag]

If our approach is to try to make IE work by doing extensive work inside
d-t-m,
then I suggest doing it on the shoot branch, which

• does not trigger any of those those IE errors
• compatible with dynamic pages
• can be ported more easily to the new dom-anchors-core library.

I am wary of adding more fixes to the master branch of the d-t-m, but I
will see what I can do later.

(Right now I am focusing on the dom-anchors library.)

On 2014-06-17 13:22, gergely-ujvari wrote:

Adding a bit more info.
As @csillag https://github.com/csillag suggested the newer
implementations of the d-t-m (observer, shoot) contain a configurable
solution for ignoring parts of the DOM from scanning. So if we can
deny scan our injected parts in the DOM then maybe we can workaround
this recent IE problem.

As a test, I've backported the relevant functions and made a test with
this. (Ignoring the |"div.annotator-notice", "div.annotator-frame",
"div.annotator-adder"| elements and their descendants from the scan)

The good news: The IE exception for the |removeAllRanges| call is now
gone! 🍺
The bad news: We have new exceptions but now, inside the d-t-m, and
hopefully they are not browser relevant. So I'll ask a quick help from
@csillag https://github.com/csillag and hopefully we can release a
better IE compatible version of d-t-m and H.

The current exception is now inside d-t-m, |collectPositions()|,
pathInfo is not always available.

—
Reply to this email directly or view it on GitHub
#1234 (comment).

[gergely-ujvari]

2014.06.17. 13:29 keltezéssel, Kristof Csillag írta:

If our approach is to try to make IE work by doing extensive work inside
d-t-m,
then I suggest doing it on the shoot branch, which

• does not trigger any of those those IE errors
• compatible with dynamic pages
• can be ported more easily to the new dom-anchors-core library.

I am wary of adding more fixes to the master branch of the d-t-m, but I
will see what I can do later.
Nevermind. I have found the problem. Now I've...
new problems but with our authentication. So, leaving d-t-m for a while.
(I'll prepare a PR for the master branch)

(Right now I am focusing on the dom-anchors library.)

On 2014-06-17 13:22, gergely-ujvari wrote:

Adding a bit more info.
As @csillag https://github.com/csillag suggested the newer
implementations of the d-t-m (observer, shoot) contain a configurable
solution for ignoring parts of the DOM from scanning. So if we can
deny scan our injected parts in the DOM then maybe we can workaround
this recent IE problem.

As a test, I've backported the relevant functions and made a test with
this. (Ignoring the |"div.annotator-notice", "div.annotator-frame",
"div.annotator-adder"| elements and their descendants from the scan)

The good news: The IE exception for the |removeAllRanges| call is now
gone! 🍺
The bad news: We have new exceptions but now, inside the d-t-m, and
hopefully they are not browser relevant. So I'll ask a quick help from
@csillag https://github.com/csillag and hopefully we can release a
better IE compatible version of d-t-m and H.

The current exception is now inside d-t-m, |collectPositions()|,
pathInfo is not always available.

—
Reply to this email directly or view it on GitHub
#1234 (comment).

—
Reply to this email directly or view it on GitHub
#1234 (comment).

[gergely-ujvari]

It seems I was too optimistic.
Now, the adder button doesn't show up, d-t-m has problems within the performUpdateOnNode call (under IE, at least the initial scan had no problems).

Digging deeper.

[tilgovi]

By the way, I think you reported the issue to Rangy in the wrong place. That project has moved to GitHub, as it says front and center of the Google Code site.

[gergely-ujvari]

So, it seems that simply introducing the node ignore functions alone will not help us.
Both the observer/shoot branch has many-many improvements which are not on the master branch, therefore it'd be an endless debug to keep on continuing to try to have IE support that way.

So I suggest to have the d-t-m future conversation before making more effort to make the d-t-m master branch IE compatible.
Or, of course, if the IE guys tell us something very trivial I'll try that.

[gergely-ujvari]

@tilgovi: You're right. What got me confused that the googlecode site had issue status changes, so I thought it was alive too.
Reopened here: timdown/rangy#214

[tilgovi]

@gergely-ujvari do we deploy okay on IE with fuzzy anchoring disabled at this point? Is the app functional?

[csillag]

Yes. That has been the case since the 28th of May. (But to deploy for IE,
you need to specify a parameter, which kills fuzzy for all users, as
described in #1233, because you vetoed auto detection.)
2014.07.07. 22:43 ezt írta ("Randall Leeds" notifications@github.com):

@gergely-ujvari https://github.com/gergely-ujvari do we deploy okay on
IE with fuzzy anchoring disabled at this point? Is the app functional?

—
Reply to this email directly or view it on GitHub
#1234 (comment).

[tilgovi]

But to deploy for IE, you need to specify a parameter, which kills fuzzy for all users, as described in #1233, because you vetoed auto detection.

var ua = window.navigator.userAgent;
var msie = ua.indexOf("MSIE ");
var embed = document.createElement('script');

if (msie > 0) {
  embed.src = 'https://hypothes.is/embed.js?dtm=false';
} else {
  embed.src = 'https://hypothes.is/embed.js';
}

The above code is why I vetoed that solution. I would rather not complicate our embed code. Doing so makes it harder to turn on and off for testing and adds bloat to the payload of every embed even for users who don't care about IE support.

Please stop repeating that it must be for all users or no users. That's patently false.

[csillag]

The above code is why I vetoed that solution.

Well, obviously, by adding extra code to the product we ship, one can implement smarter behavior, compared to what we provide by default. However, that does not prove that our product is capable of the smart behavior; on the contrary, the fact that it needs an external fix to do the sane thing proves that our product lacks this ability by default. (Also, I still think that insisting on the external workaround proposed here introduces unnecessary responsibilities and overhead on the integrators, as described here.)

Please stop repeating that it must be for all users or no users. That's patently false.

Well, since you asked about something which has already been announced earlier, it was a reasonable assumption that you don't remember all the details - wasn't it? And hence, I gave you a short overview of the situation. To do that, it was only natural that I had to repeat things already said - wasn't it?

Also, in the thread at #1233, my claim was not that "it must be for all users or no users". I said that it's unwise to force the integrators add the required auto-configuration, instead of embedding it in the relevant parts of the code. (Especially since the required algorithm might change in the future.) You might disagree with that opinion, but it's not something which can be described as "patently false".

In the current thread, when I give a very short summary, I might have oversimplified the situation (by omitting to say that integrators might add external code to make up for the missing feature), so maybe you can describe my summary as incomplete or imprecise, but still not "patently false".

[tilgovi]

I misrepresented you when I said "must be [for all users]". Sorry.

Well, since you asked about something which has already been announced earlier, it was a reasonable assumption that you don't remember all the details

You announced that the Level 1 tasks were done. I did look at the checklist before I posted. I could not tell from that for sure that IE actually loaded and worked.

I felt I asked a direct, simple question and you seized an opportunity to remind me that you disagreed with me. That feels combative and obnoxious. I know I can do the same at times, so feel free to call me out.

[csillag]

On 2014-07-09 00:47, Randall Leeds wrote:

Well, since you asked about something which has already been
announced earlier, it was a reasonable assumption that you don't
remember all the details

You announced that the Level 1 tasks were done. I did look at the
checklist before I posted. I could not tell from that for sure that IE
actually loaded and worked.

In the same announcement which said that level 1 was done, I also wrote that

 "now we are good to go with IE. (But see the limitations described

in the OP.)"

And in the OP, I have this definition:

Tasks for "level 1" support (no dtm, no position or fuzzy strategy):

So I had the impression that we have communicated the situation clearly.

That's why I assumed that you forgot the details. (It has been 1.5
months, so it would be perfectly normal.)

I felt I asked a direct, simple question and you seized an opportunity
to remind me that you disagreed with me.

That was not my purpose. I wanted to give a short summary of the
situation, highlighting current limitations and problems, hinting at
possible next steps. I mentioned the veto as an excuse for shipping a
solution that I felt was painfully incomplete. (This limitation is the
sole reason I can't answer a simple question like "does it work?" with a
simple affirmative.) Generally, I don't like to present something as
"ready" when I don't feel it's ready. Since in this case I had to, I
felt that I needed some extra justification. That was probably the wrong
guess, because you would have remembered this bit anyway.

[gergely-ujvari]

Let's get back to this when we decide what to do with the current d-t-m problems.