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

Add private method _getSelection #219

Closed
wants to merge 2 commits into from

Conversation

hongymagic
Copy link
Contributor

Adds private method _getSelection(iframe) as requested in this comment.

var selection = _getSelection(self.editorIframe);

Will return a native Selection object. There are a couple of things to note going forward, I'll keep them short here:

  1. To get caret position, we will need to query the selection object for text ranges. Remember the ugly offset calculation? This will be required to get the accurate caret position
  2. Other commonly tested paths such as: if (selection.rangeCount > 0) range = selection.getRangeAt(0) could also be extracted out as a private function. I will leave that out for now

@hongymagic
Copy link
Contributor Author

I really should've squashed them. 👎 to me.

@OscarGodson
Copy link
Owner

Meh, personally I like the history there instead. So, 👍 to you!

For now, my main use case is–as I said–to simply save the position on preview as is then return to that state when you flip back to the editor. Since nothing is changing between editing and previewing–and if someone did I dont mind it not being in the same place–this should suffice, right? I'm still battling my way through, but do you know offhand how to set the cursor after getting it from this in a cross browser way? If you're busy, no worries :)

@OscarGodson
Copy link
Owner

Also, you wont see this merge until I fix the bug, but this is merged into my local branch. Huge thanks!

@OscarGodson
Copy link
Owner

Last question, did you check this in Firefox? Probably just me, but it seems to always be giving me a "0" as the selection location.

@hongymagic
Copy link
Contributor Author

I have a feeling its related to this: bug.

Can you give me a bit of context as to where/how you're using _getSelection? I have noticed that when I hide the editorIframe, the selection object seems to reset itself. For example:

EpicEditor.prototype.preview = function () {
  var selection = _getSelection(...);
  selection.focusOffset; // 40
  window.ddd = selection;

  ...

  // Hide the editorIframe
};

ddd.focusOffset; // 0

Feel free to share branch code, I would love to help and get this caret stuff going too.

@OscarGodson
Copy link
Owner

Here's my WIP: 2d8bf2b

A couple things.

  1. In chrome if I log out self.eeState.selection.focusOffset in edit() I get a number. Firefox it's always just 0.
  2. _setCaretPosition(self.editorIframe, selection, 10); doesn't seem to do anything for me still even tho the number is hardcoded.

@hongymagic
Copy link
Contributor Author

This appears like a bug with Firefox and same bug here. It does not look like this bug is going to get fixed any time soon judging from Boris' comment on the second bug.

If we cannot maintain reference to:

selection.anchorNode
selection.anchorOffset

After the iframe is hidden, this is not going to work in Firefox.


There are some ugly hacks which I suspect can solve this in Firefox... :sigh:

@OscarGodson
Copy link
Owner

@hongymagic there's a couple things I can think of. The easiest, in theory, would be to save the index (number) before going to preview. When you flip back manually set the cursor to that place. The problem tho was I can't move the cursor at all, displayed or not, in Firefox.

second, we could not display none it and instead maybe visibility hidden and lower z index? Maybe -99999999px it to the left? Would these work you think?

@hongymagic
Copy link
Contributor Author

The reason why it doesn't work on Firefox is because the index (number) depends on a node/element. In Firefox, the element becomes null when iframe is hidden.

UPDATE: here's an example of setting the caret position in Firefox

So this leaves us with the second solution. I think the second solution will work. And the best thing about this second way is that you won't need _getSelection nor _setCaretPosition.

@OscarGodson
Copy link
Owner

Yeah, I think the second makes sense, but im not sure I follow on why number 1 wouldn't work: http://jsbin.com/exofub/1/edit

display'd none, then display'd block and it worked.

@hongymagic
Copy link
Contributor Author

It needs to be in the iframe for the Firefox bug to show up

@OscarGodson
Copy link
Owner

@hongymagic for my own knowledge, why the hell doesn't this work on chrome even? http://jsbin.com/exofub/2/edit

@hongymagic
Copy link
Contributor Author

See: revised version which works on both Chrome and Firefox.


In short:

HTML Elements that we know and use (<a>, <br>…) are all of same nodeType called ELEMENT_NODE. In practice though, there are 12 different nodeTypes being used in any HTML document. Here's a simple HTML markup:

<body>1234567</body>

But here's how the DOM may look like:

ELEMENT_NODE
  - TEXT_NODE: { text: 1234567 }

So if you want to set the caret between 1 and 2, then I need to tell the browser to set cursor position at 1 starting from the TEXT_NODE.

It gets worse obviously as the HTML grows.


This is part of the reason why I wanted to remove browser generate HTML tags (like br etc) completely and only have one node inside the editorIframeDocument.body, which would be a TEXT_NODE. Then we won't have to worry about these extra artifacts that make things that are trivial, extremely difficult and buggy.

@OscarGodson
Copy link
Owner

Daaaaammmmmnnnnn. Im glad you gave me this super minimal version so I could visualize this better. This isn't going to be fun once we start doing more DOM editing stuff. Thanks man!

@Gankra
Copy link
Collaborator

Gankra commented Jun 6, 2013

Reading these comments I'm not entirely clear on where we are vis-a-vis this feature or pull-request. If I tackle this, is there a "good" branch to base my work off of? Should all work on this just be scraped...?

@OscarGodson
Copy link
Owner

I'm not sure where @hongymagic is at this point with his work locally, but the "bug" I'm talking about in these comments I fixed and its in develop: dd4dc1b

The changes in this PR tho are pretty minimal.

@Gankra
Copy link
Collaborator

Gankra commented Jun 6, 2013

So what's blocking this pull now?

@OscarGodson
Copy link
Owner

Mostly that it'd be unused code as of now. There's nothing that'd really use it yet, and I wasn't sure when I'd be able to get to implementing all the selection API stuff.

@Gankra
Copy link
Collaborator

Gankra commented Jun 7, 2013

I started digging into how to implement the rest of the proposed API-- oh god the selection API and specs are horrible.

@OscarGodson
Copy link
Owner

Yeah... browser selection APIs are kinda a cluster.

@OscarGodson
Copy link
Owner

Since there hasn't been any progress in over 2 months now it looks like, I'm going to close this. @hongymagic, feel free to reopen if you're still working on this.

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

Successfully merging this pull request may close these issues.

None yet

3 participants