Extracting author from DOM could be more surgical if we detected itemprop="name"

[danielnixon]

Take https://lithub.com/why-are-writers-particularly-drawn-to-tarot/ as an example.

There are no meta tags or json-ld from which to extract the author, so Readability looks in the DOM. It matches this snippet:

<div class="author_info" itemprop="author" itemscope="" itemtype="http://schema.org/Person">
  <div class="author_name">By&nbsp;<a href="https://lithub.com/author/rochelle/" itemprop="url"><span itemprop="name">Rochelle&nbsp;Spencer</span></a></div>
  <div class="author_hr"><hr></div>
  <div class="publish_date">August 27, 2019</div>
</div>

It greedily takes the whole text contents, resulting in a byline of

By Rochelle Spencer August 27, 2019

If we were a bit more surgical and noticed that the [itemprop="author"] contained an [itemprop="name"] and then only took that name, we'd get an byline of just

Rochelle Spencer

Here's a patch. It uses querySelector -- which I realise is rarely used in Readability -- but I think it's well enough established now that we can rely on it being available.

       var itemprop = node.getAttribute("itemprop");
     }
 
-    if ((rel === "author" || (itemprop && itemprop.indexOf("author") !== -1) || this.REGEXPS.byline.test(matchString)) && this._isValidByline(node.textContent)) {
+    if (itemprop && itemprop.indexOf("author") !== -1) {
+      const nameItem = node.querySelector && node.querySelector('[itemprop="name"]');
+      if (nameItem && this._isValidByline(nameItem.textContent)) {
+        this._articleByline = nameItem.textContent.trim();
+        return true;
+      } else if (this._isValidByline(node.textContent)) {
+        this._articleByline = node.textContent.trim();
+        return true;
+      }
+    }
+
+    if ((rel === "author" || this.REGEXPS.byline.test(matchString)) && this._isValidByline(node.textContent)) {
       this._articleByline = node.textContent.trim();
       return true;
     }

[gijsk]

querySelector is not available in the JSDOMParser.js implementation, which is what gets used inside Firefox (on a worker thread, where there is no "real" DOM). So we can't really rely on it from that perspective. Building a fully-fledged CSS selector parsing and matching engine is also somewhat out of scope for the library.

It looks like this patch provides graceful fallback though, so we may still be able to take it... would you mind submitting it as a PR instead? That would also offer some visibility as to what (if anything) this change would do to the existing testcases.

[danielnixon]

Thanks for taking the time to explain that @gijsk.

I did notice precedent for querySelectorAll in _getAllNodesWithTag but that looks like it's just a performance boost if querySelectorAll happens to be available. It would probably be unwise to wind up in a state where the actual outcome is materially affected by whether querySelectorAll is available or not (which would happen with my patch above).

I can probably reformulate this in terms of repeated calls to _getNextNode with a check for itemprop === name applied to each node. I'll do that and get a PR up.

[danielnixon]

PR in terms of _getNextNode: #943