oka@, should QuickView support .html file?

I guess here we should show the html file as a text, instead of rendering the page inside quick view, which I believe the current behavior.
What do you think?

Because associated css file and so on are not downloaded together, the rendered page is incomplete anyways. So showing the file as a text is better IMO.

+weifangsun@ to input the ideal behavior from UX perspective.

IIUC styles can be embedded in mhtml file so we can preserve the layout etc... to some extent.
Showing the file as a text also makes sense to me, but I can not see the text when I open mhtml file in quick view.

Just to confirm - We should currently able to display .html files as text in Quick View.

For .mhtml files - Out of curiosity, are the external resources for the page downloaded/cached? I tested opening the .mhtml file offline and the full page is still able to load in a browser so I would assume we'd be able to provide a preview for it with the resources intact?

Marianne, could you take a look?

Sorry. It looks harder than I thought. (I'll examine the bugs to assign carefully going forward). You may want to work on other bugs.
I think we should create our own text content viewer for html following the construct of files_safe_img_webview_content.html and its companions. Here is the rationale.

We let .txt file to be shown by Chrome, simply setting its url to <webview>’s src.
1. FileEntry entry = getSelectedEntry()
2. entry.file() -> file
3. createObjectURL(file) -> url
4. webview.src = url

IIUC, (2) determines the mime type (file.type) merely from the file extension.
If we were able to change the file.type in (2) before passing it to createObjectURL, Chrome should render the file as a text. But file.type is readonly. Also I didn't find a way to override mime type over webview.
With that I think we need a customized page to render text, samely as we have a customized page to show an image within <webview>.

Started to look into it.

First thought: use FileReader instead of FileEntry.file() to read the file: this allows reading the file as binary/text file.

I think reading arbitrary file in main process is not good. Security things aside, it may cause crash/hang on reading a big file.

How about reading the file inside webview? It should be possible using XMLHttpRequest.
https://www.reddit.com/r/javascript/comments/1d9qun/help_want_to_display_content_of_txt_file_in_html/

Actually, I had already started working on the webview and I only need 2 more tweaks I need to debug, so the CL should be in review by EOD today :)

I should have updated the bug.^^

(XMLHttpRequest should not be necessary, when using the <script> tag with type="text/plain", it should be possible to pass any file as src attribute and force displaying it as plain text)

Current state: HTML is being rendered as text file!
Current issues:
* rendering is kind of ugly because of CSS issues - fukino@ is helping to look into it
* clear text to prevent viewing e. g. images as text when rendering takes too long
* .mhtml files have the same mime type as .html files - for now, this is probably better than having a blank white page being rendered, but not as good as actually rendering it

weifangsun@: I would rather keep the "render MHTML as plain text instead of rendering a blank image" until a better solution for MHTML has been found, what do you think?

mariannet@ - Agreed :) Rending as plain text is better than the blank image until we have a better solution for mHTML.

Update:

huge thanks to fukino@ for helping me with the styling issue.

weifangsun@, another question. What should the default behavior be when the text file is really wide? I have added scroll bars for big text files, but the left and right arrow key that are usually used to move between different files in Quickview will be used for scrolling in the case of very long lines (as in scroll left and right). I haven't been able to grab the slider with the mouse, so with the mouse it is probably only possible to scroll via selecting text, it shouldn't be an issue for touch. What do you think?

mcirimele@, I guess that question is also of interest to you - what do you think?

An idea to solve this issue might be to wrap lines, but I'm not sure which alternative is the better approach.

mariannet@ Let's not allow horizontal scrolling for the reasons you mentioned. I just put together a proposal for sizing previews in Quick view so we can decrease the padding. You can check it out in issue 

I think we should follow the same rules which means showing the whole doc in the preview (could be one page at a time). That leaves two options.

1. We zoom out so the full width fits. This would make the text very small for very wide docs. 
2. We set a default width and wrap the text to fit. 

I can make a call here but it actually depends on what will be the most important to see in these type of files to make them recognizable and distinguish them from a different file. Is it more important to see the structure of the code like paragraphs and relationships between long and short lines, or is it more important that you can read the text?

Remember, the preview will likely be used to help identify a file when the name isn't enough, like if you had multiple index.html for example and were looking for a particular one. 

You all in the dev team look more at code than I do, would be interested in hearing your thoughts :)

For a basic text file, I believe that the default behavior is to wrap to the size of the window, correct? I agree with Maria that we should not allow horizontal scrolling. My proposal for solution would be set a default size for the text file and wrap the lines.

Yes. we let Chrome to render text files, and Chrome wraps lines on showing
text files.
So +1 to wrapping.

Here are the screenshots of the current state - both the autogenerated CSS by webview for text files, and my attempts to mimic that for HTML files.

Deleted: quick_view_html.png 251 KB

	quick_view_html.png 251 KB View Download

Deleted: quick_view_text.png 97.8 KB

	quick_view_text.png 97.8 KB View Download

Looks fine to me, thanks for adding the screenshots. mariannet@ do we have the ability to set the document margin size? It could help with readability to increase the margins. 

It's not a blocker or anything, just a nice-to-have.

How much margin will be preferred?

BTW I guess we can re-use the style for other code-like files, e.g. *.css, *.js, *.h, *.cc etc. What do you think Maria?

Yes, reusing the style for other similar files would be great. Let's do 32dp (1x) on all sides. It would look something like the attached screenshot. 

Full disclosure: I cheated in the mock, the text should not resize of course. 

Deleted: quick-view-txt.png 138 KB

	quick-view-txt.png 138 KB View Download

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7ff332470340943fad856cf758eba00f4d3c1c4f

commit 7ff332470340943fad856cf758eba00f4d3c1c4f
Author: mariannet <mariannet@google.com>
Date: Wed Oct 11 05:30:11 2017

Display HTML files like text files.

When HTML files get saved, they don't have CSS attached to them, resulting in
ugly rendering inside QuickView. Treat them like text files.

Started playing with webview

Bug:  742944 
Test: manually
Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation
Change-Id: Ifcab81b7bf33b286b25c00e8c1f3d3a17d7fa7ef
Reviewed-on: https://chromium-review.googlesource.com/691395
Commit-Queue: Marianne Thieffry <mariannet@google.com>
Reviewed-by: Naoki Fukino <fukino@chromium.org>
Reviewed-by: Keigo Oka <oka@chromium.org>
Cr-Commit-Position: refs/heads/master@{#507901}
[modify] https://crrev.com/7ff332470340943fad856cf758eba00f4d3c1c4f/ui/file_manager/file_manager/foreground/elements/files_quick_view.html
[modify] https://crrev.com/7ff332470340943fad856cf758eba00f4d3c1c4f/ui/file_manager/file_manager/foreground/elements/files_quick_view.js
[add] https://crrev.com/7ff332470340943fad856cf758eba00f4d3c1c4f/ui/file_manager/file_manager/foreground/elements/files_safe_html_webview_content.css
[add] https://crrev.com/7ff332470340943fad856cf758eba00f4d3c1c4f/ui/file_manager/file_manager/foreground/elements/files_safe_html_webview_content.html
[modify] https://crrev.com/7ff332470340943fad856cf758eba00f4d3c1c4f/ui/file_manager/file_manager/foreground/elements/files_safe_media.js
[modify] https://crrev.com/7ff332470340943fad856cf758eba00f4d3c1c4f/ui/file_manager/file_manager/foreground/elements/files_safe_media_webview_content.js
[modify] https://crrev.com/7ff332470340943fad856cf758eba00f4d3c1c4f/ui/file_manager/file_manager/foreground/js/quick_view_controller.js
[modify] https://crrev.com/7ff332470340943fad856cf758eba00f4d3c1c4f/ui/file_manager/file_manager/manifest.json
[modify] https://crrev.com/7ff332470340943fad856cf758eba00f4d3c1c4f/ui/file_manager/file_manager_resources.grd

TODOs: Apply the suggested margin, and actually render mHTML

Small follow-up on the margin (I had completely misunderstood that first, sorry for not noticing earlier):
Would that be only for the html files rendered as text or also for normal text files?

If it should also apply to normal text files (else it would be inconsistent), the change will be slightly bigger (not just CSS but include custom rendering for text files).

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/08cfd25ee07ff2110e03b76481e484dd3f3f9476

commit 08cfd25ee07ff2110e03b76481e484dd3f3f9476
Author: mariannet <mariannet@google.com>
Date: Tue Oct 17 06:35:43 2017

Increase the margin when viewing text files in FilesApp QuickView.

Add more margin to quick viewing text files in order to improve readability.

Bug:  742944 
Test: manually
Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation
Change-Id: Icc25905d01eba4e266b949ddde075d10e1ad93ed
Reviewed-on: https://chromium-review.googlesource.com/720656
Reviewed-by: Keigo Oka <oka@chromium.org>
Commit-Queue: Marianne Thieffry <mariannet@google.com>
Cr-Commit-Position: refs/heads/master@{#509305}
[modify] https://crrev.com/08cfd25ee07ff2110e03b76481e484dd3f3f9476/ui/file_manager/file_manager/foreground/elements/compiled_resources2.gyp
[modify] https://crrev.com/08cfd25ee07ff2110e03b76481e484dd3f3f9476/ui/file_manager/file_manager/foreground/elements/files_quick_view.html
[modify] https://crrev.com/08cfd25ee07ff2110e03b76481e484dd3f3f9476/ui/file_manager/file_manager/foreground/elements/files_quick_view.js
[modify] https://crrev.com/08cfd25ee07ff2110e03b76481e484dd3f3f9476/ui/file_manager/file_manager/foreground/elements/files_safe_media.js
[rename] https://crrev.com/08cfd25ee07ff2110e03b76481e484dd3f3f9476/ui/file_manager/file_manager/foreground/elements/files_safe_text_webview_content.css
[rename] https://crrev.com/08cfd25ee07ff2110e03b76481e484dd3f3f9476/ui/file_manager/file_manager/foreground/elements/files_safe_text_webview_content.html
[modify] https://crrev.com/08cfd25ee07ff2110e03b76481e484dd3f3f9476/ui/file_manager/file_manager/manifest.json
[modify] https://crrev.com/08cfd25ee07ff2110e03b76481e484dd3f3f9476/ui/file_manager/file_manager_resources.grd

I have submitted a Cl that introduces consistent behavior (i. e. the same margin for both normal text files and HTML files).

mcirimele@: once you have confirmed the desired layout, I will adjust the code or close the bug.

Great, thanks. If you want you can add a screenshot to the bug, or I'll take a look when it has landed in canary.

Screenshot attached

Deleted: text-margin.png 255 KB

	text-margin.png 255 KB View Download

Looks good, thanks mariannet@ !