Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix interact animation page jump FF #5459

Merged
merged 10 commits into from
Apr 8, 2014
Merged

Conversation

jdfreder
Copy link
Member

Firefox doesn't render images immediately as the data is available. When animating the way that we animate, this causes the output area to collapse quickly before returning to its original size. When the output area collapses, FireFox scrolls upwards in attempt to compensate for the lost vertical content (so it looks like you are on the same spot in the page, with respect to the contents below the image's prior location). The solution is to resize the image output after the img onload event has fired.

This PR:

  • Releases the clear_output height lock after the image has been loaded (instead of immediately or using a timeout).
  • Removes a setTimeout call in the append_output method.
  • clear_output in zmqshell no longer sends \r to the stream outputs.

closes #5128

@takluyver
Copy link
Member

I still see the issue with this branch, unfortunately. Also, the output area overlaps the cell below:

screenshot from 2014-03-28 09 30 48

@jdfreder
Copy link
Member Author

I still see the issue with this branch, unfortunately

Strange, the output area shouldn't be collapsing like I see in your screenshot. What version of FF are you running? On Linux?

@takluyver
Copy link
Member

28, on Linux. Technically it's Iceweasel, because of some issue Debian has, but the only difference is the branding - the code is all Firefox.

I hit Ctrl-F5 several times, and then tried opening it in a private window, but the problem persisted, so I don't think it's a caching issue. But there could be something I didn't manage to clear.

@jdfreder
Copy link
Member Author

But there could be something I didn't manage to clear.

Woops, actually it was a mistranslation from my FF "scratchpad" to the IPython codebase. s/this/that - literally. I amended the commit, please test again 😄

@jdfreder
Copy link
Member Author

Also, it seems like sometimes, on my machine FF will hickup and won't render the image within 1/4 second. When this happens, the page jumps... I don't know if we can fix that though (other than increasing the timeout).

@takluyver
Copy link
Member

OK, I no longer see the overlapping. But I can very easily still trigger the page jump, even if I'm pretty gentle with the sliders. I can see that the fix is there, but it's not nearly enough. Even if I increase it to 500ms, I can still get the jump without much trouble.

Are you testing with the same example notebook? It renders a 3d graph, which presumably takes longer than producing a trivial 2d example.

Is there no way we can hook the resizing to an image onload event or something?

@jdfreder
Copy link
Member Author

It renders a 3d graph, which presumably takes longer than producing a trivial 2d example.

I'm on my desktop, which runs at least an order of magnitude faster than my laptop.

Is there no way we can hook the resizing to an image onload event or something?

That's a good idea. I think I'd have to parse the output to see if it had an image though.

@takluyver
Copy link
Member

I'm also on my desktop, which has 12GB of RAM and a quad core Xeon, so if things are happening too slow here, we've got problems. ;-)

@jdfreder
Copy link
Member Author

12GB of RAM and a quad core Xeon, so if things are happening too slow here, we've got problems. ;-)

That is a lot of RAM!

@takluyver
Copy link
Member

The real issue is clearly that the RAM is so spacious and comfortable that data is reluctant to leave it.

@jdfreder
Copy link
Member Author

The real issue is clearly that the RAM is so spacious and comfortable that data is reluctant to leave it.

lol

In the last commit I changes it so that the resize waits for the img onload event... I can still get the jump, but less often, can you confirm that it is a step in the right direction? The jQuery docs mention that it may be inconsistent - https://api.jquery.com/load-event/

@minrk
Copy link
Member

minrk commented Mar 28, 2014

I don't think changes to signatures and events like this can make it into 2.0. Marking for 3.0.

@minrk minrk added this to the 3.0 milestone Mar 28, 2014
@minrk
Copy link
Member

minrk commented Mar 28, 2014

Probable candidate for 2.1, though.

@jdfreder
Copy link
Member Author

Marking for 3.0.

I think that's a good idea. You can achieve flicker&jump-free widget based matplotlib animations in master, it just requires a different approach than the one used in this notebook.

@takluyver
Copy link
Member

Sorry, I can still trigger it very easily. It looks like whenever I move the slider fast enough that there's more than one update processing at a time, it goes wrong. Could it be to do with overlapping messages?

