Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IE8: selection methods not available. #4

Closed
krusynth opened this issue Sep 5, 2014 · 18 comments
Closed

IE8: selection methods not available. #4

krusynth opened this issue Sep 5, 2014 · 18 comments

Comments

@krusynth
Copy link

krusynth commented Sep 5, 2014

In IE8, things like document.createRange and window.getSelection don't exist. Per the discussion over on Annotator openannotation/annotator#412 (comment) we're suggesting users use the Rangy plugin to fill these gaps - but that doesn't polyfill window or document, it just provides a standard interface instead a la jQuery.

For instances like this and this I think it would make sense to test if rangy is being used and if not, default to the current methods. E.g.:

win = rangy ? Util.getGlobal()
sel = win.getSelection() # Get the browser selection object

Relates to openannotation/annotator#428

@krusynth
Copy link
Author

krusynth commented Sep 9, 2014

One more instance.

krusynth pushed a commit to opengovfoundation/xpath-range that referenced this issue Sep 10, 2014
krusynth pushed a commit to opengovfoundation/xpath-range that referenced this issue Sep 10, 2014
@krusynth krusynth mentioned this issue Sep 10, 2014
@csillag
Copy link
Contributor

csillag commented Sep 18, 2014

@nickstenning @tilgovi What do you think?

krusynth referenced this issue in openannotation/annotator Sep 19, 2014
@tilgovi
Copy link
Member

tilgovi commented Sep 19, 2014

Seems fine to me.

@tilgovi
Copy link
Member

tilgovi commented Sep 19, 2014

But lets call the variable something other than win... not sure what... maybe ctx?

@krusynth
Copy link
Author

That's certainly doable - but it is a replacement for window in most of these cases.

@tilgovi
Copy link
Member

tilgovi commented Sep 19, 2014

Yeah it's not a big deal either way. These functions would typically be
on the window, of course. But here they're not, so the win variable name
made me do a little wtf.
On Sep 19, 2014 2:53 PM, "Bill Hunt" notifications@github.com wrote:

That's certainly doable - but it is a replacement for window in most of
these cases.


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

@nickstenning
Copy link
Member

I understand the need for a replacement for window.getSelection() but I don't understand why this issue is on this repository. The classes in this repository operate on selection objects, but they don't (I don't think) need to know where they came from, do they?

@krusynth
Copy link
Author

@nickstenning This repo calls out to window.geSelection directly, so yes, some sort of replacement is needed. (See my patches above.)

@tilgovi
Copy link
Member

tilgovi commented Sep 21, 2014

That's maybe a good thing for us to work toward changing.
On Sep 20, 2014 6:43 AM, "Bill Hunt" notifications@github.com wrote:

@nickstenning https://github.com/nickstenning This repo calls out to
window.geSelection directly, so yes, some sort of replacement is needed.
(See my patches above.)


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

@nickstenning
Copy link
Member

Am I being an idiot? The only reference I see to getSelection in this repository is in the function Util.readRangeViaSelection, which in turn is only used in the tests.

@krusynth
Copy link
Author

There's also createRange. opengovfoundation@7e3dd65

@nickstenning
Copy link
Member

So, it doesn't call getSelection in a way that matters. The tests can probably be modified.

This repository does however, call document.createRange in NormedRange#toRange. This is only used in one place in Annotator and can easily be removed.

@krusynth
Copy link
Author

@nickstenning you clearly understand the problem better than I do, feel free to have at it and I can amend my PR to revert my proposed changes here.

nickstenning added a commit to openannotation/annotator that referenced this issue Sep 21, 2014
The `toRange()` method of `NormalizedRange` is the only point of
coupling between the DOM implementation of the W3C Range APIs, and the
xpath-range library. By inlining the tiny amount of code need to convert
a `NormalizedRange` into a DOM Range in the text selector, we can remove
this coupling.

This was motivated by openannotation/xpath-range#4.
@nickstenning
Copy link
Member

Okay, brilliant. Thank you, @krues8dr, for identifying this coupling. I've submitted #8 here to refactor the test, and have committed openannotation/annotator@e28f193 to remove the need for the NormalizedRange#toRange() method. With #8 merged, we can remove this method from this repository and with it the last explicit reference to the W3C Range APIs, leaving rangy detection to be done elsewhere.

@krusynth
Copy link
Author

@nickstenning Great, so that's half of it - but there's still getSelection to deal with as well. And also this repo will need to be refactored for that change.

@nickstenning
Copy link
Member

There are no uses of getSelection in this repository that will remain after #8 is merged and NormalizedRange#toRange() is removed.

@krusynth
Copy link
Author

Great, so I've merged that in to our fork and tests are still working. I'm at a conference atm so I can't dig too much on this right now, but it looks good at first glance.

@tilgovi
Copy link
Member

tilgovi commented Sep 25, 2015

It sounds like this was resolved. Please re-open if that isn't so.

@tilgovi tilgovi closed this as completed Sep 25, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants