No innerHTML and pseudo-html cleanup #1792

alexcjohnson · 2017-06-16T05:09:17Z

Started out as a bugfix but kind of turned into a spring cleaning...

In an effort to fix bounding box calculations in IE (one part of #1782) - which were broken by trying to use the innerHTML property of an SVG element - I wound up going through every user of Drawing.bBox and trying to make sure it was a <text> or .*-math-group node that had previously had svgTextUtils.convertToTspans called on it. Because if so, I could use the data-unformatted attribute, along with whether or not the node was converted to MathJax, in place of the innerHTML in the cache key, which is also much smaller than the innerHTML, reducing my memory concerns about this cache.

But then while doing THAT it became clear that all the monkey business using tspan.line elements to count lines and to properly align multi-line text was a disaster, and not done properly in a lot of places, so I centralized all that in two new utilities:

svgTextUtils.positionText - to be used instead of Drawing.setPosition, handles multiline alignment, and also gets called within convertToTspans so if you use transform instead of the x and y attributes to position the text, you never even need to use this at all.
svgTextUtils.lineCount - nuff said. No more hacky and/or broken selectors for counting lines of text.

So after that, almost nobody outside of svgTextUtils even needs to know about <tspan> or the .line class.

Along the way I found a few places we were not supporting or had bugs supporting pseudo-HTML but easily could, and added it: rangeselectors, updatemenus, sliders, carpet axes (titles and tick labels). And in cases that currently break with MathJax I explicitly cancel it (with the data-notex attribute). (MathJax test mocks coming in a future PR)

And while I was working in test_syntax.js I took care of #1773 as well.

There are still a few Drawing.bBox users that call it on elements larger than a <text> or MathJax group node, that at this point in this branch we will NOT cache at all. For 🐎 I need to either convert them to their base nodes or find an alternate way to give them a robust cache key:

colorbar/draw:473 & 490
titles/index:146 & 171

not a huge deal, but can improve caching

so we don't depend on callers getting this right

because now text nodes (that have been through convertToTspans) are cacheable but larger nodes are generally not.

fallback avoids errors with mathjax in the legend, but it still doesn't always get the sizing right because we're not handling async correctly.

that way nobody needs to know about 'tspan.line' outside of svg_text_utils

hidden elements still contribute to bounding boxes. mostly we are careful to calculate the bbox of the visible one but not in all cases.

etpinard

Looking great so far 👍

As of the latest commit, a few image tests are failing as well as one updatemenu jasmine test and there's one linting mistake.

Thanks for making svg_text_utils.js robust and reusable. Not having to think about tspan.line should speed up development for new components.

etpinard · 2017-06-16T14:35:35Z

src/lib/svg_text_utils.js


    function showText() {
        if(!parent.empty()) {
            svgClass = _context.attr('class') + '-math';
            parent.select('svg.' + svgClass).remove();
        }
        _context.text('')
-            .style({
-                visibility: 'inherit',


Interesting. Why did this line get 🔪ed ?

Ah, I didn't notice the original issue #984 that led to this change... assumed it was a remnant of a much older time. My goal was to remove inline styles whenever possible, rather than setting an explicit reset value, but anyway I moved from visibility:hidden to display:none because only the latter takes the item out of the bounding box, and there are still a couple of places (most notably titles/index:146 & 171 that I'm still working on) we get the bounding box of the combined text+MathJax element. I'll see if I can come up with a test of #984 to include with this PR.

etpinard · 2017-06-16T14:42:11Z

src/components/updatemenus/draw.js


        // width is given by max width of all buttons
        var tWidth = text.node() && Drawing.bBox(text.node()).width,
            wEff = Math.max(tWidth + constants.textPadX, constants.minWidth);

        // height is determined by item text
        var tHeight = menuOpts.font.size * constants.fontSizeToHeight,
-            tLines = tspans[0].length || 1,
+            tLines = tspans.size() || 1,


Oops. My bad for putting these in.

etpinard · 2017-06-16T14:47:27Z

src/components/legend/draw.js

-            textLines = textSpans[0].length || 1;
+            textSpans = text.selectAll('tspan.line'),
+            textLines = textSpans.size() || 1,
+            textNode = text.node();


Is there always only one <.legendtext> node per g? If so, we could replace the

var text = g.selectAll('.legendtext')

line with

var text = g.select('.legendtext')

sure, that's a bit cleaner -> 27b4be3

etpinard · 2017-06-16T14:55:41Z

src/components/drawing/index.js

-    }
+    // standardize its position (and newline tspans if any)
+    d3.select(testNode)
+        .attr('transform', '')


Interesting. Wouldn't .attr('transform', null) be d3-esque?

yes, that's better -> 82ba7ee

etpinard · 2017-06-16T14:56:11Z

src/components/rangeselector/draw.js

@@ -149,8 +149,6 @@ function getFillColor(selectorLayout, d) {
 function drawButtonText(button, selectorLayout, d, gd) {
    function textLayout(s) {
        svgTextUtils.convertToTspans(s, gd);
-
-        // TODO do we need anything else here?


... I guess not. 😆

etpinard · 2017-06-16T14:59:47Z

src/components/rangeselector/draw.js

-            tHeight = opts.font.size * 1.3,
-            tLines = svgTextUtils.lineCount(text);
+        var tWidth = text.node() && Drawing.bBox(text.node()).width;
+        var tHeight = opts.font.size * 1.3;


that 1.3 factor should be put in a constant file.

Maybe in

https://github.com/plotly/plotly.js/blob/master/src/constants/alignment.js

?

or maybe inside a new svg_text_utils.js method?

Good call. It's used in a few different ways, so I went for alignment.js -> c5fa44b

etpinard · 2017-06-16T15:09:00Z

src/snapshot/tosvg.js

+            // but in a static plot it's useless and it can confuse batik
+            // we've tried to standardize on display:none but make sure we still
+            // catch visibility:hidden if it ever arises
+            if(txt.style('visibility') === 'hidden' || txt.style('display') === 'none') {


Referencing #990

etpinard · 2017-06-16T15:10:09Z

src/traces/carpet/plot.js

@@ -214,6 +221,7 @@ function drawAxisTitle(layer, trace, t, xy, dxy, axis, xa, ya, offset, labelClas
        var el = d3.select(this);

        el.text(axis.title || '')
+            .call(svgTextUtils.convertToTspans, gd)


Amazing, Pinging @rreusser

etpinard · 2017-06-16T15:15:30Z

tasks/test_syntax.js

                        logs.push(file + ' : contains .classList (IE failure)');
                    }
+                    else if(lastPart === 'innerHTML') {
+                        logs.push(file + ' : contains .innerHTML (IE failure in SVG)');


There might be exceptions to this rule in the future, if we end up using innerHTML on non-SVG DOM elements. Oh well, we could always filter the cases out then. We might we want to add a comment here signalling this check is too strict.

good thought. Comment added -> a68f861

etpinard · 2017-06-16T15:16:12Z

test/jasmine/tests/snapshot_test.js

            d3.select(gd).style('visibility', 'inherit');

            Plotly.plot(gd, subplotMock.data, subplotMock.layout).then(function() {

                d3.select(gd).selectAll('text').each(function() {
                    expect(d3.select(this).style('visibility')).toEqual('visible');
+                    expect(d3.select(this).style('display')).toEqual('block');


Nice touch. Thanks!

otherwise each tspan is relative to the previous, so the 3rd line `dy=2.6em` goes 2 lines below the 2nd. Alternatively I guess we could put `dy=1.3em` on all of them except the first but this way seems more explicit and robust

and stopped autosizing the mock, which makes it look different in the test dashboard vs the image server

and this is fine because we're calculating them in a totally different way in order to allow for multiline labels

multiline labels are calculating their widths correctly now because the newline tspans are lining up rather than stair stepping

etpinard · 2017-06-16T20:26:30Z

💃 after that linting mistake is fixed.

For 🐎 I need to either convert them to their base nodes or find an alternate way to give them a robust cache key:

I suppose this will wait for a future PR?

alexcjohnson · 2017-06-16T20:33:36Z

For 🐎 I need to either convert them to their base nodes or find an alternate way to give them a robust cache key:

I suppose this will wait for a future PR?

I'm going to take a stab at making the ones in titles cacheable for this PR, as I think that's actually a big part of the speedup in #1772 that we're losing at the moment. The ones in colorbar though I'll probably leave for later.

this must have been broken since day 1, oddly enough...

etpinard · 2017-06-19T14:10:11Z

src/components/updatemenus/draw.js

@@ -214,7 +214,7 @@ function drawHeader(gd, gHeader, gButton, scrollBox, menuOpts) {
        .classed('user-select-none', true)
        .attr('text-anchor', 'end')
        .call(Drawing.font, menuOpts.font)
-        .text('▼');
+        .text(constants.arrowSymbol[menuOpts.direction]);


etpinard · 2017-06-19T14:13:14Z

tasks/test_syntax.js

@@ -77,6 +77,9 @@ function assertSrcContents() {
                    else if(lastPart === 'innerHTML') {
                        logs.push(file + ' : contains .innerHTML (IE failure in SVG)');
                    }
+                    else if(lastPart === 'parentElement') {
+                        logs.push(file + ' : contains .parentElement (IE failure)');


For reference purposes: https://stackoverflow.com/questions/8685739/difference-between-dom-parentnode-and-parentelement

etpinard · 2017-06-19T14:14:58Z

src/constants/alignment.js

-    }
+    },
+    // multiple of fontSize to get the vertical offset between lines
+    LINE_SPACING: 1.3


🎉 🎉 🎉 🎉 🎉

etpinard · 2017-06-19T14:29:33Z

src/plots/gl2d/camera.js

@@ -131,7 +131,7 @@ function createCamera(scene) {

                        if(Math.abs(dx * dydx) > Math.abs(dy)) {
                            result.boxEnd[1] = result.boxStart[1] +
-                                Math.abs(dx) * dydx * (Math.sign(dy) || 1);
+                                Math.abs(dx) * dydx * (dy >= 0 ? 1 : -1);


[Non-blocking comment] It might be worth making a Lib.sign method at some point.

etpinard · 2017-06-19T14:32:39Z

test/jasmine/tests/page_test.js

+
+        // then merge in a few more with other component types
+        mock.layout.updatemenus = require('@mocks/updatemenus.json').layout.updatemenus;
+        mock.layout.sliders = require('@mocks/sliders.json').layout.sliders;


Great idea here putting all traces and components in one data/layout. Maybe we should do the same in the svg/pdf/eps image export tests

etpinard · 2017-06-19T14:34:09Z

Amazing PR. 💃

Text placement was always a tricky part of adding new layout components. This PR will make that so much easier. 🎉

alexcjohnson · 2017-06-19T14:46:22Z

Text placement was always a tricky part of adding new layout components. This PR will make that so much easier. 🎉

I hope so! One piece that's still pretty tricky is mathjax (because you have to support async rendering, and any bbox calls need to look for the mathjax group), and I'll try to address that later. In the meantime, we should make a practice of including data-notex (as I did with existing components in this PR) anytime we call convertToTspans if it hasn't been explicitly tested with tex. That way not only will it work consistently, but we'll have an easy tag to look for when someone wants to upgrade a component with tex compatibility.

alexcjohnson added 17 commits June 15, 2017 00:27

standardize on attr over style for text-anchor

3ad11f6

not a huge deal, but can improve caching

centralize data-unformatted and use it for bBox cache key

9949092

let Drawing.bBox handle tspan.line position reset

951a75e

so we don't depend on callers getting this right

fix pseudo-html rendering in rangeselectors and updatemenus

639878e

call bBox on text node when possible

16d7515

because now text nodes (that have been through convertToTspans) are cacheable but larger nodes are generally not.

clean up legend sizing and fallback on missing node

0b2a7b2

fallback avoids errors with mathjax in the legend, but it still doesn't always get the sizing right because we're not handling async correctly.

minor lint to bar text

a5a578f

cleaner data-math usage

5d3648c

fix sliders with pseudo-HTML line breaks

d29159a

make positionText and lineCount fns

6eedaa5

that way nobody needs to know about 'tspan.line' outside of svg_text_utils

linting related to tspan.line centralization

76fc8b8

use display:none instead of visibility:hidden

61dd3b1

hidden elements still contribute to bounding boxes. mostly we are careful to calculate the bbox of the visible one but not in all cases.

remove Drawing.measureText and support pseudo-HTML in carpets

936936b

prohibit Math.sign and .innerHTML in syntax test

4ae49f5

remove Math.sign

9f4ec66

fix title alignment

1a5679e

update snapshot test

af5b7f2

etpinard added status: reviewable bug something broken labels Jun 16, 2017

etpinard reviewed Jun 16, 2017

View reviewed changes

alexcjohnson added 7 commits June 16, 2017 13:56

oops, we need y=0 on tspan.line after all

4886985

otherwise each tspan is relative to the previous, so the 3rd line `dy=2.6em` goes 2 lines below the 2nd. Alternatively I guess we could put `dy=1.3em` on all of them except the first but this way seems more explicit and robust

hover labels need explicit position reset for proper positioning later

717c7d8

get correct bbox for carpet tick labels

7c3dc3b

fixed range selector mocks

378c805

and stopped autosizing the mock, which makes it look different in the test dashboard vs the image server

update binding mock - new label positions are slightly different

f8101b0

and this is fine because we're calculating them in a totally different way in order to allow for multiline labels

update updatemenus test

3b293ac

multiline labels are calculating their widths correctly now because the newline tspans are lining up rather than stair stepping

lint axes.js

c908ed4

etpinard mentioned this pull request Jun 16, 2017

Drawing.bBox measures multi-line text incorrectly #973

Closed

etpinard added this to the 1.28.0 milestone Jun 16, 2017

alexcjohnson added 13 commits June 16, 2017 17:58

reorder sliders findDimensions so dummy calculations always succeed

0bb38e9

test #984 - ensure we never override visibility

6c72c77

remove some obsolete code and comments from sliders/draw

30b9fa5

updatemenus point in the direction they'll open

930e58e

update updatemenus_positioning mock for new arrows

301fba7

fix #1782 (second half: arrowheads broken) - 🔪 parentElement

4b87b2e

this must have been broken since day 1, oddly enough...

note about innerHTML prohibition

a68f861

pull 1.3 out into alignment constants

c5fa44b

null transform, rather than empty string

82ba7ee

selectAll -> select because it can only be one node

27b4be3

lint titles var statements

46d6e44

let drawing.bBox cache simple nested elements

8305c02

don't recalculate nodehash for recursive bBox

8e3d4fa

alexcjohnson changed the title ~~[WIP] No innerHTML and pseudo-html cleanup~~ No innerHTML and pseudo-html cleanup Jun 19, 2017

etpinard reviewed Jun 19, 2017

View reviewed changes

alexcjohnson merged commit 3e83252 into master Jun 19, 2017

alexcjohnson deleted the no-innerHTML branch June 19, 2017 14:46

alexcjohnson mentioned this pull request Jun 19, 2017

Remove Math.sign #1773

Closed

This was referenced Jun 26, 2017

HTML entities no longer render in ticktext #1814

Closed

Fixup html entity number #1820

Merged

JMante1 mentioned this pull request Apr 17, 2019

sankey: node labels do not render HTML tags #3742

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

No innerHTML and pseudo-html cleanup #1792

No innerHTML and pseudo-html cleanup #1792

alexcjohnson commented Jun 16, 2017 •

edited

Loading

etpinard left a comment

etpinard Jun 16, 2017

alexcjohnson Jun 16, 2017

etpinard Jun 16, 2017

etpinard Jun 16, 2017

alexcjohnson Jun 16, 2017

etpinard Jun 16, 2017

alexcjohnson Jun 16, 2017

etpinard Jun 16, 2017

etpinard Jun 16, 2017

etpinard Jun 16, 2017

alexcjohnson Jun 16, 2017

etpinard Jun 16, 2017

etpinard Jun 16, 2017

etpinard Jun 16, 2017

alexcjohnson Jun 16, 2017

etpinard Jun 19, 2017

etpinard Jun 16, 2017

etpinard commented Jun 16, 2017

alexcjohnson commented Jun 16, 2017

etpinard Jun 19, 2017

etpinard Jun 19, 2017

etpinard Jun 19, 2017

etpinard Jun 19, 2017

etpinard Jun 19, 2017

etpinard commented Jun 19, 2017

alexcjohnson commented Jun 19, 2017

No innerHTML and pseudo-html cleanup #1792

No innerHTML and pseudo-html cleanup #1792

Conversation

alexcjohnson commented Jun 16, 2017 • edited Loading

etpinard left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard commented Jun 16, 2017

alexcjohnson commented Jun 16, 2017

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

etpinard commented Jun 19, 2017

alexcjohnson commented Jun 19, 2017

alexcjohnson commented Jun 16, 2017 •

edited

Loading