@jdfreder
Copy link
Member Author

Could it be to do with overlapping messages?

Good thought! I'll check

@takluyver takluyver modified the milestones: 2.1, 3.0 Mar 28, 2014
@takluyver
Copy link
Member

I've made a 2.1 milestone so we can start tracking things like this.

@jdfreder
Copy link
Member Author

jdfreder commented Apr 2, 2014

@takluyver woohoo! I think I've figured this out. For some reason rouge newline characters (just chr13) were getting sent on both stdout and stderr for each time the plot was displayed. I haven't removed the other logic yet or looked into why that character is being sent alone, but could you give this a try and see if it fixes the problem for you now?

I'll look into removing all of the extra logic in the meantime, and getting to the root cause of the single chr13 stream msgs.

@ellisonbg
Copy link
Member

Awesome! Weird!

On Wed, Apr 2, 2014 at 11:13 AM, Jonathan Frederic <notifications@github.com

wrote:

@takluyver https://github.com/takluyver woohoo! I think I've figured
this out. For some reason rouge newline characters (just chr13) were
getting sent on both stdout and stderr for each time the plot was
displayed. I haven't removed the other logic yet or looked into why that
character is being sent alone, but could you give this a try and see if it
fixes the problem for you now?

I'll look into removing all of the extra logic in the meantime, and
getting to the root cause of the single chr13 stream msgs.

Reply to this email directly or view it on GitHubhttps://github.com//pull/5459#issuecomment-39364201
.

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

@takluyver
Copy link
Member

Sorry, I still see the bug. Again, I've had a good go at clearing the cache with ctrl-F5, and tried in a private window as well, but it still happens.

@jdfreder
Copy link
Member Author

jdfreder commented Apr 2, 2014

Can you look at your JS log? As you play with the sliders you should see a stream of integer pairs, there shouldn't be any '$el' is undefined. If you don't see anything like that, can you make sure you have e8af28d ? If you do have the stream of integers, they should always be close to each other (around 460 on my browser). If the height collapses and the page scrolls, you should see a small number (around 6 on my browser).

@jdfreder
Copy link
Member Author

jdfreder commented Apr 2, 2014

hmmm, trying to reproduce the problem in a fiddle: http://jsfiddle.net/CTzxC/21/

@takluyver
Copy link
Member

Here's the stream of numbers:

466 460 outputarea.js:290
466 460 outputarea.js:290
30 0 outputarea.js:290
466 460 outputarea.js:290
466 0 outputarea.js:290
466 460 outputarea.js:290
30 0 outputarea.js:290
466 460 outputarea.js:290

The 30 0 lines are where it jumped. 466 0 was another point where I was moving the slider quickly, but I didn't appear to get a jump. Nothing about undefined names. However, when the widgets are created, I do get a message:

Use of Mutation Events is deprecated. Use MutationObserver instead.

@takluyver
Copy link
Member

I should also say that Firefox collapses multiple adjacent copies of the same message - there were multiple copies of the 466 460 lines for each time they're printed above.

It looks like your computer clock is off again, by the way ;-)

@jdfreder
Copy link
Member Author

jdfreder commented Apr 2, 2014

Thanks @takluyver for being patient and helping me debug this one 😃

Use of Mutation Events is deprecated. Use MutationObserver instead.

In the keyboard manager, where the exception for the widget area is registered, we use a DOM event to register exceptions on new elements that are added as children. I'm pretty sure that's the code that is raising that warning.

Here's the stream of numbers:

It's much harder for me to reproduce this on my laptop (even though it is < my desktop < your desktop)... I did manage to reproduce it again (takes upwards of 2 to 3 minutes of continual sliding 😦 ). I'm going to continue trying to replicate it in the fiddle. BTW: I haven't seen 30 0 yet, but I have seen 466 0. I wonder if that's a clue I can use.

@takluyver
Copy link
Member

Thank you for being patient with the debugging process!

Is it possible that it's more likely to occur on faster machines, not slower ones? I can't immediately see a mechanism for that, but the empirical evidence appears to point that way?

@jdfreder
Copy link
Member Author

jdfreder commented Apr 2, 2014

Awesome! Cake!

I've cleaned up the code by removing the extra logs and reverting back to jQuery events. I pushed the changes in a new commit instead of amending so we can fall back to ff8e16c if we need to.

I think I actually may see why the flicker is happening... I'll ping again if I find an easy fix (to add into this PR).

Also I'll update the description in a bit, summarizing all of the things wrong in master.

@takluyver
Copy link
Member

Pulled again, still seems to be working.

@jdfreder
Copy link
Member Author

jdfreder commented Apr 2, 2014

So I shrunk the number of operations between the clear output and image insertion calls in hopes that it would remedy the flicker, but it didn't. I'll open a new issue for the flicker along with a video.

@jdfreder jdfreder changed the title Move append_output animation height lock release into timeout. Fix interact animation page jump FF Apr 2, 2014
@jdfreder
Copy link
Member Author

jdfreder commented Apr 3, 2014

  • updated so zmqshell no longer sends \r characters.

print('\r', file=sys.stdout, end='')
print('\r', file=sys.stderr, end='')
self._flush_streams()

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we be making the corresponding changes to frontends here, or merge this and clean up in another PR?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the terminal even uses this clear_output (it uses the one in core/displaypub) and when I was testing clear_output in qtconsole, I was unsure of what I was supposed to be looking for. In master, clear_output doesn't seem to do anything to the stream output inside the qtconsole.

I tried something along the lines of:

from time import sleep
from IPython.display import clear_output
for i in range(10):
    clear_output(False)
    print str(i),
    sleep(0.5)

which didn't work. But this does work:

from time import sleep
for i in range(10):
    print '\r' + str(i),
    sleep(0.5)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect we want to keep the _flush_streams call

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thinking is that the Qt console and the terminal zmq console (ipython console) should turn the clear_ouput message into print '\r'. Even if it doesn't work in master at the moment.

@jdfreder
Copy link
Member Author

jdfreder commented Apr 7, 2014

ipython console and ipython qtconsole are both aware of the changes to clear_output now.

# widget's tab width.
text = text.expandtabs(8)

print([ord(c) for c in text])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like leftover debugging?

@takluyver
Copy link
Member

Other than those comments, I think this looks OK.

@jdfreder
Copy link
Member Author

jdfreder commented Apr 8, 2014

Comments addressed.

@jdfreder
Copy link
Member Author

jdfreder commented Apr 8, 2014

@takluyver I --amended the commit

@takluyver
Copy link
Member

Thanks @jdfreder . Travis is happy, merging.

takluyver added a commit that referenced this pull request Apr 8, 2014
Fix interact animation page jump FF
@takluyver takluyver merged commit 5c3a5f1 into ipython:master Apr 8, 2014
@jdfreder jdfreder deleted the ff_animhop branch April 8, 2014 21:23
minrk added a commit that referenced this pull request Apr 9, 2014
Firefox doesn't render images immediately as the data is available.  When animating the way that we animate, this causes the output area to collapse quickly before returning to its original size.  When the output area collapses, FireFox scrolls upwards in attempt to compensate for the lost vertical content (so it looks like you are on the same spot in the page, with respect to the contents below the image's prior location).  The solution is to resize the image output after the `img onload` event has fired.

This PR:
- Releases the `clear_output` height lock after the image has been loaded (instead of immediately or using a timeout).
- Removes a `setTimeout` call in the `append_output` method.
- `clear_output` in zmqshell no longer sends `\r` to the stream outputs.

closes #5128
@maxalbert
Copy link
Contributor

Great work everyone, many thanks for tracking this down! (I just hit the same error and was in the process of submitting a bug report when I found this.)

One question: when the output is larger than the screen (e.g. due to multiple widgets taking up a lot of vertical space) the window still scrolls back to the input cell when a different parameter value is selected (I see this with latest master in both Firefox and Chromium). Thus is it possible to disable the scrolling altogether so that the visible area remains at exactly the same position even when a different value is chosen in a widget? I'm happy to provide a sample notebook illustrating this issue if required.

Many thanks!

mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
Fix interact animation page jump FF
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Page jumping when output from widget interaction replaced
5 participants