From ed3f68591f27a646e2ccb59b0d57dcb3b2e5036f Mon Sep 17 00:00:00 2001
From: Wilco Fiers <wilco@>
Date: Tue, 23 Feb 2016 15:10:42 +0100
Subject: [PATCH 1/4] get-background-color tests for graphic elements

---
 lib/commons/color/get-background-color.js  | 94 +++++++++++-----------
 test/commons/color/get-background-color.js | 43 +++++++++-
 2 files changed, 85 insertions(+), 52 deletions(-)

diff --git a/lib/commons/color/get-background-color.js b/lib/commons/color/get-background-color.js
index 7b20e807a9..40df484927 100644
--- a/lib/commons/color/get-background-color.js
+++ b/lib/commons/color/get-background-color.js
@@ -1,21 +1,27 @@
 /*global dom, color */
-/* jshint maxstatements: 31, maxcomplexity: 14 */
+/* jshint maxstatements: 35, maxcomplexity: 14 */
 //TODO dsturley: too complex, needs refactor!!
 
+var graphicNodeNames = [
+	'IMG', 'CANVAS', 'OBJECT', 'IFRAME', 'VIDEO'
+];
+
 /**
  * Returns the non-alpha-blended background color of a node, null if it's an image
  * @param {Element} node
  * @return {Color}
  */
 function getBackgroundForSingleNode(node) {
-	var bgColor,
-		nodeStyle = window.getComputedStyle(node);
+	var bgColor, bgColorString, opacity;
+	var nodeStyle = window.getComputedStyle(node);
+	var nodeName = node.nodeName.toUpperCase();
 
-	if (nodeStyle.getPropertyValue('background-image') !== 'none') {
+	if (graphicNodeNames.indexOf(nodeName) !== -1 ||
+	nodeStyle.getPropertyValue('background-image') !== 'none') {
 		return null;
 	}
 
-	var bgColorString = nodeStyle.getPropertyValue('background-color');
+	bgColorString = nodeStyle.getPropertyValue('background-color');
 	//Firefox exposes unspecified background as 'transparent' rather than rgba(0,0,0,0)
 	if (bgColorString === 'transparent') {
 		bgColor = new color.Color(0, 0, 0, 0);
@@ -23,7 +29,8 @@ function getBackgroundForSingleNode(node) {
 		bgColor = new color.Color();
 		bgColor.parseRgbString(bgColorString);
 	}
-	var opacity = nodeStyle.getPropertyValue('opacity');
+
+	opacity = nodeStyle.getPropertyValue('opacity');
 	bgColor.alpha = bgColor.alpha * opacity;
 
 	return bgColor;
@@ -51,39 +58,13 @@ dom.isOpaque = function(node) {
  * @return {Array} array of elements
  */
 var getVisualParents = function(node, rect) {
-	var visualParents,
-		thisIndex,
-		parents = [],
-		fallbackToVisual = false,
-		currentNode = node,
-		nodeStyle = window.getComputedStyle(currentNode),
-		posVal, topVal, bottomVal, leftVal, rightVal;
-
-	while (currentNode !== null && (!dom.isOpaque(currentNode) || parseInt(nodeStyle.getPropertyValue('height'), 10) === 0)) {
-		// If the element is positioned, we can't rely on DOM order to find visual parents
-		posVal = nodeStyle.getPropertyValue('position');
-		topVal = nodeStyle.getPropertyValue('top');
-		bottomVal = nodeStyle.getPropertyValue('bottom');
-		leftVal = nodeStyle.getPropertyValue('left');
-		rightVal = nodeStyle.getPropertyValue('right');
-		if ((posVal !== 'static' && posVal !== 'relative') ||
-			(posVal === 'relative' &&
-				(leftVal !== 'auto' ||
-					rightVal !== 'auto' ||
-					topVal !== 'auto' ||
-					bottomVal !== 'auto'))) {
-			fallbackToVisual = true;
-		}
-		currentNode = currentNode.parentElement;
-		if (currentNode !== null) {
-			nodeStyle = window.getComputedStyle(currentNode);
-			if (parseInt(nodeStyle.getPropertyValue('height'), 10) !== 0) {
-				parents.push(currentNode);
-			}
-		}
-	}
+	var visualParents, thisIndex, posVal;
+	var parents = [];
+	var fallbackToVisual = false;
+	var currentNode = node;
+	var nodeStyle = window.getComputedStyle(currentNode);
 
-	if (fallbackToVisual && dom.supportsElementsFromPoint(document)) {
+	if (dom.supportsElementsFromPoint(document)) {
 		visualParents = dom.elementsFromPoint(document,
 			Math.ceil(rect.left + 1),
 			Math.ceil(rect.top + 1));
@@ -92,10 +73,24 @@ var getVisualParents = function(node, rect) {
 		// if the element is not present; then something is obscuring it thus making calculation impossible
 		if (thisIndex === -1) {
 			return null;
+
+		} else if (visualParents && (thisIndex < visualParents.length - 1)) {
+			return visualParents.slice(thisIndex + 1);
 		}
+	}
 
-		if (visualParents && (thisIndex < visualParents.length - 1)) {
-			parents = visualParents.slice(thisIndex + 1);
+	while (currentNode !== null) {
+		// If the element is positioned, we can't rely on DOM order to find visual parents
+		posVal = nodeStyle.getPropertyValue('position');
+		if (posVal !== 'static') {
+			fallbackToVisual = true;
+		}
+		currentNode = currentNode.parentElement;
+		if (currentNode !== null) {
+			nodeStyle = window.getComputedStyle(currentNode);
+			if (parseInt(nodeStyle.getPropertyValue('height'), 10) !== 0) {
+				parents.push(currentNode);
+			}
 		}
 	}
 
@@ -113,23 +108,24 @@ var getVisualParents = function(node, rect) {
 //TODO dsturley; why is this passing `bgNodes`?
 color.getBackgroundColor = function(node, bgNodes) {
 	var parent, parentColor;
-
 	var bgColor = getBackgroundForSingleNode(node);
+
 	if (bgNodes && (bgColor === null || bgColor.alpha !== 0)) {
 		bgNodes.push(node);
 	}
-	if (bgColor === null || bgColor.alpha === 1) {
+	if (bgColor === null) {
 		return bgColor;
 	}
 
 	node.scrollIntoView();
-	var rect = node.getBoundingClientRect(),
-		currentNode = node,
-		colorStack = [{
+	var rect = node.getBoundingClientRect();
+	var currentNode = node;
+	var colorStack = [{
 			color: bgColor,
 			node: node
-		}],
-		parents = getVisualParents(currentNode, rect);
+		}];
+	var parents = getVisualParents(currentNode, rect);
+
 	if (!parents) {
 		return null;
 	}
@@ -139,11 +135,11 @@ color.getBackgroundColor = function(node, bgNodes) {
 
 		if (!parent && currentNode.tagName !== 'HTML') {
 			return null;
-		}
 
 		//Assume white if top level is not specified
-		if (!parent && currentNode.tagName === 'HTML') {
+		} else if (!parent && currentNode.tagName === 'HTML') {
 			parentColor = new color.Color(255, 255, 255, 1);
+
 		} else {
 
 			if (!dom.visuallyContains(node, parent)) {
diff --git a/test/commons/color/get-background-color.js b/test/commons/color/get-background-color.js
index caae28d521..d111f50392 100644
--- a/test/commons/color/get-background-color.js
+++ b/test/commons/color/get-background-color.js
@@ -217,15 +217,17 @@ describe('color.getBackgroundColor', function () {
 	});
 
 	it('should ignore 0-height elements', function () {
-		fixture.innerHTML = '<div id="parent" style="height: 40px; width: 30px; ' +
+		fixture.innerHTML =
+			'<div id="parent" style="height: 40px; width: 30px; ' +
 			'background-color: white; position: relative; z-index: 5">' +
-			'<div float="left" style="height: 0px; background-color: black">' +
-			'<div id="target" style="height: 20px; width: 25px; z-index: 25;">' +
+			'  <div float="left" style="height: 0px; background-color: black">' +
+			'    <div id="target" style="height: 20px; width: 25px; z-index: 25;">' +
 			'</div></div></div>';
 		var target = fixture.querySelector('#target');
 		var parent = fixture.querySelector('#parent');
 		var bgNodes = [];
 		var actual = commons.color.getBackgroundColor(target, bgNodes);
+
 		var expected = new commons.color.Color(255, 255, 255, 1);
 		assert.closeTo(actual.red, expected.red, 0.5);
 		assert.closeTo(actual.green, expected.green, 0.5);
@@ -259,6 +261,41 @@ describe('color.getBackgroundColor', function () {
 		assert.closeTo(actual.alpha, expected.alpha, 0.1);
 	});
 
+	it('should return null when encountering background images during visual traversal', function () {
+		fixture.innerHTML =
+		'<div id="parent" style="height: 40px; width: 30px; ' +
+		' background-color: white; position: relative; z-index: 5"> ' +
+		'	<div id="target" style="position: relative; top: 1px; height: 20px;' +
+		'	 width: 25px; z-index: 25; background:rgba(0,125,0,0.5);"></div> ' +
+		'	<div id="shifted" style="position: absolute; top: -30px; height: 40px; ' +
+		'    background-image: url(foobar.png);'+
+		'	 width: 35px; z-index: 15;">' +
+		'	</div>'+
+		'</div>';
+
+		var target = fixture.querySelector('#target');
+		var bgNodes = [];
+		var outcome = commons.color.getBackgroundColor(target, bgNodes);
+		assert.isNull(outcome);
+	});
+
+	it('should return null when encountering image nodes during visual traversal', function () {
+		fixture.innerHTML =
+		'<div id="parent" style="height: 40px; width: 30px; ' +
+		' background-color: white; position: relative; z-index: 5"> ' +
+		'	<div id="shifted" style="position: absolute; top: -10px; height: 40px; ' +
+		'	 width: 35px; z-index: 15;">' +
+		'		<img src="some-img.png" width="35" height="40">' +
+		'	</div>'+
+		'	<div id="target" style="position: relative; top: 1px; height: 20px;' +
+		'	 width: 25px; z-index: 25; background:rgba(0,125,0,0.5);"></div> ' +
+		'</div>';
+
+		var target = fixture.querySelector('#target');
+		var bgNodes = [];
+		var outcome = commons.color.getBackgroundColor(target, bgNodes);
+		assert.isNull(outcome);
+	});
 
 
 });

From e1b55d56c16242944eb8e3565e6c65973f04dba5 Mon Sep 17 00:00:00 2001
From: Wilco Fiers <wilco@>
Date: Tue, 23 Feb 2016 15:31:17 +0100
Subject: [PATCH 2/4] get-background-color considers SVG

---
 lib/commons/color/get-background-color.js | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/commons/color/get-background-color.js b/lib/commons/color/get-background-color.js
index 40df484927..6460bc32ca 100644
--- a/lib/commons/color/get-background-color.js
+++ b/lib/commons/color/get-background-color.js
@@ -3,7 +3,7 @@
 //TODO dsturley: too complex, needs refactor!!
 
 var graphicNodeNames = [
-	'IMG', 'CANVAS', 'OBJECT', 'IFRAME', 'VIDEO'
+	'IMG', 'CANVAS', 'OBJECT', 'IFRAME', 'VIDEO', 'SVG'
 ];
 
 /**

From 8edd8df346f086cd366de3f57526376f99f3c410 Mon Sep 17 00:00:00 2001
From: Wilco Fiers <wilco@>
Date: Wed, 24 Feb 2016 11:19:51 +0100
Subject: [PATCH 3/4] Solved feedback from Dylan

---
 lib/commons/color/get-background-color.js | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/lib/commons/color/get-background-color.js b/lib/commons/color/get-background-color.js
index 6460bc32ca..d1ae96fa4b 100644
--- a/lib/commons/color/get-background-color.js
+++ b/lib/commons/color/get-background-color.js
@@ -17,7 +17,7 @@ function getBackgroundForSingleNode(node) {
 	var nodeName = node.nodeName.toUpperCase();
 
 	if (graphicNodeNames.indexOf(nodeName) !== -1 ||
-	nodeStyle.getPropertyValue('background-image') !== 'none') {
+			nodeStyle.getPropertyValue('background-image') !== 'none') {
 		return null;
 	}
 
@@ -60,7 +60,6 @@ dom.isOpaque = function(node) {
 var getVisualParents = function(node, rect) {
 	var visualParents, thisIndex, posVal;
 	var parents = [];
-	var fallbackToVisual = false;
 	var currentNode = node;
 	var nodeStyle = window.getComputedStyle(currentNode);
 
@@ -83,7 +82,7 @@ var getVisualParents = function(node, rect) {
 		// If the element is positioned, we can't rely on DOM order to find visual parents
 		posVal = nodeStyle.getPropertyValue('position');
 		if (posVal !== 'static') {
-			fallbackToVisual = true;
+			return null;
 		}
 		currentNode = currentNode.parentElement;
 		if (currentNode !== null) {
@@ -113,7 +112,7 @@ color.getBackgroundColor = function(node, bgNodes) {
 	if (bgNodes && (bgColor === null || bgColor.alpha !== 0)) {
 		bgNodes.push(node);
 	}
-	if (bgColor === null) {
+	if (bgColor === null || bgColor.alpha === 1) {
 		return bgColor;
 	}
 

From 535d2658dceb50fabb5038d48bdb5d76eadfc6be Mon Sep 17 00:00:00 2001
From: Wilco Fiers <wilco@>
Date: Thu, 3 Mar 2016 15:34:57 +0100
Subject: [PATCH 4/4] Fix noscroll flip bug

---
 lib/commons/color/get-background-color.js | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/commons/color/get-background-color.js b/lib/commons/color/get-background-color.js
index 1b0182144f..c5ee6cce46 100644
--- a/lib/commons/color/get-background-color.js
+++ b/lib/commons/color/get-background-color.js
@@ -117,7 +117,7 @@ color.getBackgroundColor = function(node, bgNodes, noScroll) {
 		return bgColor;
 	}
 
-	if(noScroll === true) {
+	if(noScroll !== true) {
 		node.scrollIntoView();
 	}