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

Make scrollbars appear on cell output when the output is too wide #6939

Closed
jasongrout opened this issue Sep 15, 2009 · 19 comments
Closed

Make scrollbars appear on cell output when the output is too wide #6939

jasongrout opened this issue Sep 15, 2009 · 19 comments

Comments

@jasongrout
Copy link
Member

Currently, a scrollbar appears down at the bottom of the entire worksheet, which is only minimally helpful, since you then need to go to the bottom and scroll everything over.

This patch makes scrollbars appear on output that is too wide, but just on that output.

To test, do something like:

f=cos(x)-x
show(f.taylor(x,0,50))

in the notebook

CC: @qed777 @kcrisman @boothby

Component: notebook

Author: Jason Grout

Reviewer: Karl-Dieter Crisman, Mitesh Patel, Jason Grout, Minh Van Nguyen

Merged: Sage 4.1.2.alpha4

Issue created by migration from https://trac.sagemath.org/ticket/6939

@jasongrout
Copy link
Member Author

Attachment: trac-6939-notebook-css-overflow.patch.gz

@kcrisman
Copy link
Member

comment:3

Sweet. I love CSS. Tested successfully on Safari 4 and Firefox 3.5. Wish I had access to IE or Linux versions, but this should be okay.

Check out also:

show(plot(sin(x),-10,10),figsize=[20,20])

So you can now generate big pictures in the notebook if you have a reason to do so.

But question: why not just overflow, not overflow-x, or also overflow-y? Those eternally long outputs can be a real pain at times. Try factorial(10000). Also, how many browsers support CSS3 and not just CSS2? I can imagine a lot of people on XP having problems with some old version of IE.

@kcrisman kcrisman changed the title Make scrollbars appear on cell output when the output is too wide [nearly positive review] Make scrollbars appear on cell output when the output is too wide Sep 16, 2009
@jasongrout
Copy link
Member Author

comment:4

Not overflow-y because Sage already handles long vertical outputs by shunting to a file and suppressing text, and besides, who's to say how tall is too tall? Vertically, we just go by the browser width. It's a lot harder to guess the magical height that is just right.

If you think this is good, then you can put a positive review and we can discuss the overflow-y and see if it is needed.

@kcrisman
Copy link
Member

comment:5

No problem with the vertical, though personally I would prefer a little shorter output, as the screen sometimes jumps rather dramatically and it's hard to scroll back and forth...

What about the CSS3 versus CSS2 issue? On the one hand, I found a quote on css3.info that "Support for these properties is strong as they were first defined by IE6." but on the other hand wikipedia implies that support for this first came in Trident engine 7, which may or may not correspond to IE7... Maybe the best solution is this one: "Try specifying the CSS2 overflow property first ... then specify the CSS3 overflow-x/y properties to override it on browsers that can." Of course, that comment was from 2007.

Anyway, as long as it doesn't cause the page to not render at all, I guess this is okay. If you have time to put in an overflow check first, that would be great.

@kcrisman kcrisman changed the title [nearly positive review] Make scrollbars appear on cell output when the output is too wide Make scrollbars appear on cell output when the output is too wide Sep 17, 2009
@jasongrout
Copy link
Member Author

comment:6

Good point about CSS3 vs. CSS2. According to http://msdn.microsoft.com/en-us/library/bb250395%28VS.85%29.aspx, IE6 supports overflow-x (search for "overflow-x") when in standards-compliant mode.

@jasongrout
Copy link
Member Author

comment:7

Maybe #6835 should get reviewed and then this should be rebased on that? #6835 is definitely the larger patch.

@jasongrout
Copy link
Member Author

comment:8

I meant #6865 in the above comment.

@sagetrac-mvngu
Copy link
Mannequin

sagetrac-mvngu mannequin commented Sep 18, 2009

comment:9

I got the following doctest failure:

sage -t -long devel/sage/sage/server/notebook/cell.py
**********************************************************************
File "/scratch/mvngu/release/sage-4.1.2.alpha1/devel/sage-main/sage/server/notebook/cell.py", line 2293:
    sage: C.html_out()
