IE8 Fixes (part 2) #436

krusynth · 2014-09-18T20:42:20Z

Three small fixes here:

IE8: Annotator Widget CSS not working. #434 - CSS tweaks: Adding fallback colors for rgba, and swapping ::after for :after
IE8: error "Sorry we could not undefined this annotation" #435 - Enable CORS support in jQuery
Adder plugin (and maybe others) not working in IE8 #430 - Click event handler changes

For openannotation#435

For openannotation#434

per openannotation#430

tilgovi · 2014-09-18T22:31:23Z

css/annotator.css

This is the proper syntax anyway, AFAIK. http://www.w3.org/TR/CSS2/selector.html#before-and-after

Proper by modern standards, yes. But the alternate syntax is supported by everything. As-is, these buttons just aren't showing up at all in IE8 because these aren't matching. We could, alternately, refactor to not use ::after at all here?

No, I'm just saying that :after is correct and I don't know why ::after was here to begin with. If that was once the syntax, I never knew it.

It sounds like you thought I was saying ::after was proper. I think it isn't, if it ever was. I suspect it was because W3Schools thinks it is .

Ah, :after it was correct in CSS2 - no more in CSS3: http://www.w3.org/TR/selectors/#gen-content

Ah. CSS3 added support for :: because they're pseudo-elements rather than pseudo-classes. But originally, the syntax was the same it seems.

The ::after notation was introduced in CSS 3 in order to establish a discrimination between pseudo-classes and pseudo-elements. Browsers also accept the notation :after introduced in CSS 2.

Source: https://developer.mozilla.org/en-US/docs/Web/CSS/::after

per openannotation#428

These handlers (well, really only the mouseup handler) were returning false, thus stopping propagation of the events and breaking other handlers, as discussed in #431. Also incorporating a change in the button check, from e.which != 1 to e.which > 1 which seems to be necessary for IE8 support (see #436). Fixes #431.

nickstenning · 2014-09-30T10:52:44Z

I've cherry-picked the CSS changes as 14f56a8 and fd7bae0. I've cherry-picked the event handler changes to master as f0383c8.

That leaves only the CORS- and rangy-related commits to be merged. Could you rebase this branch against master, @krues8dr?

krusynth · 2014-09-30T21:39:50Z

Rebase was a total mess, there was just too much to resolve. I went for a straight merge which was pretty simple but we've got new failing tests in IE8 on that branch again now. I'll commit up what I've got.

nickstenning · 2014-09-30T21:46:48Z

@krues8dr If you do an interactive rebase, you should be able to drop commits efde1e1, 3104db7, and ba6228f, as they're already in the history.

Alternatively, if it's easier, I can start a new branch in this repository with the only remaining changes cherry-picked into it?

krusynth · 2014-10-01T15:03:35Z

I've got the merge working well enough to run on Chrome - but there's a lot of new code here and things have moved around. It doesn't look like there's a single quick fix for all of this in IE, I spent a while yesterday poking at it. That's all committed in our fork if someone else wants to have a go.

aron · 2014-10-15T08:58:30Z

@krues8dr @nickstenning whats the status on this guys?

krusynth · 2014-10-15T15:25:39Z

@aron Merged Nick's changes into our fork, but tests stopped passing in IE. He said he was going to have a look at those; I won't be able to in the immediate future.

aron · 2014-10-15T15:33:59Z

@krues8dr okay thanks for the update. Just want to ensure this branch doesn't rot, will catch up with @nickstenning.

krusynth · 2014-10-15T15:38:27Z

@aron The jQuery-CORS issue is probably the biggest thing to be worried about right now. The rest I would assume is manageable with the provided shims.

Which reminds me, I need to transfer our shims repo to openannotation but I don't have the privileges to do so. Can I transfer that to you to push over? I'd last talked to @tilgovi about it.

aron · 2014-10-15T15:45:22Z

@krues8dr as far as I know you'll need one of the @openannotation/owners

krusynth · 2014-10-15T15:46:06Z

@aron You are totally I right, I'd just assumed you were one. :) We can start our own cool-kids club instead.

nickstenning · 2014-10-23T23:33:42Z

Alright, so there were three parts to this pull request.

CSS fixes for IE. These have been merged.
Click event handler function fixes for IE. These have been merged.

The one remaining issue relates to support for CORS in IE. Unfortunately, this is not as simple as tweaking some of the innards of jQuery. IE8 and 9 do not support CORS using XMLHttpRequest. Instead, they have their own proprietary API, called XDomainRequest, which:

can only make GET/POST requests
does not allow the setting of custom request headers
permits only one Content-Type (text/plain) in requests (yes, seriously...)

This makes IE8/9 fundamentally incompatible with a cross-domain Annotator setup. That's not to say we can't support these browsers for integrated applications (where the document being annotated and the annotator store are on the same domain), but there isn't any easy way to support these browsers for, say, the bookmarklet.

