Increase line endpoint offset to .01 For Windows/Firefox SVG gradient rendering #1022
Conversation
|
Thanks for this fix Clint -- I've tested it and it works for me (but then, so did the last implementation!). Am i understanding correctly that we now create new defs for every single node, but only apply them (via Presuming that this does fix it on different browsers (which is really important right now), would you mind squashing the 2 commits & fixing eslint errors. I hope we can revisit this in a future PR and restore the old implementation but with more subtle fixes. @gj262, @heliosfa & @cpsandbox would you mind checking a deploy of this PR at https://auspice-master-e4vfffeopmcatkn.herokuapp.com/ to see if this fixes it for you?
Yes, please do open a separate issue! Hoping we can identify a bunch of these unnecessary renders / recomputations and get rid of them ASAP. |
There is one defs section in the svg that holds all the gradient definitions. This is taken out and totally replaced on a change to colorBy or a layout change, not one defs per node. This is simply a guess that this will get Firefox to re-render all the new gradients. I mostly needed the heroku deploy so I can use a remote Windows system to test changes. |
|
If it's helpful and possible, if we could make a deploy of this I can test in native Windows 10 environment! Or a local branch I can pull and try. (But deploy is easier for me, if it's possible!) |
|
Hi @jameshadfield it still appears broken to me with Firefox 74 + Windows 10 in a VM (note: the resolution is not perfect in a saucelabs VM but I still think the graph should resolve better then this). Do you have a running instance where Firefox 74 + Windows 10 was working so I can see what it should roughly look like?
|
|
I have just tried the instance on herokuapp linked by @jameshadfield and a local build of @cpsandbox's master, and #1005 is still present for me as well in Firefox and Waterfox.
|
|
Thanks for testing @heliosfa & @gj262. I hope to have a bit of time to tackle this tomorrow. I don't have screenshots of what windows10+firefox74 should look like, but the images on #1005 indicate that there are large sections of the tree missing. @emmahodcroft yes there's a deployment: https://auspice-master-e4vfffeopmcatkn.herokuapp.com/ |
|
No problem @jameshadfield - I'm happy to test any other potential fixes and I'll try to keep an eye on the issue and this PR. I would offer to help fix it, but I am most definitely not a web developer! As for what it should look like, from having looked at it in a few browsers over the last few weeks, it should be pretty similar to Firefox on *nix or Chrome. |
|
AWS has free desktops starting April 1 which will help me with development/debugging. LMK if you want a fast reversion to non-gradient branches until then. |
|
Thanks! Sorry I missed that. I can't work on this directly at the moment, but am happy to test new trials on a native Windows 10 + Firefox set up - though sounds like you guys might have this under control! Just ping me if I can help. @jameshadfield Do we have any idea of how many viewers this might be affecting? What are your thoughts on rolling back until we can support properly? I don't know how long that might take - if we can maybe get this fixed quickly after 1 April (tomorrow) then might not be worth a roll-back - but if it might take a few weeks, we might want to consider it. I'm a bit torn. I love the gradients, but it does look terrible with missing branches.... |
cpsandbox
changed the title
|
After finally getting a windows desktop it became apparent that changing the line offset at .01 instead of .001 would render the gradient. I cannot determine if that is sufficient for all off the lines. I don't know if the calculation is, for instance based of the angle or just the absolute offset. The other prospective changes were reverted. Only the increase in line offset is changed. Waiting for re-deploy for further checks. |
|
On first bash with a local build, this looks promising in Firefox and Waterfox: Zooming in to one of the problem areas that I used as an example in #1005, things are looking much better as well: |
|
Brilliant! Thanks @cpsandbox @emmahodcroft. The solution (0.001 -> 0.01px) is tragicomic but maybe that's browser support in general. I'll test and, assuming there are no regressions, release today. |
|
Is this still not working? |
|
@cpsandbox I had to (temporarily 🤞 ) revert to the old behavior as we had a lot of issues in narratives especially. This was #1042. In order not to waste anyone's time, i'm going to make a branch which includes the partially-working implementation and then detail a number of reproducible examples. |
Description of proposed changes
First guess at fixing missing gradients on Windows 10/Firefox. I noticed a similar issue when developing the initial gradient coloring fix. This seems to be related to how updates to the DOM are handled by change.js. (React?)
The current code updates existing gradients by changing their stop values, adds new gradients if needed and removes gradients that are not needed.
This code clears all gradient definitions and creates new where needed.
Need PR to allow deployment on heroku for external testing. (I don't have a local Windows 10 machine)
Also noted: there are two hits to change.js for one change of the colorBy menu item. (not because of this update) This causes a lot of work in unneeded traversal of the nodes and recalculation of the gradients. May need a separate issue.
Related issue(s)
Fixes # 1005
Related to #897 #947
Thank you for contributing to Nextstrain!