New: reference collection #140

montehurd · 2018-05-09T22:39:00Z

Used to make it easier to show native adjacent reference panel carousel:

Usage details:

Used on iOS when a reference is tapped to show a native panel which lets user drag side to side to see any info for references which were adjacent to the tapped reference. ie: if a lone reference is tapped ie [4] we show a panel with its reference text, but if that reference has adjacent references ie [3][4][5] the panel we show can be dragged side to side to see the references text for [3] and [5].

To use:
Pass isCitation the href string of the tapped reference anchor, if it returns true (on iOS we use it to determine if the type of the clicked item is reference) call collectNearbyReferences with the document and the tapped anchor element to extract relevant references info (iOS example) including a referencesGroup array and the index of the selected element selectedIndex from that group.

The each item in referencesGroup will contain:

id - id of the reference
rect - ClientRect of the reference (so you can do a native overlay dimming everthing but the tapped reference link ie [4])
text - the text of the tapped reference link ie [4]
html - the html/text of the reference from the actual reference at the bottom of the page (in the animation above this is everything shown below the horizontal line in the panels)

iOS native interface details:

On iOS we use it as outlined above to extract adjacent reference info, and we send that info across the iOS version of the JS/Native bridge.
We then present a native interface showing the adjacent reference group, with one panel per reference (the bottom of the panel has a web view for showing the reference html, but it just looks like part of the panel, as you can see in the animation).
The native code also applies an overlay with a cutout around the tapped ref.
It also scrolls the web view up if the native panel it presents would cover the tapped reference, as you can see in the animation when I tap reference [11].

Notes:

I also moved some utilities to NodeUtilities.js - apologies that this wasn't as separate commit.
I had to fairly heavily refactor the iOS implementation to modernize it a bit and make it more testable.
On iOS on tablet we only show the single tapped reference (no side-to-side sliding panels or dark overlay):

…ferences. Used on iOS when a reference is tapped to show a panel which lets user drag side to side to see any info for references which were adjacent to the tapped reference. ie: if a lone reference is tapped "[4]" we show a panel with its reference text, but if that reference has adjacent references ie "[3][4][5]" the panel we show can be dragged side to side to see the references text for "[3]" and "[5]". To use: Pass `isCitation` the href string of the tapped reference anchor, if it returns true call `collectNearbyReferences` with the document and the tapped anchor element to get extracted references info including a `referencesGroup` array and the index of the selected element `selectedIndex` from that group. The each item in `referencesGroup` will contain: `id` - id of the reference `rect` - ClientRect of the reference (so you can do a native overlay dimming everthing but the tapped reference link ie "[4]") `text` - the text of the tapped reference link ie "[4]" `html` - the html/text of the reference from the actual reference at the bottom of the page

sharvaniharan · 2018-05-09T22:40:47Z

Thankyou @montehurd :) yay! i'm looking forward to use this!

montehurd · 2018-05-09T22:43:44Z

@sharvaniharan You're welcome! I just edited the description with a few more details of how we use this on iOS. Let me know if you have any questions or issues :)

This doesn't include the pagelib version bump because the PR moving those bits to the page lib isn't merged at the moment. As soon as wikimedia/wikimedia-page-library#140 is merged we can bump the pagelib version (as a new commit on this PR) and it should work.

niedzielski · 2018-05-18T13:03:10Z

src/transform/ReferenceCollection.js

+
+/**
+ * Checks if element has a child anchor with a citation link.
+ * @param  {!HTMLElement}  element


Extra space between type and name.

niedzielski · 2018-05-18T13:03:59Z

src/transform/ReferenceCollection.js

+ * @return {!boolean}
+ */
+const isWhitespaceTextNode = node =>
+  !(!node || node.nodeType !== Node.TEXT_NODE || !node.textContent.match(/^\s+$/))


After discussion, this may run faster but it would read more easily if the negation was pushed inward:

node && node.nodeType === Node.TEXT_NODE && node.textContent.match(/^\s+$/))

niedzielski · 2018-05-18T13:52:55Z

src/transform/ReferenceCollection.js

