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

Responsive mail iframe #3904

Merged
merged 1 commit into from
Nov 6, 2020

Conversation

st3iny
Copy link
Member

@st3iny st3iny commented Oct 28, 2020

This is highly experimental but works well. I discovered a js library (iframe-resizer) which enables responsive iframes. The client script is injected into the message html response with a nonce attached so that it is executable.

As a nice side effect we can apply our custom scroll bars to the message container (via max-height: 50vh).

Feel free to try it out and provide feedback. Is this too big of a security risk?

responsive-iframe

@brueckner
Copy link
Collaborator

Interesting, good job! So first off, I think this is a pretty big quality-of-life improvement, since any mail would always take up 50vh regardless of its' content, which is especially problematic for very short mails in a threaded view. I tried your solution and it works for me.

I was actually looking into the same nonce-approach recently, just without the external iframe-resizer library (which seems to work pretty good!). I am not sure about the security implications of using nonce here either though.

Did you look into a hash-based policy by any chance?
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/frame-src

<hash-algorithm>-<base64-value>

A sha256, sha384 or sha512 hash of scripts or styles. The use of this source consists of two portions separated by a dash: the encryption algorithm used to create the hash and the base64-encoded hash of the script or style.

So, for a hash-based policy, one would create a hash of the script, attach it to the script-src attribute and this would effectively whitelist that exact script and nothing else. However, I think that would require changes to the ContentSecurityPolicy class of Nextcloud Server, unless I missed something. Maybe anyone else here has more experience with the security implications of either approach.

@st3iny
Copy link
Member Author

st3iny commented Nov 2, 2020

@brueckner I agree. Using a hash based policy is the most secure approach. The nextcloud server allows us to customize CSPs of responses (which we already do for the message html response). However, keeping track of the injected js bundle and rehashing it on changes would require potentially large changes to the code.

For now I think a nonce based script CSP is sufficient. The generated nonce is only valid for one request/response so guessing it is kind of impossible.

@st3iny st3iny force-pushed the enhancement/2870/responive-mail-iframe branch 4 times, most recently from b0cb8cd to 8708120 Compare November 2, 2020 14:30
@brueckner
Copy link
Collaborator

Thanks for the clarification! I am pretty new to the codebase, so I wasn't sure about it. So, again: I've got my fingers crossed that this gets merged :)

@st3iny
Copy link
Member Author

st3iny commented Nov 2, 2020

Now, that we inject a js bundle into the message html we can do some fancy stuff. The rendered body will inherit the foreground (text) color from the mail app. E.g. the dark theme now propagates to the displayed message which improves readability.

mail-iframe-theming


Additionally, I implemented some design changes to improve the scrolling experience. I replaced the gradient below the thread header with a thin border and moved the scroll bar next to the thread envelopes. In my opinion we don't need separate scroll bars for individual thread envelopes anymore since the entire thread can be scrolled now (scrap previously mentioned max-height: 50vh).

mail-iframe-design-changes

Those changes are up to discussion. If they are not welcome feel free to tell me and I will revert them so that we can at least get the responsive iframe merged :)

cc @jancborchardt

@brueckner
Copy link
Collaborator

Now, that we inject a js bundle into the message html we can do some fancy stuff. The rendered body will inherit the foreground (text) color from the mail app. E.g. the dark theme now propagates to the displayed message which improves readability.

That's pretty cool! I guess that would need to be tested with a couple of real-world-examples (newsletters etc.). I'll give it a spin with a couple of actual mails and will report back :)

In my opinion we don't need separate scroll bars for individual thread envelopes anymore since the entire thread can be scrolled now (scrap previously mentioned max-height: 50vh).

The problem is: if you have a lot of quoted content in a mail (which is fairly common in a thread, if everyone replies properly), you'll have to scroll a whole lot, even though the actual content of the mail is just a one-liner.

I've been thinking about that exact problem for a while and I believe the only way to solve this properly is a combination of:

  • (a) your solution in this PR,
  • (b) removing the max-height: 50vh and
  • (c) detecting quoted content in a mail

(c) is a hell of a task for HTML emails, because every mail client seems to be doing its' own thing. There are a bunch of solutions out there for doing it with plaintext, but I didn't really find anything suitable for HTML emails. It's not only different markup, it's a problem of different languages as well. Some libraries try to catch text-lines above quoted mails (like "On 2nd Nov 2020, Johannes wrote:", but that's obviously gonna be different for every language.

Apple Mail does a very good job at this:

That makes it super easy to focus on the latest messages and enables you to scroll through a thread of mails without having to scroll past quoted text.

What I am trying to say is: I would strongly recommend to separate that from your solution in this PR and reverting back to the max-height: 50vh for now, so that this PR can be merged. (c) should then be converted into a separate issue, with the final touch being (b). As soon as Nextcloud Mail is able to properly detect replied/quoted text, the max-height limitation can be dropped imho.

@st3iny
Copy link
Member Author

st3iny commented Nov 3, 2020

Thanks for your feedback :)

(c) detecting quoted content in a mail

Christoph created #3640 to address this. Sadly, there is no defined standard for quoted content in html mails :(

Until someone implements the collapse feature we can keep the max-height style. Without it the problem gets even worse the further you scroll down (quoted content gets longer and longer with each envelope).

Fun fact: The problem is so hard to solve that Google patented their solution.

@st3iny st3iny force-pushed the enhancement/2870/responive-mail-iframe branch 2 times, most recently from 25540ec to a39d055 Compare November 5, 2020 11:10
@st3iny
Copy link
Member Author

st3iny commented Nov 5, 2020

I implemented the proposed changes from @brueckner and fixed some minor things. If a thread contains a single message this message will expand to the full page height. Otherwise the thread envelope message is limited to max-height: 50vh.

@st3iny st3iny force-pushed the enhancement/2870/responive-mail-iframe branch from a39d055 to 2058279 Compare November 5, 2020 11:17
@st3iny st3iny marked this pull request as ready for review November 5, 2020 11:37
@ChristophWurst ChristophWurst added this to the v1.7 milestone Nov 5, 2020
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Rebased locally to get #4003 in and it's looking REALLY good. Opened a long HTML email (order confirmation of a shop) and the rendering looks flawless.

A few things, though

  • Regression: second column (envelope list) got narrower. This looks weird on a 1920px wide window
  • Let's revert the bottom border of the thread header
  • Let's revert the whitespace changes around the thread header

Like generally let's try to keep the changes to just the height fixes of the iframe. Any other change needs discussion with @jancborchardt.

Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Following up with a quick code review

Not much to argue with. I squinted on the on the css changes, though. I'll let @GretaD review that :)

src/components/MessageHTMLBody.vue Outdated Show resolved Hide resolved
@StCyr
Copy link
Collaborator

StCyr commented Nov 6, 2020

* Regression: second column (envelope list) got narrower. This looks weird on a 1920px wide window

Yes, I've that regression too:

image

@StCyr
Copy link
Collaborator

StCyr commented Nov 6, 2020

This is what I get in Firefox responsive mode for iPad:

image

Maybe worth investigating a bit more on a real iPad/tablet?

@st3iny st3iny force-pushed the enhancement/2870/responive-mail-iframe branch from 2058279 to f3ad85e Compare November 6, 2020 12:21
Signed-off-by: Richard Steinmetz <richard@steinmetz.cloud>
@st3iny st3iny force-pushed the enhancement/2870/responive-mail-iframe branch from f3ad85e to 237ce80 Compare November 6, 2020 12:22
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Works as expected now 👍

Good stuff :)

@ChristophWurst
Copy link
Member

@StCyr please test again. I think your issues are resolved as well.

@st3iny
Copy link
Member Author

st3iny commented Nov 6, 2020

Thanks for all the feedback and testing ❤️

@st3iny
Copy link
Member Author

st3iny commented Nov 6, 2020

@StCyr The overlapping of the sidebar is happening on master too (if that is what you were referring to). I thought that this is intended.

@ChristophWurst ChristophWurst merged commit 6fa1ea6 into master Nov 6, 2020
@ChristophWurst ChristophWurst deleted the enhancement/2870/responive-mail-iframe branch November 6, 2020 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants