Commit 637757fa authored by Drini Cami's avatar Drini Cami Committed by Robert Knight

Allow whitespace-only <span>s during highlight

parent 98ced81f
...@@ -140,6 +140,23 @@ ...@@ -140,6 +140,23 @@
ullamcorper commodo. Nunc at nisi ut arcu euismod venenatis quis in nisl. ullamcorper commodo. Nunc at nisi ut arcu euismod venenatis quis in nisl.
</p> </p>
</div> </div>
<h3>Syntaxhighlighting Codeblocks</h3>
<h4>Highlight js - whitespace &lt;span&gt;s</h4>
<div class="testcase">
<div class="highlight-bash notranslate">
<div class="highlight">
<pre><span></span>tox<span class="w"> </span>-qe<span class="w"> </span>dev<span class="w"> </span>--<span class="w"> </span>sh<span class="w"> </span>bin/hypothesis<span class="w"> </span>--dev<span class="w"> </span>user<span class="w"> </span>admin<span class="w"> </span>&lt;username&gt;
</pre>
</div>
</div>
</div>
<h4>Codemirror - whitespace between &lt;span&gt;s</h4>
<div class="testcase">
<pre><code type="javascript"><span class="tok-keyword">import</span> <span class="tok-punctuation">{</span><span class="tok-variableName tok-definition">EditorView</span><span class="tok-punctuation">,</span> <span class="tok-variableName tok-definition">keymap</span><span class="tok-punctuation">}</span> <span class="tok-keyword">from</span> <span class="tok-string">"@codemirror/view"</span></code></pre>
</div>
{{{ hypothesisScript }}} {{{ hypothesisScript }}}
</body> </body>
</html> </html>
...@@ -234,10 +234,16 @@ export function highlightRange( ...@@ -234,10 +234,16 @@ export function highlightRange(
// inserting highlight elements in places that can only contain a restricted // inserting highlight elements in places that can only contain a restricted
// subset of nodes such as table rows and lists. // subset of nodes such as table rows and lists.
const whitespace = /^\s*$/; const whitespace = /^\s*$/;
textNodeSpans = textNodeSpans.filter(span => textNodeSpans = textNodeSpans.filter(span => {
// Check for at least one text node with non-space content. const parentElement = span[0].parentElement;
span.some(node => !whitespace.test(node.data)), return (
// Whitespace <span>s should be highlighted since they affect layout
(parentElement?.childNodes.length === 1 &&
parentElement?.tagName.match(/^SPAN$/i)) ||
// Otherwise ignore white-space only Text node spans
span.some(node => !whitespace.test(node.data))
); );
});
// Wrap each text node span with a `<hypothesis-highlight>` element. // Wrap each text node span with a `<hypothesis-highlight>` element.
const highlights: HighlightElement[] = []; const highlights: HighlightElement[] = [];
......
...@@ -235,8 +235,8 @@ describe('annotator/highlighter', () => { ...@@ -235,8 +235,8 @@ describe('annotator/highlighter', () => {
assert.equal(result[0].textContent, 'one two'); assert.equal(result[0].textContent, 'one two');
}); });
it('skips text node spans which consist only of spaces', () => { it('skips non-<span> text node spans which consist only of spaces', () => {
const el = document.createElement('span'); const el = document.createElement('div');
el.appendChild(document.createTextNode(' ')); el.appendChild(document.createTextNode(' '));
el.appendChild(document.createTextNode('')); el.appendChild(document.createTextNode(''));
el.appendChild(document.createTextNode(' ')); el.appendChild(document.createTextNode(' '));
...@@ -249,6 +249,37 @@ describe('annotator/highlighter', () => { ...@@ -249,6 +249,37 @@ describe('annotator/highlighter', () => {
assert.equal(result.length, 0); assert.equal(result.length, 0);
}); });
it('includes whitespace elements if <span> parent', () => {
// Real-world examples:
// - Codeblocks on https://h.readthedocs.io/en/latest/developing/install
// - Text layer on https://archive.org/details/goodytwoshoes00newyiala
const parent = document.createElement('div');
const word1 = document.createElement('span');
word1.textContent = 'one';
parent.appendChild(word1);
// This will be ignored because the parent is a div.
parent.appendChild(document.createTextNode(' '));
// This will not be ignored because the parent is a span.
const space = document.createElement('span');
space.textContent = ' ';
parent.appendChild(space);
const word2 = document.createElement('span');
word2.textContent = 'two';
parent.appendChild(word2);
const range = new Range();
range.setStartBefore(word1.childNodes[0]);
range.setEndAfter(word2.childNodes[0]);
const result = highlightRange(range);
assert.equal(result.length, 3);
assert.equal(result[0].textContent, 'one');
assert.equal(result[1].textContent, ' ');
assert.equal(result[2].textContent, 'two');
});
context('when the highlighted text is part of a PDF.js text layer', () => { context('when the highlighted text is part of a PDF.js text layer', () => {
it("removes the highlight element's background color", () => { it("removes the highlight element's background color", () => {
const page = createPDFPageWithHighlight(); const page = createPDFPageWithHighlight();
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment