finished specs for Box.js and made zoom handle level 0 correctly + make it more clear when zoom cannot zoom because it cant get width and height of the element

3 files changed, 272 insertions, 124 deletions

diff --git a/spec/spec/types/Box.js b/spec/spec/types/Box.js
index 34b1e20..e220cea 100644
--- a/spec/spec/types/Box.js
+++ b/spec/spec/types/Box.js
@@ -1,15 +1,14 @@
-/* globals describe, expect, it, jasmine, container */
+/* globals describe, expect, it, beforeEach, afterEach, spyOn, jasmine, container */
 
 import {
   Box,
   Matrix,
   Rect,
+  G,
   makeInstance as SVG
 } from '../../../src/main.js'
-import { getMethodsFor } from '../../../src/utils/methods.js'
-import { withWindow } from '../../../src/utils/window.js'
-
-const { zoom, viewbox } = getMethodsFor('viewbox')
+import { withWindow, getWindow } from '../../../src/utils/window.js'
+import { isNulledBox, domContains } from '../../../src/types/Box.js'
 
 const { any, objectContaining } = jasmine
 
@@ -19,122 +18,181 @@ const { any, objectContaining } = jasmine
 // }
 
 describe('Box.js', () => {
-  describe('()', () => {
-    it('creates a new Box with default attributes', () => {
-      const box = new Box()
-      expect(box).toEqual(any(Box))
-      expect(box).toEqual(objectContaining({
-        width: 0, height: 0, x: 0, y: 0, w: 0, h: 0, cx: 0, cy: 0, x2: 0, y2: 0
-      }))
+  describe('isNulledBox', () => {
+    it('returns true if x, y, with and height is 0', () => {
+      expect(isNulledBox({ x: 0, y: 0, width: 0, height: 0 })).toBe(true)
     })
-  })
 
-  describe('init()', () => {
-    it('inits or reinits the box according to input', () => {
-      expect(new Box().init(1, 2, 3, 4).toArray()).toEqual([ 1, 2, 3, 4 ])
+    it('returns false if one or more of x, y, with and height is not 0', () => {
+      expect(isNulledBox({ x: 0, y: 0, width: 0, height: 1 })).toBe(false)
+      expect(isNulledBox({ x: 0, y: 1, width: 0, height: 1 })).toBe(false)
     })
+  })
 
-    it('works with array input', () => {
-      expect(new Box().init([ 1, 2, 3, 4 ]).toArray()).toEqual([ 1, 2, 3, 4 ])
-    })
+  describe('domContains()', () => {
+    describe('with native function', () => {
+      it('returns true if node is in the dom', () => {
+        expect(domContains(container)).toBe(true)
+      })
 
-    it('works with 3 arguments as input', () => {
-      expect(new Box().init(1, 2, 3, 4).toArray()).toEqual([ 1, 2, 3, 4 ])
+      it('returns false if node is not in the dom', () => {
+        const g = new G()
+        const rect = new Rect().addTo(g)
+        expect(domContains(rect.node)).toBe(false)
+      })
     })
 
-    it('works with string input', () => {
-      expect(new Box().init('1,2,3,4').toArray()).toEqual([ 1, 2, 3, 4 ])
-    })
+    describe('with polyfill', () => {
+      let containsBackup
+      beforeEach(() => {
+        containsBackup = getWindow().document.documentElement.contains
+        getWindow().document.documentElement.contains = null
+      })
 
-    it('creates a new box from parsed string with exponential values', function () {
-      expect(new Box().init('-1.12e1 1e-2 +2e2 +.3e+4').toArray())
-        .toEqual([ -11.2, 0.01, 200, 3000 ])
-    })
+      afterEach(() => {
+        getWindow().document.documentElement.contains = containsBackup
+      })
 
-    it('works with object input', () => {
-      expect(new Box().init({ x: 1, y: 2, width: 3, height: 4 }).toArray())
-        .toEqual([ 1, 2, 3, 4 ])
-    })
+      it('returns true if node is in the dom', () => {
+        expect(domContains(container)).toBe(true)
+      })
 
-    it('calculates all derived values correctly', () => {
-      expect(new Box().init(2, 4, 6, 8)).toEqual(objectContaining({
-        cx: 5, cy: 8, x2: 8, y2: 12, w: 6, h: 8
-      }))
+      it('returns false if node is not in the dom', () => {
+        const g = new G()
+        const rect = new Rect().addTo(g)
+        expect(domContains(rect.node)).toBe(false)
+      })
     })
+  })
 
-    it('can handle input with left instead of x and top instead of y', () => {
-      expect(new Box().init({ left: 1, top: 2, width: 3, height: 4 }).toArray())
-        .toEqual([ 1, 2, 3, 4 ])
+  describe('Box', () => {
+    describe('()', () => {
+      it('creates a new Box with default attributes', () => {
+        const box = new Box()
+        expect(box).toEqual(any(Box))
+        expect(box).toEqual(objectContaining({
+          width: 0, height: 0, x: 0, y: 0, w: 0, h: 0, cx: 0, cy: 0, x2: 0, y2: 0
+        }))
+      })
     })
-  })
 
-  describe('merge()', () => {
-    it('merges various bounding boxes', () => {
-      var box1 = new Box(50, 50, 100, 100)
-      var box2 = new Box(300, 400, 100, 100)
-      var box3 = new Box(500, 100, 100, 100)
-      var merged = box1.merge(box2).merge(box3)
+    describe('init()', () => {
+      it('inits or reinits the box according to input', () => {
+        expect(new Box().init(1, 2, 3, 4).toArray()).toEqual([ 1, 2, 3, 4 ])
+      })
 
-      expect(merged.toArray()).toEqual([ 50, 50, 550, 450 ])
-    })
+      it('works with array input', () => {
+        expect(new Box().init([ 1, 2, 3, 4 ]).toArray()).toEqual([ 1, 2, 3, 4 ])
+      })
+
+      it('works with 3 arguments as input', () => {
+        expect(new Box().init(1, 2, 3, 4).toArray()).toEqual([ 1, 2, 3, 4 ])
+      })
+
+      it('works with string input', () => {
+        expect(new Box().init('1,2,3,4').toArray()).toEqual([ 1, 2, 3, 4 ])
+      })
+
+      it('creates a new box from parsed string with exponential values', function () {
+        expect(new Box().init('-1.12e1 1e-2 +2e2 +.3e+4').toArray())
+          .toEqual([ -11.2, 0.01, 200, 3000 ])
+      })
 
-    it('returns a new instance', () => {
-      var box1 = new Box(50, 50, 100, 100)
-      var box2 = new Box(300, 400, 100, 100)
-      var merged = box1.merge(box2)
+      it('works with object input', () => {
+        expect(new Box().init({ x: 1, y: 2, width: 3, height: 4 }).toArray())
+          .toEqual([ 1, 2, 3, 4 ])
+      })
+
+      it('calculates all derived values correctly', () => {
+        expect(new Box().init(2, 4, 6, 8)).toEqual(objectContaining({
+          cx: 5, cy: 8, x2: 8, y2: 12, w: 6, h: 8
+        }))
+      })
 
-      expect(merged).toEqual(any(Box))
+      it('can handle input with left instead of x and top instead of y', () => {
+        expect(new Box().init({ left: 1, top: 2, width: 3, height: 4 }).toArray())
+          .toEqual([ 1, 2, 3, 4 ])
+      })
     })
-  })
 
-  describe('transform()', () => {
-    it('transforms the box with given matrix', () => {
-      var box1 = new Box(50, 50, 100, 100).transform(new Matrix(1, 0, 0, 1, 20, 20))
-      var box2 = new Box(50, 50, 100, 100).transform(new Matrix(2, 0, 0, 2, 0, 0))
-      var box3 = new Box(-200, -200, 100, 100).transform(new Matrix(1, 0, 0, 1, -20, -20))
+    describe('merge()', () => {
+      it('merges various bounding boxes', () => {
+        var box1 = new Box(50, 50, 100, 100)
+        var box2 = new Box(300, 400, 100, 100)
+        var box3 = new Box(500, 100, 100, 100)
+        var merged = box1.merge(box2).merge(box3)
+
+        expect(merged.toArray()).toEqual([ 50, 50, 550, 450 ])
+      })
+
+      it('returns a new instance', () => {
+        var box1 = new Box(50, 50, 100, 100)
+        var box2 = new Box(300, 400, 100, 100)
+        var merged = box1.merge(box2)
 
-      expect(box1.toArray()).toEqual([ 70, 70, 100, 100 ])
-      expect(box2.toArray()).toEqual([ 100, 100, 200, 200 ])
-      expect(box3.toArray()).toEqual([ -220, -220, 100, 100 ])
+        expect(merged).toEqual(any(Box))
+      })
     })
-  })
 
-  describe('addOffset()', () => {
-    it('returns a new instance', () => {
-      withWindow({ pageXOffset: 50, pageYOffset: 25 }, () => {
-        const box = new Box(100, 100, 100, 100)
-        const box2 = box.addOffset()
+    describe('transform()', () => {
+      it('transforms the box with given matrix', () => {
+        var box1 = new Box(50, 50, 100, 100).transform(new Matrix(1, 0, 0, 1, 20, 20))
+        var box2 = new Box(50, 50, 100, 100).transform(new Matrix(2, 0, 0, 2, 0, 0))
+        var box3 = new Box(-200, -200, 100, 100).transform(new Matrix(1, 0, 0, 1, -20, -20))
+
+        expect(box1.toArray()).toEqual([ 70, 70, 100, 100 ])
+        expect(box2.toArray()).toEqual([ 100, 100, 200, 200 ])
+        expect(box3.toArray()).toEqual([ -220, -220, 100, 100 ])
+      })
+
+      it('also works with matrix like input', () => {
+        var box1 = new Box(50, 50, 100, 100).transform(new Matrix(1, 0, 0, 1, 20, 20).toArray())
+        var box2 = new Box(50, 50, 100, 100).transform(new Matrix(2, 0, 0, 2, 0, 0).toArray())
+        var box3 = new Box(-200, -200, 100, 100).transform(new Matrix(1, 0, 0, 1, -20, -20).toArray())
 
-        expect(box2).toEqual(any(Box))
-        expect(box2).not.toBe(box)
+        expect(box1.toArray()).toEqual([ 70, 70, 100, 100 ])
+        expect(box2.toArray()).toEqual([ 100, 100, 200, 200 ])
+        expect(box3.toArray()).toEqual([ -220, -220, 100, 100 ])
       })
     })
 
-    it('adds the current page offset to the box', () => {
-      withWindow({ pageXOffset: 50, pageYOffset: 25 }, () => {
-        const box = new Box(100, 100, 100, 100).addOffset()
+    describe('addOffset()', () => {
+      it('returns a new instance', () => {
+        withWindow({ pageXOffset: 50, pageYOffset: 25 }, () => {
+          const box = new Box(100, 100, 100, 100)
+          const box2 = box.addOffset()
+
+          expect(box2).toEqual(any(Box))
+          expect(box2).not.toBe(box)
+        })
+      })
+
+      it('adds the current page offset to the box', () => {
+        withWindow({ pageXOffset: 50, pageYOffset: 25 }, () => {
+          const box = new Box(100, 100, 100, 100).addOffset()
 
-        expect(box.toArray()).toEqual([ 150, 125, 100, 100 ])
+          expect(box.toArray()).toEqual([ 150, 125, 100, 100 ])
+        })
       })
     })
-  })
 
-  describe('toString()', () => {
-    it('returns a string representation of the box', () => {
-      expect(new Box(1, 2, 3, 4).toString()).toBe('1 2 3 4')
+    describe('toString()', () => {
+      it('returns a string representation of the box', () => {
+        expect(new Box(1, 2, 3, 4).toString()).toBe('1 2 3 4')
+      })
     })
-  })
 
-  describe('toArray()', () => {
-    it('returns an array representation of the box', () => {
-      expect(new Box(1, 2, 3, 4).toArray()).toEqual([ 1, 2, 3, 4 ])
+    describe('toArray()', () => {
+      it('returns an array representation of the box', () => {
+        expect(new Box(1, 2, 3, 4).toArray()).toEqual([ 1, 2, 3, 4 ])
+      })
     })
-  })
 
-  describe('isNulled()', () => {
-    it('checks if the box consists of only zeros', () => {
-      expect(new Box().isNulled()).toBe(true)
-      expect(new Box(1, 2, 3, 4).isNulled()).toBe(false)
+    describe('isNulled()', () => {
+      it('checks if the box consists of only zeros', () => {
+        expect(new Box().isNulled()).toBe(true)
+        expect(new Box(1, 2, 3, 4).isNulled()).toBe(false)
+      })
     })
   })
 
@@ -153,10 +211,13 @@ describe('Box.js', () => {
         expect(rect.bbox().toArray()).toEqual([ 20, 30, 100, 200 ])
       })
 
-      // it('throws when it is not possible to get a bbox', () => {
-      //   const gradient = new Gradient('radial')
-      //   expect(() => gradient.bbox()).toThrow()
-      // })
+      it('throws when it is not possible to get a bbox', () => {
+        const spy = spyOn(getWindow().SVGGraphicsElement.prototype, 'getBBox')
+          .and.callFake(() => { throw new Error('No BBox for you') })
+        const rect = new Rect()
+        expect(() => rect.bbox()).toThrow()
+        expect(spy).toHaveBeenCalled()
+      })
     })
 
     describe('rbox()', () => {
@@ -169,6 +230,20 @@ describe('Box.js', () => {
         expect(rect.rbox().toArray()).toEqual([ 80, 110, 200, 400 ])
       })
 
+      it('returns the rbox box of the element in the coordinate system of the passed element', () => {
+        const canvas = SVG().addTo(container)
+        const group = canvas.group().translate(1, 1)
+        const rect = new Rect().size(100, 200).move(20, 30).addTo(canvas)
+          .attr('transform', new Matrix({ scale: 2, translate: [ 40, 50 ] }))
+
+        expect(rect.rbox(group)).toEqual(any(Box))
+        expect(rect.rbox(group).toArray()).toEqual([ 79, 109, 200, 400 ])
+      })
+
+      // svgdom actually only throws here because a new Rect without dimensions has no bounding box
+      // so in case you would create a rect with with and height this test would fail because
+      // svgdom actually can calculate an rbox for the element
+      // in that case we have to change the test like above so that the getBoundingClientRect call is mocked with a spy
       it('throws when element is not in dom', () => {
         expect(() => new Rect().rbox()).toThrow()
       })
@@ -185,31 +260,63 @@ describe('Box.js', () => {
 
     describe('viewbox()', () => {
       it('sets the viewbox of the element', () => {
-        const canvas = viewbox.call(SVG().addTo(container), 10, 10, 200, 200)
+        const canvas = SVG().addTo(container).viewbox(10, 10, 200, 200)
         expect(canvas.attr('viewBox')).toEqual('10 10 200 200')
       })
 
       it('gets the viewbox of the element', () => {
-        const canvas = viewbox.call(SVG().addTo(container), 10, 10, 200, 200)
-        expect(viewbox.call(canvas)).toEqual(any(Box))
-        expect(viewbox.call(canvas).toArray()).toEqual([ 10, 10, 200, 200 ])
+        const canvas = SVG().addTo(container).viewbox(10, 10, 200, 200)
+        expect(canvas.viewbox()).toEqual(any(Box))
+        expect(canvas.viewbox().toArray()).toEqual([ 10, 10, 200, 200 ])
       })
     })
 
     describe('zoom()', () => {
       it('zooms around the center by default', () => {
-        const canvas = zoom.call(SVG().size(100, 50).viewbox(0, 0, 100, 50).addTo(container), 2)
+        const canvas = SVG().size(100, 50).viewbox(0, 0, 100, 50).addTo(container).zoom(2)
         expect(canvas.attr('viewBox')).toEqual('25 12.5 50 25')
       })
 
       it('zooms around a point', () => {
-        const canvas = zoom.call(SVG().size(100, 50).viewbox(0, 0, 100, 50).addTo(container), 2, [ 0, 0 ])
+        const canvas = SVG().size(100, 50).viewbox(0, 0, 100, 50).addTo(container).zoom(2, [ 0, 0 ])
         expect(canvas.attr('viewBox')).toEqual('0 0 50 25')
       })
 
       it('gets the zoom', () => {
-        const canvas = zoom.call(SVG().size(100, 50).viewbox(0, 0, 100, 50).addTo(container), 2)
-        expect(zoom.call(canvas)).toEqual(2)
+        // We use a nested here because its actually harder to get a width and height for a nested svg because clientHeight
+        // is not available
+        const svg = SVG().size(100, 50).addTo(container).nested().size(100, 50).viewbox(0, 0, 100, 50).zoom(2)
+        expect(svg.zoom()).toEqual(2)
+      })
+
+      it('gets the zoom with clientHeight', () => {
+        const svg = SVG().css({ width: '100px', height: '50px' }).addTo(container).viewbox(25, 12.5, 50, 25)
+
+        const node = svg.node
+
+        // svgdom doesn't support clientHeight
+        // so we mock it here
+        if (typeof node.clientHeight === 'undefined') {
+          node.clientHeight = 50
+          node.clientWidth = 100
+        }
+
+        expect(svg.zoom()).toEqual(2)
+      })
+
+      it('throws an error if it is impossible to get an absolute value', () => {
+        const svg = SVG().size(100, 50).addTo(container).nested().viewbox(0, 0, 100, 50)
+        expect(() => svg.zoom()).toThrowError('Impossible to get absolute width and height. Please provide an absolute width and height attribute on the zooming element')
+      })
+
+      it('handles zoom level 0 which is - which basically sets the viewbox to a very high value', () => {
+        const svg = SVG().size(100, 50).viewbox(0, 0, 100, 50).addTo(container).zoom(0)
+        expect(svg.zoom()).toBeCloseTo(0, 10)
+      })
+
+      it('handles zoom level 0 and can recover from it', () => {
+        const svg = SVG().size(100, 50).viewbox(0, 0, 100, 50).addTo(container).zoom(0).zoom(1)
+        expect(svg.zoom()).toBe(1)
       })
     })
   })
diff --git a/src/modules/optional/data.js b/src/modules/optional/data.js
index 3a642c0..9986c3d 100644
--- a/src/modules/optional/data.js
+++ b/src/modules/optional/data.js
@@ -1,5 +1,4 @@
 import { registerMethods } from '../../utils/methods.js'
-import { isNumber } from '../core/regex.js'
 import { filter, map } from '../../utils/utils.js'
 
 // Store data values on svg nodes
diff --git a/src/types/Box.js b/src/types/Box.js
index ae0f2cd..3e91c35 100644
--- a/src/types/Box.js
+++ b/src/types/Box.js
@@ -6,11 +6,11 @@ import Matrix from './Matrix.js'
 import Point from './Point.js'
 import parser from '../modules/core/parser.js'
 
-function isNulledBox (box) {
+export function isNulledBox (box) {
   return !box.width && !box.height && !box.x && !box.y
 }
 
-function domContains (node) {
+export function domContains (node) {
   return node === globals.document
     || (globals.document.documentElement.contains || function (node) {
       // This is IE - it does not support contains() for top-level SVGs
@@ -111,41 +111,69 @@ export default class Box {
   }
 }
 
-function getBox (cb, retry) {
+function getBox (el, getBBoxFn, retry) {
   let box
 
   try {
-    box = cb(this.node)
+    // Try to get the box with the provided function
+    box = getBBoxFn(el.node)
 
-    if (isNulledBox(box) && !domContains(this.node)) {
+    // If the box is worthless and not even in the dom, retry
+    // by throwing an error here...
+    if (isNulledBox(box) && !domContains(el.node)) {
       throw new Error('Element not in the dom')
     }
   } catch (e) {
-    box = retry(this)
+    // ... and calling the retry handler here
+    box = retry(el)
   }
 
   return box
 }
 
 export function bbox () {
-  return new Box(getBox.call(this, (node) => node.getBBox(), (el) => {
+  // Function to get bbox is getBBox()
+  const getBBox = (node) => node.getBBox()
+
+  // Take all measures so that a stupid browser renders the element
+  // so we can get the bbox from it when we try again
+  const retry = (el) => {
     try {
       const clone = el.clone().addTo(parser().svg).show()
       const box = clone.node.getBBox()
       clone.remove()
       return box
     } catch (e) {
-      throw new Error('Getting bbox of element "' + el.node.nodeName + '" is not possible. ' + e.toString())
+      // We give up...
+      throw new Error(`Getting bbox of element "${el.node.nodeName}" is not possible: ${e.toString()}`)
     }
-  }))
+  }
+
+  const box = getBox(this, getBBox, retry)
+  const bbox = new Box(box)
+
+  return bbox
 }
 
 export function rbox (el) {
-  const box = new Box(getBox.call(this, (node) => node.getBoundingClientRect(), (el) => {
-    throw new Error('Getting rbox of element "' + el.node.nodeName + '" is not possible')
-  }))
-  if (el) return box.transform(el.screenCTM().inverse())
-  return box.addOffset()
+  const getRBox = (node) => node.getBoundingClientRect()
+  const retry = (el) => {
+    // There is no point in trying tricks here because if we insert the element into the dom ourselfes
+    // it obviously will be at the wrong position
+    throw new Error(`Getting rbox of element "${el.node.nodeName}" is not possible`)
+  }
+
+  const box = getBox(this, getRBox, retry)
+  const rbox = new Box(box)
+
+  // If an element was passed, we want the bbox in the coordinate system of that element
+  if (el) {
+    return rbox.transform(el.screenCTM().inverseO())
+  }
+
+  // Else we want it in absolute screen coordinates
+  // Therefore we need to add the scrollOffset
+  return rbox.addOffset()
 }
 
 // Checks whether the given point is inside the bounding box
@@ -169,18 +197,29 @@ registerMethods({
     },
 
     zoom (level, point) {
-      let width = this.node.clientWidth
-      let height = this.node.clientHeight
-      const v = this.viewbox()
+      // Its best to rely on the attributes here and here is why:
+      // clientXYZ: Doesn't work on non-root svgs because they dont have a CSSBox (silly!)
+      // getBoundingClinetRect: Doesn't work because Chrome just ignores width and height of nested svgs completely
+      //                        that means, their clientRect is always as big as the content.
+      //                        Furthermore this size is incorrect if the element is further transformed by its parents
+      // computedStyle: Only returns meaningful values if css was used with px. We dont go this route here!
+      // getBBox: returns the bounding box of its content - that doesnt help!
+      let { width, height } = this.attr([ 'width', 'height' ])
+
+      // Width and height is a string when a number with a unit is present which we can't use
+      // So we try clientXYZ
+      if ((!width && !height) || (typeof width === 'string' || typeof height === 'string')) {
+        width = this.node.clientWidth
+        height = this.node.clientHeight
+      }
 
-      // Firefox does not support clientHeight and returns 0
-      // https://bugzilla.mozilla.org/show_bug.cgi?id=874811
-      if (!width && !height) {
-        var style = globals.window.getComputedStyle(this.node)
-        width = parseFloat(style.getPropertyValue('width'))
-        height = parseFloat(style.getPropertyValue('height'))
+      // Giving up...
+      if (!width || !height) {
+        throw new Error('Impossible to get absolute width and height. Please provide an absolute width and height attribute on the zooming element')
       }
 
+      const v = this.viewbox()
+
       const zoomX = width / v.width
       const zoomY = height / v.height
       const zoom = Math.min(zoomX, zoomY)
@@ -190,7 +229,10 @@ registerMethods({
       }
 
       let zoomAmount = zoom / level
-      if (zoomAmount === Infinity) zoomAmount = Number.MIN_VALUE
+
+      // Set the zoomAmount to the highest value which is safe to process and recover from
+      // The * 100 is a bit of wiggle room for the matrix transformation
+      if (zoomAmount === Infinity) zoomAmount = Number.MAX_SAFE_INTEGER / 100
 
       point = point || new Point(width / 2 / zoomX + v.x, height / 2 / zoomY + v.y)