Expected:
    '\n...<table class="cell_output_box">...</table>'
Got:
    '\n               <div class="cell_output_div">\n               <table class="cell_output_box"><tr>\n               <td class="cell_number" id="cell_number_0" onClick="cycle_cell_output_type(0);">\n                 &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;\n               </td>\n               <td class="output_cell"><div class="cell_div_output_wrap" id="cell_div_output_0"><div class="cell_output_wrap" id="cell_output_0"><pre class="shrunk">5</pre></div><div class="cell_output_nowrap_wrap" id="cell_output_nowrap_0"><pre class="shrunk">5</pre></div><div class="cell_output_html_wrap" id="cell_output_html_0"> </div></div></td></tr></table></div>'
**********************************************************************
1 items had failures:
   1 of   7 in __main__.example_94
***Test Failed*** 1 failures.
For whitespace errors, see the file /home/mvngu/release/sage-4.1.2.alpha1/tmp/.doctest_cell.py
	 [28.1 s]

@jasongrout
Copy link
Member Author

comment:10

It looks like #6865 is getting reviewed pretty quickly, so I'll fix the above doctest when I rebase this patch on top of that one. If this doesn't happen before next Tuesday, I'll just fix the above doctest anyway, since I'd really like to get this into the next version of Sage.

@qed777
Copy link
Mannequin

qed777 mannequin commented Sep 21, 2009

Attachment: trac-6939-notebook-css-overflow.2.patch.gz

Rebased against #6865. Apply only this patch.

@qed777
Copy link
Mannequin

qed777 mannequin commented Sep 21, 2009

comment:12

Patch v2 is rebased against #6865's patch v3.

I also prepended #6939 to the commit string.

@qed777
Copy link
Mannequin

qed777 mannequin commented Sep 21, 2009

comment:13

I should add that I fixed the doctest failure in cell.py. I've set this ticket to WPNR.

@qed777 qed777 mannequin added the s: needs review label Sep 21, 2009
@jasongrout
Copy link
Member Author

comment:14

mvngu: can you apply the patch now and see if the doctest passes? If so, then should the positive review above become active again?

@jasongrout
Copy link
Member Author

comment:15

positive review on mpatel's rebasing and fixing the doctest (it now works), so I'm putting it back to positive review.

Minh--if there's something wrong with me positive reviewing mpatel's changes, please let me know.

@sagetrac-mvngu
Copy link
Mannequin

sagetrac-mvngu mannequin commented Sep 22, 2009

comment:16

Replying to @jasongrout:

Minh--if there's something wrong with me positive reviewing mpatel's changes, please let me know.

I don't see anything wrong, as long as you don't review your own changes and make it positive review. In this case, I see that you reviewed someone else's (mpatel) changes which in this case is a rebase. You never know; rebasing a patch can actually cause unexpected problems. Thank you for your work!

Merged patches in this order:

  1. #6865
  2. trac-6939-notebook-css-overflow.2.patch

@sagetrac-mvngu
Copy link
Mannequin

sagetrac-mvngu mannequin commented Sep 22, 2009

Reviewer: Karl-Dieter Crisman, Mitesh Patel, Jason Grout, Minh Van Nguyen

@sagetrac-mvngu sagetrac-mvngu mannequin closed this as completed Sep 22, 2009
@sagetrac-mvngu
Copy link
Mannequin

sagetrac-mvngu mannequin commented Sep 22, 2009

Merged: Sage 4.1.2.alpha3

@sagetrac-mvngu
Copy link
Mannequin

sagetrac-mvngu mannequin commented Sep 27, 2009

comment:18

There is no 4.1.2.alpha3. Sage 4.1.2.alpha3 was William Stein's release for working on the making the notebook a standalone package.

@sagetrac-mvngu
Copy link
Mannequin

sagetrac-mvngu mannequin commented Sep 27, 2009

Changed merged from Sage 4.1.2.alpha3 to Sage 4.1.2.alpha4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants