joseph / Monocle

A silky, tactile browser-based ebook JavaScript library.
http://monocle.inventivelabs.com.au
MIT License
743 stars 200 forks source link

Extended moveTo() #20

Closed aronwoost closed 11 years ago

aronwoost commented 13 years ago

The xpath extension was fabulous. However, it does not work with textnodes.

I added the ability to jump not only to element nodes (achievable with xpath) but also to text nodes.

BTW: This is the first and only change we made to monocle so far. :) Very robust framework you build there. Let's keep it up!

joseph commented 13 years ago

Hey, thanks Aron.

Can you give me an example of what this offers over and above XPath?

My primary reasons for not using objects in loci are that they are ephemeral and document-specific. So you run in to big problems if the locus refers to a component that is no longer active, or in the case where you have two or more pageDivs in a flipper — since the object will only be in one of those pages.

Essentially, XPath is a component-neutral and state-neutral string description of a document location. You can reach text nodes in XPath with text(). I think we should perhaps concentrate on a way to simply and correctly derive the XPath of a given object (at the moment I just assign the node or its parent element an id), rather than attempting to work directly with object coordinates.

aronwoost commented 13 years ago

Hi Joseph,

Anything you said makes sense. And yes, you're right, xpath does (of course) finds textnodes. Also with node().

However, another important thing I maybe didn't point out is that we need to have the ability to jump to a certain offset inside a textnode. (See range stuff in component.pageForClientRect() )

Since we need to pass the offset somehow, we can not use a simple string.

Any suggestions welcome.

joseph commented 13 years ago

In that scenario for Booki.sh search, I've just split the text node.

Assuming the text node has been assigned to 'leftNode', and assuming we are looking for the first instance of the string assigned to 'query':

  var idx = leftNode.nodeValue.toLowerCase().indexOf(query.toLowerCase());
  var midNode = leftNode.splitText(idx);
  var rightNode = midNode.splitText(query.length);

You can then construct the XPath to point to midNode. (In fact, what I do for Booki.sh search is insert a yellow span element after leftNode and append midNode to it. Then I give the span an id, making the XPath trivial.)

I do this minor DOM manipulation on all the flipper's visiblePages(), so that the XPath will work for all of them.

Though there's no real problem with fragmentation of text nodes, you can subsequently repair the containing element (after going to the locus via XPath) with:

  leftNode.parentNode.normalize();

In short: I'm not completely against objects in loci, but I'm just wondering if it can't be done more simply with the existing locus options. What do you think?

aronwoost commented 13 years ago

The advantage of our approach is that it's very exact. You don't need a query to jump to a certain position. Say you want to save a position for later use. You would have to make sure, that the query is unique enough. No "and" or " " etc.

Also you don't have to rely on XPath. I read that XPath is kinda slow in iOS devices (although I havent made hardcore tests yet).

Don't worry, it's totally fine if you decide to leave this feature out of the framework for now. Maybe I'll come back on a later point. I only thought since you was begging for a pull request... ;)

joseph commented 13 years ago

Well of course there's no reason not to split the text node on some other criteria — such as a precise character offset:

var index = 132;
var length = 14;
var midNode = leftNode.splitText(index);
var rightNode = midNode.splitText(length);

I guess the thing I'm trying to avoid is having loci that refer to the contents of a particular component frame. At the moment, when you jump by locus, it reuses the top component frame (visiblePages[0] or whatever) — but there's no guarantee it will always do this.

In fact, if it were to mimic the behaviour of iBooks, for eg, it would use the underneath frame (something that's been on my list for a while with the Slider flipper). I think that would break this patch, unless I'm missing something?