IE10+ have native support for CORS through XMLHttpRequest.

All in all, I don't think there's anything remaining for us to merge in this PR, so I'll close it. There are still some outstanding test failures on IE8 but they will be dealt with in due course.

krusynth · 2014-10-23T23:37:18Z

Fair point in closing this, no complaints here. Someone still needs to takeover the shim repo too for this to be "done."

Also - just to recap and not to beat a dead horse - there must be a reason why forcing jQuery to think that there's support for CORS "magically" works in IE8. I'm not sure why this is, but it definitely does work for some reason.

nickstenning · 2014-10-23T23:46:42Z

Someone still needs to takeover the shim repo too for this to be "done."

I'm not entirely sure there's a need for a shim repo. I want there to be clear instructions on how to deploy Annotator for legacy IE as part of the project. As such, any shims we need we will import directly into the main repo, and include in an IE-compatible build. At the moment, it's just wicked-good-xpath and the tests pass on IE9/10/11. Obviously we'll need to pull in a couple more for IE8.

there must be a reason why forcing jQuery to think that there's support for CORS "magically" works in IE8.

Right, but I'm not particularly bothered about finding out why. The tests don't actually exercise anything to do with CORS as they run on a single domain. If I had to hazard a guess, it would be that there is error-checking in jQuery which consults $.support.cors when you first make a $.ajax call.

tilgovi · 2014-10-23T23:46:53Z

It could be that IE8-9 treat origins that share a hostname as the same origin. In other words, http://localhost:4000 and http://localhost:4001 might be considered the same origin from IE's perspective. Just a random guess.

1fc7024 changed this test from `which == 1` to `which > 1` with the explanation: > Also incorporating a change in the button check, from > > e.which != 1 > > to > > e.which > 1 > > which seems to be necessary for IE8 support (see openannotation#436). (I'm offline so can't check openannotation#436, but) this seems wrong: the only button we care about is the primary button, used to select text, and the commit and the message disagree anyway!

Bill Hunt added 6 commits September 18, 2014 15:52

Enabling CORS support in jQuery.

8d34b51

For openannotation#435

Adding fallback css colors for all rgba() values.

efde1e1

For openannotation#434

Replacing ::after with :after in css for older browsers.

3104db7

For openannotation#434

Modifying click event checks to be friendlier to older browsers.

ba6228f

per openannotation#430

Merge remote-tracking branch 'upstream/master'

730ee03

Disregard that semicolon, it never happened.

b84add0

tilgovi reviewed Sep 18, 2014
View reviewed changes

Adding rangy check in a few more places.

039754a

per openannotation#428

This was referenced Sep 20, 2014

IE8: Annotator Widget CSS not working. #434

Closed

Adder plugin (and maybe others) not working in IE8 #430

Closed

krusynth mentioned this pull request Sep 20, 2014

IE8: error "Sorry we could not undefined this annotation" #435

Closed

nickstenning mentioned this pull request Sep 21, 2014

Internet Explorer support (XPath, etc.) #173

Closed

Merge remote-tracking branch 'upstream/master'

a806357

This was referenced Sep 30, 2014

IE 9 Unit Test Failures #350

Closed

IE 10 Unit Test Failures #349

Closed

Merge from upstream

8de67c6

nickstenning closed this Oct 23, 2014

IE8 Fixes (part 2) #436

IE8 Fixes (part 2) #436

Uh oh!

Conversation

krusynth commented Sep 18, 2014

Uh oh!

tilgovi Sep 18, 2014

Choose a reason for hiding this comment

Uh oh!

krusynth Sep 18, 2014

Choose a reason for hiding this comment

Uh oh!

tilgovi Sep 18, 2014

Choose a reason for hiding this comment

Uh oh!

tilgovi Sep 18, 2014

Choose a reason for hiding this comment

Uh oh!

krusynth Sep 18, 2014

Choose a reason for hiding this comment

Uh oh!

tilgovi Sep 18, 2014

Choose a reason for hiding this comment

Uh oh!

nickstenning commented Sep 30, 2014

Uh oh!

krusynth commented Sep 30, 2014

Uh oh!

nickstenning commented Sep 30, 2014

Uh oh!

krusynth commented Oct 1, 2014

Uh oh!

aron commented Oct 15, 2014

Uh oh!

krusynth commented Oct 15, 2014

Uh oh!

aron commented Oct 15, 2014

Uh oh!

krusynth commented Oct 15, 2014

Uh oh!

aron commented Oct 15, 2014

Uh oh!

krusynth commented Oct 15, 2014

Uh oh!

nickstenning commented Oct 23, 2014

Uh oh!

krusynth commented Oct 23, 2014

Uh oh!

nickstenning commented Oct 23, 2014

Uh oh!

tilgovi commented Oct 23, 2014

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants