second attempt at scrolled long output #1825

Merged
merged 6 commits into from Jun 18, 2012

Conversation

Projects
None yet
3 participants
Owner

minrk commented Jun 1, 2012

uses max-height css property

This is dramatically simpler than the previous implementation, but did require a funky hack to workaround a Firefox CSS bug.

closes #1553

Owner

ellisonbg commented Jun 1, 2012

So i think this is a really good start. I have tested on both Chrome+FF and it behaves reasonably well. Some observations:

  • Right now, clicking and double clicking inside cells is really awkward because of how we force-focus the code mirror cell each click. I am running into this same issue as I play with interactive JS widgets. It is really a separate issue, but we need to get rid of this force-focus behavior so that mouse click actions in output areas behave more sensibly.
  • I have mixed feelings about a fixed max-height value. What about also making the output div resizable?
  • I observe very wacky things when I run the following in a cell in Chrome (FF is OK).
for i in range(200):
    print 200*'x'

Make your window narrow so you see horizontal scroll bars as well. Then double-click to expand and things go crazy.

Owner

minrk commented Jun 1, 2012

Right now, clicking and double clicking inside cells is really awkward because of how we force-focus the code mirror cell each click. I am running into this same issue as I play with interactive JS widgets. It is really a separate issue, but we need to get rid of this force-focus behavior so that mouse click actions in output areas behave more sensibly.

Agreed.

I have mixed feelings about a fixed max-height value. What about also making the output div resizable?

Possible - I'll look into it.

I observe very wacky things when I run the following in a cell in Chrome (FF is OK).

What would you have it do? When it expands in Chrome, I see no difference from the output in master, other than the fact that the cell is highlighted, which looks a bit funny, but is no different than if you highlight the same cell on master.

It seems impossible to have one dimension of overflow be scrolling and the other be visible at the same time, so the choices seem to be:

truncated:
scroll-y and hidden-x
scroll-y and scroll-x
hidden-x,y
expanded:
visible-y,
scroll-x
visible-x
hidden-x

Owner

fperez commented Jun 4, 2012

Did you see @hmeine's comments in #1553? Just pinging here; @hmeine: let's continue the feedback here on the PR itself so we don't accidentally miss something and merge prematurely. Thanks for pitching in.

Owner

minrk commented Jun 4, 2012

Yes, I'll put a slight tint while its elided, like I did in the first go. I've been playing with making it resizable instead of static, as Brian suggested, but I'm about to give up on that, as it seems to make everything worse.

Assuming the scrolled size is static, whereabouts do we want to draw the line? I have it currently at ~30 lines, so only quite long output will get scrolled, but I have no trouble with making it smaller or longer. A smaller size makes the scrolling itself a bit less annoying, but makes it come up a lot more often (net more annoying, I expect).

Owner

fperez commented Jun 4, 2012

I was thinking about having the cutoff be dynamic, with the following logic:

  • nlines > 50: scroll on, but scroll window kept at 30 lines
  • nlines < 50: no scroll

where we can fine tune 30 and 50. But the point is: we only activate scrolling if there's actually a fair amount to scroll, rather than using a fixed cutoff that would give us a scrollbar for just a few lines.

How does that sound?

Owner

minrk commented Jun 4, 2012

How does that sound?

Pretty much exactly like my first attempt in Boulder ;). I guess I can't do it purely with CSS, but I think I've steeped in jQuery enough to do it a little better this time.

Owner

fperez commented Jun 4, 2012

On Sun, Jun 3, 2012 at 9:21 PM, Min RK
reply@reply.github.com
wrote:

Pretty much exactly like my first attempt in Boulder ;).  I guess I can't do it purely with CSS, but I think I've steeped in jQuery enough to do it a little better this time.

Hey, great minds think alike ;)

Owner

minrk commented Jun 4, 2012

I made some adjustments, and I think it works pretty nicely, on webkit at least. It won't auto-scroll until ~50 lines, and the abbreviated view is ~20. I also added a gradient border for the abbreviated view, so it looks like there is something to scroll through:

img

The mozilla flexible-box implementation, unfortunately, appears to be a total shit show, consistently computing radically incorrect height values. And bizarrely, the cell height seems to be a function of the cursor position in an input cell (presumably CodeMirror is doing something funky). To see this, move the cursor back and forth across the last few characters of a line. This is already apparent in master, but the behavior is much more problematic with the elided output.

Owner

ellisonbg commented Jun 5, 2012

On Mon, Jun 4, 2012 at 1:38 AM, Min RK
reply@reply.github.com
wrote:

I made some adjustments, and I think it works pretty nicely, on webkit at least.  It won't auto-scroll until ~50 lines, and the abbreviated view is ~20.  I also added a gradient border for the abbreviated view, so it looks like there is something to scroll through:

img

The mozilla flexible-box implementation, unfortunately, appears to be a total shit show, consistently computing radically incorrect height values.  And bizarrely, the cell height seems to be a function of the cursor position in an input cell (presumably CodeMirror is doing something funky).  To see this, move the cursor back and forth across the last few characters of a line.  This is already apparent in master, but the behavior is much more problematic with the elided output.

Yes, this is something I ran into previously. The particular thing
that seems to mess it up is the combination of scrolling and the
flexible-box model. Don't know what to do about it.

I was thinking more about this though. I think that whatever we do
needs to incorporate the existing "Toggle Output" functionality. By
this I mean that we should have a single UI that handles all three
output states:

  • Fully hidden.
  • Shown, but shortened/scrolling.
  • Fully shown.

Reply to this email directly or view it on GitHub:
#1825 (comment)

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

Owner

minrk commented Jun 5, 2012

I'm just about to push a fix that plays fairly nicely with FF. It's specifically max-height that it just totally doesn't understand, but with the current logic I can just use height instead of max-height, because the CSS only applies when output is larger than that. This wasn't true at the earlier part in the process.

  • Fully hidden.
  • Shown, but shortened/scrolling.
  • Fully shown.

I agree 99%. I might add that if you toggle output, it should remember whether it's scrolled up or not. If it were stored in a single three-way variable, then collapsing would forget whether it is scrolled up or not.

What do we want for menu items:

We currently have:

  • toggle current cell output (collapse/expand, orthogonal to scroll)

But might also have some subset of:

  • collapse all outputs | this output
  • expand all outputs | this output
  • scroll all long outputs | this output

Of course, all of these are easy, but I imagine we probably don't want all of them.

Also, we currently store 'collapsed' in the notebook format. Do we want to do this for the scrolled state as well? I would think not.

Owner

ellisonbg commented Jun 5, 2012

Glad you have figured out something that plays well with FF.

I think the first step should be to figure out what the mouse driven UI is like for switching between output views. Then we should come up with a good keyboard shortcut to cover the switching.

In terms of the menu options, I am not sure they make sense at all if there is an existing mouse driven UI. Maybe we could just have the one to toggle the output of all cells.

In terms of the notebook format, do you think it makes sense to even save collapsed at all?

Owner

minrk commented Jun 5, 2012

I think the first step should be to figure out what the mouse driven UI is like for switching between output views. Then we should come up with a good keyboard shortcut to cover the switching.

Right now, it's double-click to toggle between scrolled/full-expand. There is no mouse UI for hiding the output besides the menu entry, and I'm not sure there's a need for one.

Given states:

C - collapsed
S - scrolled
E - expanded

I only see a reason for the following events, in either mouse or keyboard UI:

  • (S|E) <=> C (currently ^M-o, Menu entry)
    and
  • S <=> E (currently double-click only)

There aren't many sensible keyboard shortcuts left (S,C,O,L are all taken), so I am not sure it's worth taking up an extra one for toggling the scroll.

I'm not sure a three-way switch makes sense, but maybe the current ^M-o shortcut should cycle through collapse/scroll/expand.

As for the menu entries, I think it might make sense to have the three "apply state X to all cells" options, possibly keeping the existing toggle current or not.

I think W,E,R shortcuts are all available, so they could be collapse (like close which is ^W), Expanded, and scRoll?

In terms of the notebook format, do you think it makes sense to even save collapsed at all?

It seems a little bit odd to me, but the use case that comes to mind is a demo/teaching notebook where you don't want all your outputs to be visible on load, but you still want them stored in the notebook.

Owner

ellisonbg commented Jun 7, 2012

Functionally, I think this is really starting to work well. A few comments:

  • I think we definitely need a simple mouse driven UI that switches between all 3 states. I think that a dblclick based switch makes sense, but we need to add the hidden output state to the UI. Part of the problem with the current hide output state is that it doesn't provide any indication that there is hidden input for that cell. When the output is hidden, we should probably insert/show a div to indicate the hidden output and which a user can dblclick on to unhide things.
  • I don't think the current gradient on the top+bottom border of the scrolling area works. It could be that the shadow is not gradual enough so it looks like a thicker broder. I think the lack of a shadow on the R/L is also an issue, as is the lack of a symmetric space on the L side between the output area and the cell's border. We will probably just have to play around with this.
Owner

minrk commented Jun 7, 2012

What browser are you using? The gradient works very well for me in current FF, Chrome, and Safari on OS X. I can make it longer, but it doesn't seem like it should be more than one character high.

Owner

ellisonbg commented Jun 7, 2012

I am using Chrome and on my browser, it looks exactly like the screenshot posted in the PR above. I would see about making it wider (higher) but more "faded" so it is more obvious that it is a shadow.

Owner

minrk commented Jun 8, 2012

Your monitor must be a different color than mine, because it's rather clear on any of my machines. I did double it, which seems like a bit too much to me, but not too bad. I also accommodated the asymmetrical padding in the parent container, and added rounded corners for symmetry:

screenshot

Owner

minrk commented Jun 8, 2012

Do you want a mouse UI for hiding output? I have a clickable area for unhiding it, but you still have to go to the menu or keyboard to hide it:

screenshot

Were you suggesting that it be double-click each to cycle through the three states? That's doable, but I'm not sure it's desirable. 3-way switches are not intuitive.

Owner

ellisonbg commented Jun 8, 2012

Yes, I think we do want a mouse UI for hiding. Not sure if we want to
have it be a full 3 way switch or if the hiding/unhiding toggle should
be a separate action from the other two states. I agree that strict 3
way switches are not ideas.

On Thu, Jun 7, 2012 at 6:17 PM, Min RK
reply@reply.github.com
wrote:

Do you want a mouse UI for hiding output?  I have a clickable area for unhiding it, but you still have to go to the menu or keyboard to hide it:

screenshot

Were you suggesting that it be double-click each to cycle through the three states?  That's doable, but I'm not sure it's desirable.  3-way switches are not intuitive.


Reply to this email directly or view it on GitHub:
#1825 (comment)

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

Owner

ellisonbg commented Jun 8, 2012

Your monitor must be a different color than mine, because it's rather clear on any of my machines.  I did double it, which seems like a bit too much to me, but not too bad.  I also accommodated the asymmetrical padding in the parent container, and added rounded corners for symmetry:

screenshot

There are a few things I think might be worth trying:

  • No shadow at all. When the output area is in scrolling mode, it
    does already have scroll bars to indicate the output area visually.
    In the spirit of our uncluttered UI, this might be sufficient.
  • Lighter shadow around all 4 edges.
Owner

minrk commented Jun 8, 2012

Your monitor must be a different color than mine, because it's rather clear on any of my machines. I did double it, which seems like a bit too much to me, but not too bad. I also accommodated the asymmetrical padding in the parent container, and added rounded corners for symmetry:

screenshot

There are a few things I think might be worth trying:

  • No shadow at all. When the output area is in scrolling mode, it
    does already have scroll bars to indicate the output area visually.
    In the spirit of our uncluttered UI, this might be sufficient.

Not reliably true - many environments (Ubuntu, OS X) are moving towards having no visible scroll bars when not scrolling.

