christophergandrud / networkD3

D3 JavaScript Network Graphs from R
http://christophergandrud.github.io/networkD3
650 stars 269 forks source link

<br> in Node tooltips should be \n instead to render correctly in Chrome and Edge #251

Open mac471 opened 5 years ago

mac471 commented 5 years ago

This is a new issue for an old problem: how text displays in tooltips. For "links", the two lines are separated by a newline character \n. For "nodes", the two lines are separated by a <br> tag.

While this works in Safari, it does not appear to work in Chrome or Edge. The <br> tag is just ignored and the two lines run together.

I believe the answer is simple. In the line below, just replace <br> with \n (like the way links are handled) to fix the problem. I've edited my local copy of sankeyNetwork.js and this now works on Safari, Chrome and Edge.

https://github.com/christophergandrud/networkD3/blob/9c0a9c9ff32c53212d2d43e0ae3cc664137315e6/inst/htmlwidgets/sankeyNetwork.js#L187

As an aside, I believe using <br> should work in Chrome and Edge. I've posted a question on StackOverflow to ask this question

Do you want me to make this change via pull request?

cjyetman commented 5 years ago

Ugh... this issue has caused way more hassle than anything else by far... all just to put a newline in tooltips. see #180 #152 #130 #146 #151 #175 #215 #250 just for starters.

The problem is, tooltips were not designed to be multiline, so browsers react differently to whatever hack you use to enable it. Then, SVG titles are not designed to contain HTML, so whatever hack we use to enable that causes various problems and conflicts. We've gone through multiple rounds of trying to get this working on the widest range of browsers, and spent an inordinate amount of time testing and researching options. Frankly, it pains me to imagine reviewing another change that might fix something for someone but break something for everyone else, particularly with this issue that has caused so much trouble before.

That being said, since the line for the links is... https://github.com/christophergandrud/networkD3/blob/9c0a9c9ff32c53212d2d43e0ae3cc664137315e6/inst/htmlwidgets/sankeyNetwork.js#L173-L174

and the line for the nodes is... https://github.com/christophergandrud/networkD3/blob/9c0a9c9ff32c53212d2d43e0ae3cc664137315e6/inst/htmlwidgets/sankeyNetwork.js#L187-L188

it seems logical that they should be the same. The main significant difference between the nodes and links is that one is a SVG rect and one is a SVG path, and I can't imagine why that would matter. There's another pending PR #215 that I will hopefully merge soon that may conflict with this change though... he was appending text to the pre object instead of html, hence the \n instead of a <br>.

cjyetman commented 5 years ago

and there's some report here https://github.com/christophergandrud/networkD3/pull/130#issuecomment-232888736 that suggests \n is not necessarily the most compatible option

mac471 commented 5 years ago

Seems like different browsers are rendering <br> differently so short of creating lots of browser-specific code, how about the following as a potential solution:

Add new arguments to the sankeyNetwork function that lets the caller specify the strings to use for delimiting between the node/link display name and the node/link value. For instance the argument nodeDelimiter defaults to "
" to maintain current compatibility and the argument linkDelimiter defaults to "\n". Those of us who don't really care about showing the tool tip on two lines can set these values to ": " so that the tool tip appears on a single line.

The code in sankeyNetwork.js would change to something like this:

For nodes: .html(function(d) { return "<pre>" + d.name + options.nodeDelimiter + format(d.value) + " " + options.units + "</pre>"; });

For links: .html(function(d) { return "<pre>" + d.source.name + " \u2192 " + d.target.name + options.linkDelimiter + format(d.value) + " " + options.units + "</pre>"; });

This way, you can avoid a rash of new bug reports from changing <br> to \n while giving developers the choice of how to handle the delimiter in both node and link tooltips. Probably the safest thing is to just show it all on the same line but in the spirit of not changing things that work the way people already want, set the defaults as suggested above.

cjyetman commented 5 years ago

My preference would be to make the default delimiter either ": " or " - " for full, safe compatibility. Then maybe add some functionality that would allow a user to set custom code for the tooltips, then they take full responsibility if they want to use <br> or "\n", and whatever the consequences are on their browser.

mac471 commented 5 years ago

So that's fine but I would use ": " instead of " - " to avoid confusion with the negative sign (even though negative values don't make sense in a Sankey).

If you go this route, then instead of adding the arguments I suggested above for backward compatibility, I would consider adding the ability to provide custom JavaScript functions to render the node and link tooltip text as desired - perhaps through the $options list?

cjyetman commented 5 years ago

also of note... using 0.4 release version and the 0.4.9000 dev version of networkD3 on macOS 10.14.3, R version 3.5.2, and RStudio version 1.2.1114... neither (links or nodes) of the line breaks in the tooltips work in the RStudio Viewer pane. Personally, I believe that the RStudio Viewer pane should be the primary target for compatibility because that's the most accessible output.

cjyetman commented 5 years ago

If you go this route, then instead of adding the arguments I suggested above for backward compatibility, I would consider adding the ability to provide custom JavaScript functions to render the node and link tooltip text as desired - perhaps through the $options list?

Yes, so for example, the defaults could be... link_tooltip = 'd.source.name + " \u2192 " + d.target.name + " - " + format(d.value) + " " + options.units' and node_tooltip = 'd.name + " - " + format(d.value) + " " + options.units'

mac471 commented 5 years ago

This is probably the best alternative as it gives the caller the ability to specify what the node/link tooltips should look like while making sure that it at least works in RStudio.

Sounds like a plan!