+const hasCitationLink = element => {
+  try {
+    return isCitation(getFirstChildAnchor(element).getAttribute('href'))
+  } catch (e) {


I think this was wrapped previously to prevent catch on empty links. I feel that needless try / catch blocks obscure how easy it is to reason about the code. What do you think of:

const anchor = element.querySelector('a') return anchor && anchor.getAttribute('href')

Instead of

try { return isCitation(getFirstChildAnchor(element).getAttribute('href')) } catch (e) { return false }

Hmm I don't recall the exact reason for the try/catch - been a while. I played with your alternative for a bit (adding call to isCitation where needed) but didn't get it to work (tests failed). Would be ok to leave this as-is for now?

~~Oh nevermind I think I fixed it...~~

Perfect! I'm glad we have a test case that fails! Now we can easily make the improvement right now and be confident in our code instead of punting it down the line and be confused later.

I looked into this myself. This try / catch block was obscuring the real underlying issue which was that adjacentNonWhitespaceNode() can return null and getFirstChildAnchor() expects nonnull.

Heya just a heads-up I reverted your change - see my comment here #140 (comment) for details.

niedzielski · 2018-05-18T13:58:40Z

src/transform/ReferenceCollection.js

+ * @param  {!string}  href
+ * @return {!boolean}
+ */
+const isCitation = href => href.indexOf('#cite_note') > -1


It may be more more precise to say anchor.hash.startsWith('#cite_note'). You may consider also moving the selector to the top of the file, as CITE_SELECTOR_PREFIX, as you have done with REFERENCE_SELECTOR.

I think I prefer to keep the method narrowly scoped vs passing it the entire anchor element. Open to refactoring this later if you feel strongly about it though :)

I added and used a constant per your suggestion.

niedzielski · 2018-05-18T13:59:53Z

src/transform/ReferenceCollection.js

+
+/**
+ * Determines if node is a text node containing only whitespace.
+ * @param  {!Node}  node


Extra space between type and name.

niedzielski · 2018-05-18T14:06:49Z

src/transform/ReferenceCollection.js

+const getRefTextContainer = (document, sourceNode) => {
+  const refTextContainerID = getFirstChildAnchor(sourceNode).getAttribute('href').slice(1)
+  const refTextContainer = document.getElementById(refTextContainerID)
+    || document.getElementById(decodeURIComponent(refTextContainerID))


Do we really need to check both decoded and undecoded?

Perhaps no longer, but I do recall some of these had funky encoding at one time...

niedzielski · 2018-05-18T14:20:36Z

src/transform/ReferenceCollection.js

+/**
+ * Reference item model.
+ */
+class ReferenceItem {


What do you think about using a JSDoc for typing instead of a class? Then creating a ReferenceItem is just matching the shape which requires explicit labeling of properties. For example, before:

class ReferenceItem { constructor(id, rect, text, html) { this.id = id this.rect = rect this.text = text this.html = html } } const reference = new ReferenceItem('a', {width, height}, 'foo', '<b>bar</b>')

With labeled properties:

/** * @typedef ReferenceItem * @prop {string} id * @prop {DOMRect} rect * @prop {?string} text * @prop {?string} html */ const reference = {id: 'a', rect: {width, height}, text: 'foo', html: '<b>bar</b>'}

Same thing for NearbyReferences.

I think I prefer using explicit language constructs over JSDoc shape conformance, though I'd be interested in chatting about this in the future. I think there are some benefits you get with classes, but there may be details I'm missing.

Usage of the class keyword exposes us to misusage of extends and we're not using any extra functionality here. It's just plain data. I think it's easier to reason about a simple JSON object than a class. What do you think?

Usage of the class keyword exposes us to misusage of extends

Using classes to model data is pretty common. How is classes being extendable practically risky here?

niedzielski · 2018-05-18T14:24:45Z

test/transform/ReferenceCollection.test.js

+    beforeEach(() => {
+      document.documentElement.innerHTML = `
+        <stuff>
+          \t<b id=one>one</b>


Sorry but why an escaped tab here?

The test is for adjacentNonWhitespaceNode(), so iirc I separated the elements with newlines and leading space "whitespace", so I thought it would be good to throw tabs in there too.

niedzielski · 2018-05-18T14:28:44Z

src/transform/ReferenceCollection.js

+    currentNode = adjacentNonWhitespaceNode(currentNode, siblingGetter)
+    if (hasCitationLink(currentNode)) {
+      nodeCollector(currentNode)
+      continue


If you invert this condition, you can replace the continue here with the break below.

Doh! Good catch! Fixed

niedzielski · 2018-05-18T14:29:34Z

src/transform/ReferenceCollection.js

+const collectAdjacentReferenceNodes = (node, siblingGetter, nodeCollector) => {
+  let currentNode = node
+  while (true) {
+    currentNode = adjacentNonWhitespaceNode(currentNode, siblingGetter)


You may wish to explicate in the JSDocs that the starting node is never collected.

Ah good point. Added

And that when there is only one node, the passed in collector is never called.

Doesn't it go without saying that if there no adjacent reference nodes, none will be collected?

sharvaniharan · 2018-05-29T21:43:07Z

@montehurd works on Android now! @niedzielski will let you merge once the changes you have requested are in!

niedzielski · 2018-06-01T17:04:11Z

Re-review pending follow-ups from @montehurd.

…starting node.

montehurd · 2018-06-06T23:52:48Z

@niedzielski I think I replied to and/or pushed commits to all your comments. Should be good for a re-review when you have time. Thanks again for the great feedback!

sharvaniharan · 2018-06-11T17:17:43Z

@niedzielski @montehurd can we merge this?

Many JavaScript developers agree to favor strict equality (===) to loose equality (==). This refactor makes the same argument for tests that strict is generally safer.

niedzielski · 2018-06-11T16:29:00Z

src/transform/ReferenceCollection.js

+/**
+ * Reference item model.
+ */
+class ReferenceItem {


Usage of the class keyword exposes us to misusage of extends and we're not using any extra functionality here. It's just plain data. I think it's easier to reason about a simple JSON object than a class. What do you think?

niedzielski · 2018-06-11T17:25:31Z

test/transform/ReferenceCollection.test.js

+const ReferenceCollection = pagelib.ReferenceCollection
+
+const referenceGroupHTML = `
+  <sup id="cite_ref-a" class="reference"><a id='a1' href="#cite_note-a">[4]</a></sup>


There's mixed single and double quotes in this string and others. Can we use single?

niedzielski · 2018-06-11T18:02:49Z

src/transform/ReferenceCollection.js

+ * @param {!HTMLElement} element
+ * @return {?HTMLAnchorElement}
+ */
+const getFirstChildAnchor = element => element.querySelector('A')


Heya!

I wanted to share my perspective on replacing comments with functions as you've done here.

In general, I think it's great practice to use good naming on our symbols and, of course, abstraction for the win. However, I probably wouldn't write a comment for querying the first anchor of a DOM subtree since it's so commonplace as to be un-noteworthy and the abstraction here does nothing. Function calls may now read more like English but they also obscure the underlying simplicity of what's actually happening here with a new custom DSL and greatly inflate the verbosity of the code which is more of a decrease to readability than an increase.

Consider what happens also when the second child anchor is needed in some new patch. Do we make a new function that sits alongside getFirstChildAnchor() called getSecondChildAnchor()? That seems bad enough but when the third anchor is needed, suddenly we might wonder if getNChildAnchor() should be added which takes an argument, n. Now we have three functions that all boil down to querySelector / querySelectorAll and we still need to pass an argument.

I think we both feel very comfortable working with querySelector / querySelectorAll so my opinion is that we should favor their direct usage unless we're doing something extra that we want to capture in a function.

Ya this was overly abstracted. I removed it. Good catch :)

niedzielski · 2018-06-11T18:12:47Z

test/transform/ReferenceCollection.test.js

+    })
+    it('correctly affirms child anchor does not have citation link', () => {
+      const element = document.querySelector('#cite_ref-b')
+      assert.equal(hasCitationLink(element), false)


I replaced all these calls to assert.equal with assert.strictEqual since for the same reason we favor === to ==. 👍

Coolio! Thanks!

niedzielski · 2018-06-11T18:14:49Z

src/transform/ReferenceCollection.js

+const hasCitationLink = element => {
+  try {
+    return isCitation(getFirstChildAnchor(element).getAttribute('href'))
+  } catch (e) {


Perfect! I'm glad we have a test case that fails! Now we can easily make the improvement right now and be confident in our code instead of punting it down the line and be confused later.

I looked into this myself. This try / catch block was obscuring the real underlying issue which was that adjacentNonWhitespaceNode() can return null and getFirstChildAnchor() expects nonnull.

niedzielski · 2018-06-11T18:26:14Z

src/transform/ReferenceCollection.js

+const collectAdjacentReferenceNodes = (node, siblingGetter, nodeCollector) => {
+  let currentNode = node
+  while (true) {
+    currentNode = adjacentNonWhitespaceNode(currentNode, siblingGetter)


And that when there is only one node, the passed in collector is never called.

montehurd · 2018-06-12T12:11:49Z

@niedzielski Heya! Just a head's-up I reverted 22bfca3 as it didn't work. I'll take a peek at the cause and undo the revert if I can suss it out :)

Edit: So it looks like it's not working because you're trying to call querySelector on a text node. btw sorry if some of these bits were a little hard to follow - I've tried to make it more grok-able than the original but there's certainly room for further improvement :)

This reverts commit 22bfca3.

niedzielski · 2018-06-22T14:07:23Z

Edit: So it looks like it's not working because you're trying to call querySelector on a text node. btw sorry if some of these bits were a little hard to follow - I've tried to make it more grok-able than the original but there's certainly room for further improvement :)

Great! Thanks for diagnosing. I've added a test case for the scenario you've described and as I understand the desired behavior, and made the fix. Please be sure to check it out!

montehurd · 2018-06-27T22:32:00Z

@niedzielski Your 5ae2c77 commit worked! Thanks!

If you have a sec there are a couple questions above:
#140 (comment)
#140 (comment)

montehurd · 2018-06-28T14:43:00Z

@niedzielski btw thanks again for all the great comments and CR!!!

sharvaniharan · 2018-06-29T12:48:53Z

@niedzielski @montehurd merging this! thanks for all the review work you guys did!! 💯

- Added a ViewPager to view grouped citations - integrated with the PR for grouped citations : wikimedia/wikimedia-page-library#140 Bug: T172283 Change-Id: Id1302b4a4ee0ce5553d397301ce17c528c08d07d

montehurd · 2019-02-11T20:14:05Z

I mentioned this PR on...
https://phabricator.wikimedia.org/T164312

montehurd added 2 commits May 9, 2018 15:08

Update changelog.

2950709

montehurd requested review from niedzielski and sharvaniharan May 9, 2018 22:39

montehurd requested review from joewalsh and natharateh May 9, 2018 22:45

montehurd mentioned this pull request May 9, 2018

Use new pagelib reference collection bits. wikimedia/wikipedia-ios#2295

Merged

montehurd and others added 2 commits May 9, 2018 16:24

Remove linebreak.

61a802a

Fix: use browser ESLint environment

56adc49

niedzielski suggested changes May 18, 2018

View reviewed changes

montehurd and others added 3 commits May 29, 2018 13:43

Fix parsoid content ref extraction.

f3d8e67

Fix changelog merge conflict.

08831a2

Merge branch 'master' into new/reference-collection

812f753

montehurd added 5 commits June 6, 2018 13:44

Remove extra JSDoc spaces.

ad416cf

Simplify isWhitespaceTextNode logic.

fbf6be5

Use constant for cite hash.

d1f91cf

Invert continue/break logic.

395f79d

Added comment clarifying adjacent ref node collector doesn't collect …

b911219

…starting node.

niedzielski added 2 commits June 11, 2018 12:35

Chore: replace assert.equal with assert.strictEqual

c17afb1

Many JavaScript developers agree to favor strict equality (===) to loose equality (==). This refactor makes the same argument for tests that strict is generally safer.

Fix null dereference in references

22bfca3

niedzielski suggested changes Jun 11, 2018

View reviewed changes

montehurd added 2 commits June 12, 2018 14:26

Revert "Fix null dereference in references"

a39ce13

This reverts commit 22bfca3.

Remove overly abstracted method.

e1e06a8

montehurd force-pushed the new/reference-collection branch from 7fec1a6 to e1e06a8 Compare June 12, 2018 12:34

Fix: RefereneCollection text Node processing

5ae2c77

montehurd added 4 commits June 27, 2018 14:55

Temporarily remove changelog update.

1cbb4d3

Merge branch 'master' into new/reference-collection

1a3ba33

Re-add changelog update.

7262cb1

Remove some double quotes.

1442dcd

niedzielski approved these changes Jun 28, 2018

View reviewed changes

sharvaniharan merged commit 2e7d7d0 into master Jun 29, 2018

sharvaniharan deleted the new/reference-collection branch June 29, 2018 12:49

New: reference collection #140

New: reference collection #140

Conversation

montehurd commented May 9, 2018 • edited

Usage details:

iOS native interface details:

Notes:

sharvaniharan commented May 9, 2018

montehurd commented May 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

montehurd Jun 6, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

montehurd Jun 6, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharvaniharan commented May 29, 2018

niedzielski commented Jun 1, 2018

montehurd commented Jun 6, 2018

sharvaniharan commented Jun 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

montehurd commented Jun 12, 2018 • edited

niedzielski commented Jun 22, 2018 • edited

montehurd commented Jun 27, 2018

montehurd commented Jun 28, 2018

sharvaniharan commented Jun 29, 2018

montehurd commented Feb 11, 2019 • edited

montehurd commented May 9, 2018 •

edited

montehurd Jun 6, 2018 •

edited

montehurd Jun 6, 2018 •

edited

montehurd commented Jun 12, 2018 •

edited

niedzielski commented Jun 22, 2018 •

edited

montehurd commented Feb 11, 2019 •

edited