This is what I had at first, and it was pretty rough. I am pretty convinced at this point that the shadow idea is where we want to end up, and I think we can tweak until it feels right.

  • Lighter shadow around all 4 edges.

I'll give this a try, shrinking it back down, because this larger shadow is much too thick to have all the way around. And maybe trying a lighter end color. I will also try a 1px solid border on the sides, because I doubt a horizontal gradient will look nice.


Reply to this email directly or view it on GitHub:
#1825 (comment)

Owner

minrk commented Jun 8, 2012

After some more fiddling, here's where it is now: lighter, shorter shadow, with 1px border for the sides:

screenshot

As you can see, there are no scrollbars because I am using default settings on OS X.

What Mouse UI did you have in mind for collapsing the output? With double-click toggling scroll, I don't see an obvious choice aside from creating a button somewhere, which seems counter to our design.

Owner

minrk commented Jun 8, 2012

I found native box-shadow CSS, which seems more sensible, so here's Firefox with a scrollbar:

firefox

And here's Chrome without:

chrome

Owner

ellisonbg commented Jun 8, 2012

Ahh, this explains things as I thought you were using the CSS shadows all along. This looks spot on! Great work. It is a bit of a bummer that the FF and Chrome versions look different - I like the thinner Chrome version. But this is fine.

I will comment separately about the other issues.

Owner

ellisonbg commented Jun 8, 2012

I am really liking this now and it works and looks great on both FF and Chrome. Only two other comments:

  • We have to be very careful about adding mouse event handlers to the output area. The reason is that in the future there will be JavaScript widgets in that div that can register its own arbitrary event handlers. Because we are using dblclick, those widgets will not ever be able to use that event. I don't have any good alternatives in mind, but we may want to think about this.
  • I really do think we want a mouse driven UI for hiding the output. I like how hiding/unhiding is independent of the scrolling and the reshowing remembers the scrolling state. I don't know that we need to solve this issue right now though. @fperez any ideas on this one?
Owner

minrk commented Jun 8, 2012

  • We have to be very careful about adding mouse event handlers to the output area. The reason is that in the future there will be JavaScript widgets in that div that can register its own arbitrary event handlers. Because we are using dblclick, those widgets will not ever be able to use that event. I don't have any good alternatives in mind, but we may want to think about this.

I agree - I think dblclick is the only event we can even consider for this reason, but it has obvious disadvantages. For instance, dblclick here would conflict with dblclick mentioned in #1887. The only option I see is to restrict our click actions to the prompt area on the left. I tried this at some point, but since there are actually N prompt divs rather than one, it was a bit funky, and I gave up. I can try floating a single div overlay on the whole prompt area again, and that would then be the active area for scroll/hide, rather than the whole div.

If we do focus on the prompt area, I could have, for instance, buttons that only appear on hover on the prompt area. That would remove the conflict with output widget events, and might even make the scroll/hide more discoverable.

Owner

ellisonbg commented Jun 8, 2012

On Fri, Jun 8, 2012 at 11:51 AM, Min RK
reply@reply.github.com
wrote:

  • We have to be very careful about adding mouse event handlers to the output area. The reason is that in the future there will be JavaScript widgets in that div that can register its own arbitrary event handlers. Because we are using dblclick, those widgets will not ever be able to use that event. I don't have any good alternatives in mind, but we may want to think about this.

I agree - I think dblclick is the only event we can even consider for this reason, but it has obvious disadvantages.  For instance, dblclick here would conflict with dblclick mentioned in #1887. The only option I see is to restrict our click actions to the prompt area on the left.  I tried this at some point, but since there are actually N prompt divs rather than one, it was a bit funky, and I gave up.  I can try floating a single div overlay on the whole prompt area again, and that would then be the active area for scroll/hide, rather than the whole div.

If we do focus on the prompt area, I could have, for instance, buttons that only appear on hover on the prompt area.  That would remove the conflict with output widget events, and might even make the scroll/hide more discoverable.

If we want to have a strict "no event" policy for the area that
widgets will appear over, then I think the floating div over the
prompt area makes sense. Not sure how easy it will be to pull that
off though. It does open the door for a hover based UI, but we have
to remember that the output prompt area can be as small as 1 char
high.

Do you want to give this a shot before we make the decision on the UI
for image resizing?


Reply to this email directly or view it on GitHub:
#1825 (comment)

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

Owner

minrk commented Jun 8, 2012

I think "no events" make sense, so I'll at least give the div another try.

For scroll/expand at least, height is not an issue because those are no-ops unless the area is at least 20em high, so they need not be drawn. This is once again a disadvantage of conflating UI for collapse, which makes sense for one line, and scroll, which does not, and is this much easier.

-MinRK

On Jun 8, 2012, at 12:08, "Brian E. Granger"reply@reply.github.com wrote:

On Fri, Jun 8, 2012 at 11:51 AM, Min RK
reply@reply.github.com
wrote:

  • We have to be very careful about adding mouse event handlers to the output area. The reason is that in the future there will be JavaScript widgets in that div that can register its own arbitrary event handlers. Because we are using dblclick, those widgets will not ever be able to use that event. I don't have any good alternatives in mind, but we may want to think about this.

I agree - I think dblclick is the only event we can even consider for this reason, but it has obvious disadvantages. For instance, dblclick here would conflict with dblclick mentioned in #1887. The only option I see is to restrict our click actions to the prompt area on the left. I tried this at some point, but since there are actually N prompt divs rather than one, it was a bit funky, and I gave up. I can try floating a single div overlay on the whole prompt area again, and that would then be the active area for scroll/hide, rather than the whole div.

If we do focus on the prompt area, I could have, for instance, buttons that only appear on hover on the prompt area. That would remove the conflict with output widget events, and might even make the scroll/hide more discoverable.

If we want to have a strict "no event" policy for the area that
widgets will appear over, then I think the floating div over the
prompt area makes sense. Not sure how easy it will be to pull that
off though. It does open the door for a hover based UI, but we have
to remember that the output prompt area can be as small as 1 char
high.

Do you want to give this a shot before we make the decision on the UI
for image resizing?


Reply to this email directly or view it on GitHub:
#1825 (comment)

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


Reply to this email directly or view it on GitHub:
#1825 (comment)

Owner

minrk commented Jun 8, 2012

Oh, for crying out loud. Once again, I have a nice, working implementation on Webkit of hover on the prompt area, with click to toggle scroll / dblclick to collapse.

BUT

Firefox has no idea where these elements should be placed.

I cannot wait until Firefox is completely abandoned or at least abandons their renderer.

Owner

minrk commented Jun 8, 2012

Since Firefox does such a bad job at everything, I am inclined to disable the scrolled output for FF altogether. Any objection, @fperez? ;)

third attempt at scrolled long output
click/double-click on prompt area for toggling scroll/collapse
Owner

minrk commented Jun 8, 2012

Okay, I managed to beat FF into submission. UI is now entirely on the prompt area, which has highlight-on-hover, and the following events:

click: toggle scroll
dblclick: collapse

Based on #1881, #1832, I think it makes good sense to never bind events on the whole output area, to avoid conflict with widgets / single-element events.

Owner

fperez commented Jun 11, 2012

Sorry I hadn't gotten around to giving feedback on this, it's looking great!! A couple of minor comments:

  • I think the calltip should say "double click" and not "dblclick", since that's meant for human reading.
  • Would it make sense to shade lightly the left active area or otherwise somehow indicate it's different? It took me a while to realize that's how I could expand the scrollable area...
Owner

minrk commented Jun 11, 2012

  • I think the calltip should say "double click" and not "dblclick", since that's meant for human reading.

Certainly, done.

  • Would it make sense to shade lightly the left active area or otherwise somehow indicate it's different? It took me a while to realize that's how I could expand the scrollable area...

It would make sense, but it's a pretty high aesthetic cost to have coloring on the entire prompt column (and worse, only on the output sections of the prompt column, which are the only actual active area).

I don't know what would be best. I can add hover text on the whole output area that directs you toward the prompt area.

UI that helps you find your way around the first few uses, but gets out of your way once it is all familiar is always a hard balance to strike.

Owner

fperez commented Jun 11, 2012

On Sun, Jun 10, 2012 at 8:06 PM, Min RK
reply@reply.github.com
wrote:

UI that helps you find your way around the first few uses, but gets out of your way once it is all familiar is always a hard balance to strike.

Yeah, I thought the same. Perhaps we should just leave it as-is, and
we can always refine over time if we notice it's really a stumbling
block for users.

In that case, this is looking merge-ready, no?

Owner

minrk commented Jun 11, 2012

I haven't heard from @ellisonbg on the last revision (which was essentially a total rewrite since the previous discussion).

The only thing discussed above that I hadn't added yet (but just pushed) was a keyboard shortcut for toggling scroll. Since we use ^M-o for show/hide, I went with ^M-O for toggling scroll. other options include: ^M-o cycles through three states, use non-caps option for toggling scroll, no shortcut for scroll.

Owner

ellisonbg commented Jun 11, 2012

Opps, I forgot that I hadn't replied. I think I composed the email in
my head while I was driving home. I can't do it this second (grade
are due tomorrow), but will try to get back to this by tomorrow.

On Sun, Jun 10, 2012 at 9:29 PM, Min RK
reply@reply.github.com
wrote:

I haven't heard from @ellisonbg on the last revision (which was essentially a total rewrite since the previous discussion).

The only thing discussed above that I hadn't added yet (but just pushed) was a keyboard shortcut for toggling scroll.  Since we use ^M-o for show/hide, I went with ^M-O for toggling scroll.  other options include: ^M-o cycles through three states, use non-caps option for toggling scroll, no shortcut for scroll.


Reply to this email directly or view it on GitHub:
#1825 (comment)

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

Owner

fperez commented Jun 11, 2012

OK, there's no rush. BTW, I've had a ton of those 'write email in my head' lately. I really want not just siri, but flat-out telepathic email :)

Owner

fperez commented Jun 13, 2012

Quick status check: should we try to merge this before the first beta? We said we'd put it out by tomorrow...

Owner

minrk commented Jun 13, 2012

I can imagine people liking / being annoyed by it, so I think it should be tested and ironed out by users. But I would like @ellisonbg's feedback before merging.

Owner

fperez commented Jun 13, 2012

OK, then let's hold off on the beta for this one (unless Brian, you're too busy for it now, case in which we can put it in after the beta).

Owner

minrk commented Jun 14, 2012

alignment and color issue brought up by @ellisonbg on Skype should both be addressed.

Owner

ellisonbg commented Jun 16, 2012

So I really like the look of the prompt overlay hover effect. Still some glitches in the margins though:

  • In Chrome, when the output area is in scrolling mode, the R margin between the output area and cell border is missing. Ironically, FF doesn't have this problem.
  • In FF, the width of the prompt overlay is wider than in Chrome. On FF is actually extends past the left edge of the input area (out of the prompt area's width).

To be consistent with the styling, do you think we should style the button to un-hide the output area in the same way as the prompt overlay? Not sure how I feel about this.

Owner

minrk commented Jun 17, 2012

I don't see any weirdness on the RHS in Chrome or FF, so I don't know what you mean by that. I fiddled around with various manual styling on the collapsed button, and everything I've found so far is worse than the existing button. If we want to cut the beta, I would merge this as-is, and allow some CSS fine-tuning between now and final.

minrk added a commit that referenced this pull request Jun 18, 2012

Merge pull request #1825 from minrk/elide2
second attempt at scrolled long output

Some amount of CSS tweaking will probably want to be done before 0.13 final,
but this is good enough for beta.

closes #1553

@minrk minrk merged commit ef57f6e into ipython:master Jun 18, 2012

@minrk minrk deleted the minrk:elide2 branch Mar 31, 2014

mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014

Merge pull request #1825 from minrk/elide2
second attempt at scrolled long output

Some amount of CSS tweaking will probably want to be done before 0.13 final,
but this is good enough for beta.

closes #1553
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment