OneZoom / OZtree

OneZoom Tree of Life Explorer
Other
90 stars 20 forks source link

Bugs introduced recently that were not there before #140

Open jrosindell opened 5 years ago

jrosindell commented 5 years ago
  1. [ ] (1) Refresh loop a little slow - feels unresponsive when mouse moves over a clickable area
  2. [x] (2) Back button does not work at all - it used to take user back to previous places in the tree saved by URL
  3. [x] (3) In china loading is hanging on jquery.min - we should use our own local version only and not from Google which is blocked inside China
  4. [x] (4) Mouseover for Cyanobacteria doesn’t work properly - live area on node doesn’t include expected spaces
  5. [x] (5) Flickering graphics in tips of the tree when mouse moves over signposts
  6. [x] (6) Can scroll whole page up and down on mobile from within pop down boxes - this should be disallowed. (actually this might have been present before, but needs fixing in any case)
  7. [x] (7) There is a bug in the interior signpost text - if you zoom in enough the signpost text apparently moves down and stays the same size - this feels like unwanted behaviour.
  8. [x] (8) it's no longer possible to click on the common ancestor button from advanced search and get to the common ancestor - possibly related to issues with flight but more likely something simpler related to changing the behaviour elsewhere from jump to fly but not doing the same everywhere else.
  9. [ ] (9) The main tree views in OneZoom (not OTOP or polytomy) have a restrictor in place which prevents the screen from been zoomed and moved away from the tree when using the mouse - hence you can't get lost in whitespace. This feature does not translate to OTOP and polytomy view. I think this must be because of a missing instruction in the node drawing routine used for both cases.
jrosindell commented 5 years ago

5.) Flickering graphics in tips of the tree when mouse moves over signposts

6.) Can scroll whole page up and down on mobile from within pop down boxes - this should be disallowed. (actually this might have been present before, but needs fixing in any case)

7.) There is a bug in the interior signpost text - if you zoom in enough the signpost text apparently moves down and stays the same size - this feels like unwanted behaviour.

jrosindell commented 5 years ago

8.) it's no longer possible to click on the common ancestor button from advanced search and get to the common ancestor - possibly related to issues with flight but more likely something simpler related to changing the behaviour elsewhere from jump to fly but not doing the same everywhere else.

lentinj commented 5 years ago

(3): @hyanwong @jrosindell onezoom actually loaded jQuery twice, once in web2py and once from google in layouts. I'm not sure if there's a reason for the google / fall back to local logic, but given we load a local copy regardless I've ripped out the fallback.

lentinj commented 5 years ago

(2): @jrosindell the above should fix this, however jump_to_position() feels like a hack to me, and should be using something in position_helper (e.g.)

lentinj commented 5 years ago

(4) Mouseover for Cyanobacteria doesn’t work properly - live area on node doesn’t include expected spaces

is_mouseover_high_res_text() assumes the node will have a bunch of images, and checks that the mouse is outside the central area. This assumption is obviously incorrect in the Cyanobacteria case.

Seems to me the easiest solution is to just remove this condition, in which case mouse-overing an image within a node overrides the main node mouse-over anyway. Not sure if there's other cases that this will cause a problem for though.

lentinj commented 5 years ago

(5) Flickering graphics in tips of the tree when mouse moves over signposts

"mouses over signposts" will be anything that triggers the refresh loop to start again, this is another case of unsafe recycling of shape objects.

The above stops a few cases I've noticed with fake "fan" leaves being incorrectly styled, both in OTOP and regular polytomy. Not sure what in particular you are looking at here.

Note that some flickering will be expected, as extra data loads and more nodes get rendered.

lentinj commented 5 years ago
jrosindell commented 5 years ago

(1)

There's no easy wins to speed up the refresh loop at this point. The way to solve this is to have a separate SVG layer where these interactive elements are rendered, and let the browser handle interactions for us. But that's not a task for today.

