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

Undo ClassApplier to Range fails after Saving/Restoring Selection #202

Closed
timdown opened this issue Mar 23, 2014 · 3 comments
Closed

Undo ClassApplier to Range fails after Saving/Restoring Selection #202

timdown opened this issue Mar 23, 2014 · 3 comments

Comments

@timdown
Copy link
Owner

timdown commented Mar 23, 2014

From ben.get...@gmail.com on March 11, 2014 02:43:20

What steps will reproduce the problem? 1. Use Firefox (I'm on 27.0.1)
2. JSFiddle: http://jsfiddle.net/tx8Ly/ 3. Select the word 'Domain' (i.e., double-click it)
4. Click the button What is the expected output? What do you see instead? Expected: no error. The class applier successfully creates and removes decoration from the page.
Actual: An error: "Index or size is negative or greater than the allowed amount." What version of the product are you using? On what operating system? 1.3alpha.799; Windows Vista SP2; Firefox 27.0.1 Please provide any additional information below. The issue only occurs when the selected range ends at the end of an HTML element. You can see the same issue with the trailing period in the paragraph.

I can reproduce this in Firefox, but not in Chrome.

When the ClassApplier is applied to the range after save/restore, the endContainer is wrong. Instead of a Text node, the endContainer is HTMLHeadingElement. Then when attempting to undoToRange(), Rangy fails when "ensuring the range is still valid" at the end via range.setStartAndEnd().

At the moment, I'm unclear if the bug is that the HTMLHeadingElement is set as the endContainer, or that the setStartAndEnd fails with an error. I think it's the former. My reasoning: the endContainer is only set that way after save/restore on Firefox, and Chrome sets it as 'Text' regardless of the save/restore.

For good measure, here's the script I'm using (copied from fiddle):

var applier = rangy.createClassApplier("test");
var saved = rangy.saveSelection();
rangy.restoreSelection(saved);
var appliedRanges = applier.applyToRanges(rangy.getSelection().getAllRanges());

// This fails
var undoneRanges = applier.undoToRanges(appliedRanges);

Original issue: http://code.google.com/p/rangy/issues/detail?id=202

@timdown
Copy link
Owner Author

timdown commented Mar 23, 2014

From ben.get...@gmail.com on March 11, 2014 15:29:51

Still digging, but I've tracked the divergence down to the Selection.addRange() method. Both Chrome and Firefox seem to restore the same range:
startContainer: Text ("Example Domain")
startOffset: 8
endContainer: h1
endOffset: 1

Then, this range gets added to the current selection. For Chrome, the resulting range on the selection spans offsets 8 and 14 on the Text Node. For Firefox, the original range is copied onto the selection.

The difference happens in the line
this.nativeSelection.addRange(getNativeRange(range).cloneRange());
...specifically when adding the range to the native selection (the cloned ranges are the same).

What I'm still unclear on:

  • whether the restore is generally creating the wrong range
  • whether nativeSelection.addRange() is correct to modify the range in Chrome's case
  • whether it is setStartAndEnd() that's incorrect for failing on the original range (that Firefox maintains)

@timdown
Copy link
Owner Author

timdown commented Mar 23, 2014

From timd...@gmail.com on March 12, 2014 17:06:10

Status: Accepted

@timdown
Copy link
Owner Author

timdown commented Mar 23, 2014

From timd...@gmail.com on March 14, 2014 17:07:47

I tracked this down and fixed it.

Status: FixedInNextRelease

@timdown timdown closed this as completed Jul 5, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant