Find suitable z-index for the adder

[esanzgar]

This PR incorporates a solution to programatically find a z-index for the adder. Before this, the z-index of the adder was a hard-coded value, which didn't work for some websites (see for example #2009).

We evaluated the option of finding the greatest z-index in the page. However, this takes too long for heavy pages.

This solution in this PR is not bulletproof, but probably works for the most part of the websites. It uses the greatest z-index of five nearby elements: left-top, left-bottom, middle-center, right-top, right-bottom elements around the adder.

Resolves #2009

[robertknight]

I have yet to figure out exactly why using attachTo: document.body failed, but using the following pattern which is quite common in our tests avoids the issue:

diff --git a/src/annotator/test/adder-test.js b/src/annotator/test/adder-test.js
index 5ee39cc98..7f0d54cd5 100644
--- a/src/annotator/test/adder-test.js
+++ b/src/annotator/test/adder-test.js
@@ -32,6 +32,17 @@ function revertOffsetElement(el) {
 }
 
 describe('findHighestZindex', () => {
+  let container;
+
+  beforeEach(() => {
+    container = document.createElement('div');
+    document.body.appendChild(container);
+  });
+
+  afterEach(() => {
+    container.remove();
+  });
+
   it('returns zero z-index (default value) if no element is passed)', () => {
     assert.strictEqual(findHighestZindex(undefined), 0);
     assert.strictEqual(findHighestZindex(null), 0);
@@ -44,7 +55,7 @@ describe('findHighestZindex', () => {
         <div className="child1" style={{ zIndex: 5 }} />
         <div className="child2" style={{ zIndex: 15 }} />
       </div>,
-      { attachTo: document.body }
+      { attachTo: container }
     );
 
     const parent = wrapper.find('.parent').getDOMNode();

The reason for the above pattern of putting the cleanup in an afterEach is that it ensures it happens even if the test fails, reducing the probability that changes made to the environment by the failed test cause failures elsewhere.

[codecov]

Codecov Report

Merging #2585 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #2585   +/-   ##
=======================================
  Coverage   97.28%   97.29%           
=======================================
  Files         197      197           
  Lines        7300     7311   +11     
  Branches     1615     1616    +1     
=======================================
+ Hits         7102     7113   +11     
  Misses        198      198           

Impacted Files	Coverage Δ
src/annotator/adder.js	96.70% <100.00%> (+0.45%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2414a08...d5f7443. Read the comment docs.

[robertknight]

Hi Eduardo,

I had another look over this. I suggested some minor changes but I also noted a scenario which was highlighted by the test page that you added that might require re-thinking the approach a little.

[esanzgar]

I made a more robust and simple implementation.

[robertknight]

Hi Eduardo,

This looks nice and simple. We'll need a fallback for browsers that don't support document.elementsFromPoint since it requires, for example, quite recent versions of Firefox and Safari (>= 12). Returning a static value in that case would be fine.

Can you also add a few tests. These can mock document.elementsFromPoint to make things easier.

[esanzgar]

@robertknight, I fully agree about (1) returning a default static value (as we were doing previously) in case of document.elementsFromPoint is not available, and (2) adding more tests.

[esanzgar]

On the second commit I provided a fallback option in case document.elementsFromPoint is not defined.
IE and Edge supports a non-standard version of this method.

[robertknight]

Hi Eduardo,

This looks good. I think the comment for _findZindex could be improved though to explain why it is doing what it does. I believe the comments about Opera on MDN are out of date, although the value of 2^15 is a perfectly reasonable fallback value.

In tests we generally avoid testing private methods, although this may occasionally be OK if it is unlikely to get in the way of refactoring the code in future. I suggested a way that you could test the z-index without calling _findZindex directly.

[robertknight]

Hi Eduardo - This looks good. Can you update the description of the describe(... block in the test following the change to use only public API of the class. Also, I would suggest to remove the reference to MDN and Opera because that information is long out of date. 2^15 is effectively just "an arbitrary large value that will work almost all web pages". Other than these two small items, this is good to merge.