Adds edit and view buttons #1905

gnestor · 2016-11-17T22:24:35Z

Continuation of #1755

gnestor · 2016-11-17T22:26:23Z

@takluyver It's working for me. However, on Chrome, the user must enable pop-ups for the domain, otherwise only the first file in the selection will be opened and a pop-up warning will display in the address bar.

takluyver · 2016-11-18T00:06:25Z

I think that's what Min expected on #1755. I'm inclined to make it a single-item action so that we're not dealing with browser pop-up blockers.

gnestor · 2016-11-18T05:08:58Z

@minrk @Carreau What say you? If we decide that "View" (viewing a notebook opens the JSON file in a separate tab) and "Edit" (editing a notebook opens the JSON file in a text editor) should only work for single files vs. multiple files, then no action is required and this PR can be closed.

minrk · 2016-11-18T09:16:49Z

@gnestor I think this is still useful. I'd just set the limit at 1, so that it doesn't run into pop-up blockers.

minrk · 2016-11-18T09:17:03Z

notebook/static/tree/js/notebooklist.js

@@ -595,6 +597,20 @@ define([
            $('.delete-button').css('display', 'none');
        }

+        // View is visible when an item is renderable or downloadable
+        if (selected.length > 0 && !has_directory) {


selected.length === 1 here, being what I mean

I'd say let the popup blockers do their job, and let the user manage that, rather than making it harder to open many files at once. When I am grading (not with nbgrader) selecting all, and View saves me a lot of time. Of course, you can only do this with class sizes less than about 25, but that is where I live anyway.

@dsblank I'm just curious: How do you use "View" and "Edit" for grading? I assumed "View" would open the notebook, but it opens the raw JSON file in the browser and "Edit" opens the JSON file in a text editor...

Sometimes, I just want to open the notebooks as notebooks (with kernel), be able to run, view, and print them. That was my intention by adding these buttons for the group selection operations.

Ok, then I believe the expected behavior for "View" is to open notebooks as a notebook vs. raw file (see my screencap below for an example). Given that, we can remove "View" entirely because viewing and editing any file other than a notebook is the same (e.g. open it in a text editor) and editing a notebook is to open it.

What about HTML files?

Ah, good point! HTML files seem like the one file type that should provide a "View" button. I'll try that now...

Great... that looks useful! What about all other types that one can view in the browser, such image files (jpg, gif)? Could there be a way to register editors for particular type of files?

I think the primary purpose of this PR is to allow "View" and "Edit" on multiple files at once. If we only allowed these button for single file selections, then there would be no need for "Edit" as it has the same effect of clicking on a file, and we are realizing that "View" is only needed for HTML and image files...so if we remove this for multiple files, we are left with only "View" for a handful of file types.

gnestor · 2016-11-18T15:44:01Z

Current "View" behavior:

Current "Edit" behavior:

gnestor · 2016-11-18T16:03:59Z

"Edit" notebook and markdown files:

"View" HTML file:

Carreau · 2016-11-18T16:48:44Z

Sweet ! +1

minrk · 2016-11-18T16:49:18Z

Nice!

Carreau · 2016-11-18T16:49:40Z

Looking forward to have an occasion to use that !

gnestor · 2016-11-18T17:45:29Z

notebook/static/tree/js/notebooklist.js

@@ -595,6 +597,22 @@ define([
            $('.delete-button').css('display', 'none');
        }

+        // View is visible when an item is renderable or downloadable
+        if (selected.length > 0 && !has_directory && selected.every(function(el) {
+            return el.path.match(/html?|jpe?g|png|gif|tiff?|svg|bmp|ico|pdf|doc|xls/);


@dsblank I updated the "View" condition to a regex of common browser-renderable file types. I could create an API method on NotebookList to register additional file types, but it would probably be easier to just decide on them now...

@gnestor Thank you... this is very good for this version. Later, when there is a full API, I agree that being able to register mime-types and specific viewers/editors would be quote useful. This is great!

You're welcome! If you take a look at JupyterLab and specifically https://github.com/jupyterlab/jupyterlab_json, you will see that lab allows for extensions to both render output by mime type and render files by extension, so there is a lot opportunity to create custom file handlers.

takluyver · 2016-11-18T18:35:33Z

I think it might be a bit surprising for new users that 'View' and 'Edit' on a notebook both show you the raw JSON. I understand that that fits in with editing other text-based file types, but I would naively expect 'edit' on a notebook to be equivalent to opening it normally, and 'view' to be equivalent to the nbconvert-based 'print preview' we offer from the file menu.

How hard would it be to make the buttons say something like 'View raw' and 'Edit raw' if you have a notebook selected?

gnestor · 2016-11-18T20:08:32Z

@takluyver I've revised so that "View" is only visible for HTML, image, and PDF files and "Edit" will open notebooks as notebooks and all other files in the text editor.

gnestor · 2016-11-19T00:10:55Z

One more use case to consider: What if you wanted to look at or edit the JSON of a notebook file? There is currently no way to open a notebook in a text editor. Do we want to accommodate that with an "Edit Raw" or "Edit Metatdata" for selected notebook files?

dsblank · 2016-11-19T01:18:34Z

I don't think we need a button for that.... Maybe be able to open from editor would be enough?

takluyver · 2016-11-19T12:37:49Z

Thanks @gnestor, that sounds good to me. I don't think there's a pressing need for 'edit raw'.

I'd still probably agree with Min that I'd only show the buttons with a single file selected, and avoid popup blockers altogether. I think the main thing we wanted to fix is that there's no easy way to view an HTML file, because clicking on it opens an editor. I don't feel strongly about this, though.

We should think about the security implementations. An HTML file could contain Javascript, which can execute when you 'view' it. Because it's served from the notebook domain, that Javascript can presumably start a kernel and use it to execute code (unless I've overlooked something). That seems a bit unexpected for a 'view' action. Is there any way we can mitigate this? Ping @rgbkrk

rgbkrk · 2016-11-19T19:08:23Z

We should think about the security implementations. An HTML file could contain Javascript, which can execute when you 'view' it. Because it's served from the notebook domain, that Javascript can presumably start a kernel and use it to execute code

That's exactly correct. If the view needs to be supported it needs to be in a sandboxed iframe (since we don't have access to other subdomains).

dsblank · 2016-11-19T20:07:06Z

As for showing the buttons when there is only 1 vs. more selected, often times the better option doesn't make itself clear until you try to write the documentation (or teach someone how to use it). If you start describing how to use this, it becomes complicated... buttons showing/not showing because different combinations of types (files, directories) and counts (one, two) are being selected. If you can make an option more flexible and easier to explain, then go with that.

takluyver · 2016-11-19T20:07:26Z

Aha, so we would use that to disable JS scripts entirely for the HTML you're viewing?

I'm going to bump this PR to milestone 5.0 as it seems like a bigger thing to work out, and I'd like to get 4.3 released fairly soon.

rgbkrk · 2016-11-19T21:01:33Z

I'm going to bump this PR to milestone 5.0 as it seems like a bigger thing to work out, and I'd like to get 4.3 released fairly soon.

I certainly agree with that.

rgbkrk · 2016-11-19T21:13:18Z

It's not that we need to disable JS so much as we want to disable access to the Jupyter REST APIs and websockets.

<iframe sandbox="allow-scripts" src="test.html">
</iframe>

However, you can disable JS by omitting the allow-scripts set in the sandbox options:

<iframe sandbox src="test.html">
</iframe>

takluyver · 2016-11-19T22:15:33Z

Ah, I see, a sandboxed iframe has a special origin, so any attempt to access the Jupyter REST APIs would be blocked by the cross-origin protection. Thanks, I didn't know about that :-)

rgbkrk · 2016-11-20T01:30:41Z

Another security option that is available includes disabling scripts on specific paths with a more stringent content security policy.

takluyver · 2016-11-20T11:03:43Z

I think that the general 'treat this as a different origin' security measure should do the trick.

@gnestor does this make sense? I imagine we will need a separate handler at something like /viewhtml/, which renders pages something like:

<!-- /viewhtml/foo.html -->
<iframe sandbox="allow-scripts" src="/files/foo.html">
</iframe>

And viewing an HTML file would take you to this page instead of the /files/ URL. I don't see any way around serving the HTML insecurely at /files/, but at least we can make it a bit harder to get there.

gnestor · 2016-12-15T04:01:38Z

@takluyver Makes sense. Thanks!

@rgbkrk Could we modify the CSP in lieu of redirecting to a page with an iframe? If so, that seems like a simpler solution...

@minrk I see that you created the FileRedirectHandler, how would you go about disallowing HTML files from accessing the Jupyter REST APIs? If we went with iframe approach, would it make sense to create a file.html template and do something like:

self.render_template('file.html', source='/files/foo.html')

minrk · 2016-12-15T10:38:50Z

how would you go about disallowing HTML files from accessing the Jupyter REST APIs?

I think IFrames are the only way to do this. I'm not sure of the details, though. A properly configured IFrame should prevent access to the API via existing CORS, etc.

gnestor · 2016-12-15T16:03:42Z

@minrk Ok, then we'll take that approach. Would you recommend creating a template and doing something like this in FilesRedirectHandler:

self.write(self.render_template('file.html', source='/files/foo.html'))

minrk · 2016-12-15T17:34:17Z

Yeah, a template for the iframe page sounds quite sensible.

takluyver · 2017-01-13T21:21:39Z

Ping - are we ready to implement this with the iframe for 5.0?

gnestor · 2017-01-14T00:15:25Z

@takluyver Yes, I will do this next. Thanks for the reminder 👌

ivanov

I would like to see some tests of the functionality provided here before merging it. Adding new features without tests makes for future sad panda developers and users.

gnestor · 2017-01-25T05:38:58Z

Update:

Edit button: Edit functionality is currently broken in master, I rebased to declare contents variable #2073 and it's working in the PR now (thanks @minrk)
View button: HTML files use the /view/filename.html route which loads the files inside of an iframe to prevent access to the Jupyter API via existing CORS
I added a test for the new view route

I would appreciate any feedback regarding tests and UX before merging.

takluyver · 2017-01-25T12:09:50Z

notebook/static/tree/js/notebooklist.js

+        }
+
+        // Edit is visible when an item is editable
+        if (selected.length > 0 && !has_directory) {


Do we want to whitelist/blacklist files for editability? E.g. you can't open a png in the text editor.

We could use a regex pattern like above to include/exclude specific extensions. I just opened a PNG in the text editor and it's pretty useless and harmless, so I'm neutral.

That was what I was thinking. :-) I'd probably go for a blacklist. Maybe there are too many types to be worth trying to list them all, though. Do we have mimetypes here? Maybe we could block edit on image/*, with an exception for image/svg+xml.

* Edit notebook opens a notebook, Edit any other file type open it in a text editor * View only available for HTML files to disambiguate between “viewing” a notebook or text file (which is the same as editing it) and viewing an HTML file

takluyver · 2017-01-30T14:04:38Z

OK, let's land this, we can refine things later.

gnestor added the enhancement label Nov 17, 2016

gnestor added this to the 4.3 milestone Nov 17, 2016

minrk reviewed Nov 18, 2016

View reviewed changes

minrk approved these changes Nov 18, 2016

View reviewed changes

gnestor force-pushed the pr/1755 branch from 9b6992f to afa9c35 Compare November 18, 2016 17:42

gnestor commented Nov 18, 2016

View reviewed changes

takluyver mentioned this pull request Nov 19, 2016

Adds edit and view buttons, #1752 #1755

Closed

takluyver modified the milestones: 5.0, 4.3 Nov 19, 2016

takluyver added the tag:Security label Nov 19, 2016

takluyver mentioned this pull request Nov 20, 2016

separate edit and view links #1752

Closed

ivanov requested changes Jan 21, 2017

View reviewed changes

gnestor force-pushed the pr/1755 branch from afa9c35 to 07b6da4 Compare January 25, 2017 03:59

takluyver approved these changes Jan 25, 2017

View reviewed changes

Doug Blank and others added 7 commits January 25, 2017 13:46

Adds edit and view buttons, jupyter#1752

482ea4b

Revise jupyter#1755

a7387b8

Update View and Edit conditions

b7dcae6

* Edit notebook opens a notebook, Edit any other file type open it in a text editor * View only available for HTML files to disambiguate between “viewing” a notebook or text file (which is the same as editing it) and viewing an HTML file

Create /view route, handler, and template

0034f2a

Use /view route for HTML files

224294c

Add test_view_html

3cbb570

Clean up

acf4fa8

gnestor force-pushed the pr/1755 branch from e5dedde to acf4fa8 Compare January 25, 2017 21:47

takluyver merged commit 39b756b into jupyter:master Jan 30, 2017

gnestor deleted the pr/1755 branch January 30, 2017 23:34

github-actions bot added the status:resolved-locked label Apr 9, 2021

github-actions bot locked as resolved and limited conversation to collaborators Apr 9, 2021

Adds edit and view buttons #1905

Adds edit and view buttons #1905

Conversation

gnestor commented Nov 17, 2016 • edited by minrk Loading

gnestor commented Nov 17, 2016

takluyver commented Nov 18, 2016

gnestor commented Nov 18, 2016

minrk commented Nov 18, 2016

Choose a reason for hiding this comment

dsblank Nov 18, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnestor commented Nov 18, 2016 • edited Loading

gnestor commented Nov 18, 2016

Carreau commented Nov 18, 2016

minrk commented Nov 18, 2016

Carreau commented Nov 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

takluyver commented Nov 18, 2016

gnestor commented Nov 18, 2016

gnestor commented Nov 19, 2016

dsblank commented Nov 19, 2016

takluyver commented Nov 19, 2016

rgbkrk commented Nov 19, 2016

dsblank commented Nov 19, 2016

takluyver commented Nov 19, 2016

rgbkrk commented Nov 19, 2016

rgbkrk commented Nov 19, 2016

takluyver commented Nov 19, 2016

rgbkrk commented Nov 20, 2016 • edited Loading

takluyver commented Nov 20, 2016

gnestor commented Dec 15, 2016

minrk commented Dec 15, 2016

gnestor commented Dec 15, 2016

minrk commented Dec 15, 2016

takluyver commented Jan 13, 2017

gnestor commented Jan 14, 2017

ivanov left a comment

Choose a reason for hiding this comment

gnestor commented Jan 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

takluyver commented Jan 30, 2017

gnestor commented Nov 17, 2016 •

edited by minrk

Loading

dsblank Nov 18, 2016 •

edited

Loading

gnestor commented Nov 18, 2016 •

edited

Loading

rgbkrk commented Nov 20, 2016 •

edited

Loading

gnestor commented Jan 25, 2017 •

edited

Loading