getAggregateText function sticks together text between different nodes

[ilya-khorev]

Hi

Please, look at my test case
https://jsfiddle.net/weirrrdy/xLcfnrpk/

I'd like to match the exact word "Highlighted", that's why there is a special metacharacter "\b" wrapping the regex expression. Semantically it doesn't necessary to put '\b' in this case but I'm using it in my application and it's required for the test case.

The expected behaviour here would be selecting both 'highlighted' words, however only the second phrase has matched.

The problem is the getAggregateText function has combined text of first two paragraphs into the following text: "HighlightedPhrase". Therefore the current regexp expression has skipped the word "Highlighted".

I would really appreciate if you fix this.

[padolsey]

Sorry for the massive delay on this. Am investigating now.

[ethantw]

A paragraph can be styled presentationally as an inline element in CSS. Likewise, an emphasis text run can be styled as a block element too.

Two adjacent ps should be handled just like two adjacent ems by default. Maybe it's better to add an additional option (element/selector-wise) for such cases.

[padolsey]

This is a tough one...

I'm happy to make it configurable, but it seems like the default behaviour should be what people typically expect, and people probably don't typically expect <p> elements to be treated as inline. But I'm very wary of making this assumption because it could lead people to a lot of undesirable behaviour.

[padolsey]

So, yeh, defaulting to "everything is inline" makes sense. Just trying to figure out how to make this easily configurable and obvious.

[padolsey]

The alternative is to assume everything is a block-level element by default, and then provide the opportunity to configure inline elements.

[ethantw]

The alternative is to assume everything is a block-level element by default, and then provide the opportunity to configure inline elements.

Do you mean with the boundary matcher \b only?

[padolsey]

Ok, so, this is what I've come up with: (Please let me know what you both think)

---

PROPOSAL

Everything stays the same as it currently is, by default. The findAndReplaceDOMText function will assume that it can bleed matches through ALL elements.

If someone wants to explicitly disallow this, e.g. in the case of block-level <p> elements, then they can set a custom boundary function that returns true in the case that either of its arguments are paragraphs (the boundary function will always be passed two elements so you can decide whether to create a boundary between them):

findAndReplaceDOMText(document.getElementById('test'), {
  find: RegExp('\\b' + 'highlighted' + '\\b', 'gi'),
  boundary: function(a, b) {
    return a.nodeName.toLowerCase() === 'p' || b.nodeName.toLowerCase() === 'p';
  },
  wrap: 'em'
});

You can also set boundary to true which will mean that NO matches bleed through elements, e.g. a match must exist in contiguous text nodes.

Additionally there will be an easy way to set ALL block-level borders as boundaries:

findAndReplaceDOMText(document.getElementById('test'), {
  find: RegExp('\\b' + 'highlighted' + '\\b', 'gi'),
  boundary: findAndReplaceDOMText.BLOCK_LEVEL_ELEMENTS,
  wrap: 'em'
});

(I imagine this will be the most common usage)

[ethantw]

Would you care to use Element.prototype.matches method instead of the fairly long nodeName comparison?

https://github.com/ethantw/fibre.js/blob/6857deac21/dist/fibre.js#L18-28

[padolsey]

Oo, yep, didn't realize that had such good support. Still a shame about IE8.

[ilya-khorev]

Thanks for the answer.

The second option is the best one, at least to me. That's what I'm looking for, if I understood you correctly.
boundary: findAndReplaceDOMText.BLOCK_LEVEL_ELEMENTS,

[padolsey]

Some progress: d41ae22 introduces something similar to my proposal above. Instead of 'boundaries' I've chosen the term 'contexts' and specifically a new option: forceContext which accepts either a boolean or a function that returns a boolean. With this concept you can prescribe that any specific element creates a new matching context (so that no matches bleed through the element in either direction).

So, for the block-level use-case, you can do:

findAndReplaceDOMText(document.getElementById('test'), {
  find: RegExp('\\b' + 'highlighted' + '\\b', 'gi'),
  forceContext: findAndReplaceDOMText.BLOCK_LEVEL_MATCH,
  wrap: 'em'
});

This means that any block-level elements will create new matching contexts. The findAndReplaceDOMText.BLOCK_LEVEL_MATCH function returns true if passed a traditionally block-level element.

You can set forceContext to your own function too, and even use Element.matches to figure out which elements to force a new context on.

---

If this looks good to you, I'll merge into master and push a new release. (Shall timeout in 48h and assume we're all happy to go ahead with it if I don't hear anything).

@weirdy: Here's your JSFiddle with the change added: https://jsfiddle.net/8dcqgcoa/1/

[ilya-khorev]

Awesome! Thank you, good work.

[ethantw]

Great! Thanks for the significant fix. I have two questions though.

• Even without the \b matcher they'll still be considered as another context?
• Also, I believe that br, details, dt, img, li, summary, tbody, td, th, thead, tr (, etc) are accidentally missed out?

The br and img (like hr) are necessary for cases like,

Highlight<img src="./path"><span>xxx</span>

https://developer.mozilla.org/en/docs/Web/HTML/Element