From 6decf222a3be685dcb50a468463fb44ebe05ccb7 Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Sun, 1 Nov 2015 19:23:08 +0100 Subject: [PATCH] :bug: Fix incorrectly reported width when measuring lines This commit fixes what seems to be a bug in Chromium. When measuring lines (and with a special character sequence), it could happen that Range(0, 0). getBoundingClientRect().width reports a number greater than 0. This seems to happen when the font size is smaller than 12px and it's probably due to subpixel font scaling. To solve it we've explicitly included a guard clause that prevents this problem to happen. --- spec/lines-yardstick-spec.coffee | 35 ++++++++++++++++++++++++++++---- src/lines-yardstick.coffee | 9 +++++--- 2 files changed, 37 insertions(+), 7 deletions(-) diff --git a/spec/lines-yardstick-spec.coffee b/spec/lines-yardstick-spec.coffee index 022f535f4..ae85a0e9d 100644 --- a/spec/lines-yardstick-spec.coffee +++ b/spec/lines-yardstick-spec.coffee @@ -2,7 +2,7 @@ LinesYardstick = require "../src/lines-yardstick" {toArray} = require 'underscore-plus' describe "LinesYardstick", -> - [editor, mockPresenter, mockLineNodesProvider, createdLineNodes, linesYardstick] = [] + [editor, mockPresenter, mockLineNodesProvider, createdLineNodes, linesYardstick, buildLineNode] = [] beforeEach -> waitsForPromise -> @@ -49,10 +49,10 @@ describe "LinesYardstick", -> buildLineNode(screenRow) textNodesForLineIdAndScreenRow: (lineId, screenRow) -> lineNode = @lineNodeForLineIdAndScreenRow(lineId, screenRow) + iterator = document.createNodeIterator(lineNode, NodeFilter.SHOW_TEXT) textNodes = [] - for span in lineNode.children - for textNode in span.childNodes - textNodes.push(textNode) + while textNode = iterator.nextNode() + textNodes.push(textNode) textNodes editor.setLineHeightInPixels(14) @@ -126,6 +126,33 @@ describe "LinesYardstick", -> expect(linesYardstick.pixelPositionForScreenPosition([0, 9]).left).toBe 67 expect(linesYardstick.pixelPositionForScreenPosition([0, 11]).left).toBe 84 + it "doesn't report a width greater than 0 when the character to measure is at the beginning of a text node", -> + # This spec documents what seems to be a bug in Chromium, because we'd + # expect that Range(0, 0).getBoundingClientRect().width to always be zero. + atom.styles.addStyleSheet """ + * { + font-size: 11px; + font-family: monospace; + } + """ + + text = " \\vec{w}_j^r(\\text{new}) &= \\vec{w}_j^r(\\text{old}) + \\Delta\\vec{w}_j^r, \\\\" + buildLineNode = (screenRow) -> + lineNode = document.createElement("div") + lineNode.style.whiteSpace = "pre" + # We couldn't reproduce the problem with a simple string, so we're + # attaching the full one that comes from a bug report. + lineNode.innerHTML = ' \\vec{w}_j^r(\\text{new}) &= \\vec{w}_j^r(\\text{old}) + \\Delta\\vec{w}_j^r, \\\\' + jasmine.attachToDOM(lineNode) + createdLineNodes.push(lineNode) + lineNode + + editor.setText(text) + + expect(linesYardstick.pixelPositionForScreenPosition([0, 35]).left).toBe 230.90625 + expect(linesYardstick.pixelPositionForScreenPosition([0, 36]).left).toBe 237.5 + expect(linesYardstick.pixelPositionForScreenPosition([0, 37]).left).toBe 244.09375 + it "doesn't measure invisible lines if it is explicitly told so", -> atom.styles.addStyleSheet """ * { diff --git a/src/lines-yardstick.coffee b/src/lines-yardstick.coffee index fa00bae40..54ba6cf57 100644 --- a/src/lines-yardstick.coffee +++ b/src/lines-yardstick.coffee @@ -159,9 +159,12 @@ class LinesYardstick 0 leftPixelPositionForCharInTextNode: (lineNode, textNode, charIndex) -> - @rangeForMeasurement.setStart(textNode, 0) - @rangeForMeasurement.setEnd(textNode, charIndex) - width = @rangeForMeasurement.getBoundingClientRect().width + if charIndex is 0 + width = 0 + else + @rangeForMeasurement.setStart(textNode, 0) + @rangeForMeasurement.setEnd(textNode, charIndex) + width = @rangeForMeasurement.getBoundingClientRect().width @rangeForMeasurement.setStart(textNode, 0) @rangeForMeasurement.setEnd(textNode, textNode.textContent.length)