Yes I see what you mean - I agree with your solution and also think that it's part of a bigger project to improve performance by rendering the trees on multiple canvases.

I have a memory of there being a separate register of 'live areas' against which the position of the mouse is checked. I've noticed that the power consumption goes up a lot just based on mouse move and this could be eliminated as well as solving (1) by triggering the main loop not on mouse move but in case there is a change in status of the mouse intersection with live areas. Mouse move would then trigger only the check for intersection with live areas and not the main redrawing or maintenance loops. I think we'll leave this open for now as Naziha has strongly requested performance issues to be solved if time.

jrosindell commented 5 years ago

(2) thanks I agree it's fixed. I also find this part of the code hacked and I'm not happy with it, but (assuming we're talking about the same thing) it will do for now. My problem is primarily that the x, y and w parameters parsed in only make sense for a window aspect ration and resolution that the URL was made for so they are not the correct things to go into the URL really. Do say however, if you have something else in mind. This is related to #109 and the URL discussion in #139

jrosindell commented 5 years ago

(3) Thanks all solved! (4)

Seems to me the easiest solution is to just remove this condition, in which case mouse-overing an image within a node overrides the main node mouse-over anyway. Not sure if there's other cases that this will cause a problem for though.

I agree to this. Please note that there are lots of layout cases depending on whether there are images or no images, dates or no dates name or no name, and if there is a name if it's latin, common or both. These are already handled in terms of layout - in terms of mouse live areas I expect that it's just a case of whether there are images or not so yes, live area on the complete circle (not the outer part which is separate and links to sponsor only) with override live area on images makes most sense.

jrosindell commented 5 years ago

(5) The problem that I observed on beta does not reoccur at all on the latest version with you fix so I think this is done thanks. (6) OK I remember now, sorry for raising again. I'll tick it off (7) I cannot replicate this on the latest version of the code. However I do fear it's related to being deployed not on a local server, and thus on the delay caused by loading everything required in preparation for the flight start. so I'd like to leave this open until we can deploy the latest code and test it from the server.

jrosindell commented 5 years ago

(7) I poorly described the problem before it's easy to replicate in any browser / any named node.... here is the issue.

jrosindell commented 5 years ago
chimp1
jrosindell commented 5 years ago
chimp2
jrosindell commented 5 years ago

distance of text below the centre of the node join scales differently to size of text itself creating the feeling that the text 'runs away' in terms of its position in space as you zoom in.

lentinj commented 5 years ago

(1)

There's no easy wins to speed up the refresh loop at this point. The way to solve this is to have a separate SVG layer where these interactive elements are rendered, and let the browser handle interactions for us. But that's not a task for today.

Yes I see what you mean - I agree with your solution and also think that it's part of a bigger project to improve performance by rendering the trees on multiple canvases.

I have a memory of there being a separate register of 'live areas' against which the position of the mouse is checked. . . .

If this is done there's no reason to listen for mousemove at all, which would be a dramatic power saving.

jrosindell commented 5 years ago

I agree that (3) and (7) are now resolved - thanks.

Regarding (1) yes exactly it would have a big benefit so let's leave the issue open for this

jrosindell commented 5 years ago

I've also added a (9) based on testing done just now, but I think it's no longer deserving of the essential flag

lentinj commented 5 years ago

(9) seems to work fine in the examples I've tried, both in OTOP and polytomy.

jrosindell commented 5 years ago

(9) works on leaves but not on interior nodes in OTOP or polytomy - I've tried on Safari and Chrome. The issue is being able to navigate to an area of the page that's totally blank. It's definitely still an issue, but I think now this and the other issues are a nice to have

jrosindell commented 5 years ago

(8) I think is solved now, possibly because it was a symptom of some other bug that's been solved elsewhere. Anyway I think we can tick off.

lentinj commented 5 years ago

(8) I think is solved now, possibly because it was a symptom of some other bug

@jrosindell Yeah, it was probably a symptom of flights not starting the refresh loop, like the forward/back buttons.