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

add option to view full (html) message #1763

Merged
merged 19 commits into from
Jan 28, 2021
Merged

add option to view full (html) message #1763

merged 19 commits into from
Jan 28, 2021

Conversation

r10s
Copy link
Member

@r10s r10s commented Dec 31, 2020

this is the ui counterpart of deltachat/deltachat-core-rust#2125 - the pr adds a button "Show full message..." whenever core thinks, this is reasonable. a click on the button opens an html-view with the full message as returned by the core.

the full-message is even searchable :)

  • adapt to api
  • add button if core says so
  • show html-mails in webview
  • make mailto: links work
  • add option to downoad remote content, defaults of "never" for now


left: message with the new button, right: after tap on the button

EDIT: now, remote content is disabled by default and can be enabled in the menu:

related related: deltachat/deltachat-desktop#2078, deltachat/deltachat-ios#1046, deltachat/deltachat-core-rust#2125

@Hocuri
Copy link
Collaborator

Hocuri commented Dec 31, 2020

  • When fetching from remote servers, we have to think a lot about security, e.g. efail (on Heise) and I must say, it was a nice feeling to know that there is no way DC could be affected by such security problems

  • Is javascript enabled? (also, do newsletters even need javascript)

  • We now have 3 possible ways of showing a message:

    • plain-text, as we currently do it (of course we will still do this, but if start cropping more, the user might be forced to hit "show full message" even though there are now images at all)
    • html but fetching from a remote server disabled
    • html, fetching images from a remote server enabled

    It probably won't be possible UX-wise to always let the user choose between those, but what about this:

    • By default, disable all remote-fetching of images in full-message-view. But add a button to enable it. Then, directly enable it for all messages from this contact, but add a possibility to hide images again. Thunderbird writes "To protect your privacy, Thunderbird has blocked remote content in this message." and just adds a "Preferences" drop-down menu. K9 directly shows a "SHOW PICTURES" button.

@Simon-Laux
Copy link
Member

Is javascript enabled? (also, do newsletters even need javascript)

hopefully not, js is not available in email clients for a good reason.

html but fetching from a remote server disabled

remote fetching should be controllable by the user, like a load external content button and the option to trust certain domains / filetypes?

Also we should sanitize the html files even if we disable js in the webview there are probably still vulnerabilities. I would just allow a safe subset of html.

@r10s
Copy link
Member Author

r10s commented Jan 1, 2021

sanitizing html-files is planned, filtering out js stuff and maybe other things, this will be done together with adding embedded images which requires iterating over html anyway.
EDIT: see recent discussions at deltachat/deltachat-core-rust#2127

for loading remote content - we had several discussions over that, there are several possible approaches, yes.

@r10s
Copy link
Member Author

r10s commented Jan 2, 2021

Is javascript enabled? (also, do newsletters even need javascript)

no, javascript is not enabled at all. for subsequent discussions wrt sanitizing, please refer to deltachat/deltachat-core-rust#2127

@r10s r10s force-pushed the save-original branch 2 times, most recently from 3a1043c to 6b442af Compare January 24, 2021 22:29
@r10s r10s marked this pull request as ready for review January 24, 2021 22:32
@r10s
Copy link
Member Author

r10s commented Jan 24, 2021

remote content is now disabled by default, there is a menu option to load it once or always, see image atop.

i think, this is mergable now - also to get the strings to master and make them translatable for the other UIs. if needed, we can add a banner or so later on in a subsequent pr, however, for the first thing, the state is not that bad, esp. as there are not many options currently :) also, the subject is not yet shown (cmp deltachat/deltachat-core-rust#2138), but this is also for a subsequent pr.

@r10s r10s requested a review from Hocuri January 26, 2021 21:39
@Hocuri
Copy link
Collaborator

Hocuri commented Jan 27, 2021

First testing...

  • How hard would it be to allow zooming?
  • Currently the user has to hit "Load remote content" to show images. This will prevent users to accidentally give up their privacy and load images, but users might also not find this button. (just noted, I don't have a strong opinion on this)
  • +1 for blocking loading images via http (I first wondered why some images are not shown but images that come via http don't deserve to be shown)
  • The "Always" option means "Always for this sender", doesn't it?
  • it took me a while to understand that the check means "current selection"
    • For the last two points, maybe make radio buttons so that 1. we can write "Always for this sender" 2. it's clear how to communicate the currently-selected option? disadvantage: We need a custom view (OTOH, we have experience with custom views, so adding them will be faster :) )

@r10s r10s changed the title add option to access original message add option to view full (html) message Jan 27, 2021
@r10s
Copy link
Member Author

r10s commented Jan 27, 2021

How hard would it be to allow zooming?

probably not that hard, i've skimmed some options in WebSettings, but this is for another pr then.

users might also not find this button.

we will see. it is one of two menu entries.

i also noted above that a banner in a subsequent pr might be nice, however, this would also need detection of request to remote content, not sure how hard that is and if it is really worth the effort.

The "Always" option means "Always for this sender", doesn't it?

no, "Always" means "Always" :) so, "Always for all HTML-messages".

at least for now, that should be sufficient. tbh, i do not think we really need this "always for this message", "always for this sender", "always for this domain" etc., i do not think that this bureaucracy is really helpful. also, as you usually have confirmed a contact request before and you always have to hit the "show full message" button. having that in mind, that the html-stuff is used for confirmed contacts only, the privacy thing is maybe not a huge one.

also, i do not want to have too high barriers for initial implementation for ios/desktop (see deltachat/deltachat-ios#1046)

but i would like to get this discussion out of this pr as well, the current options are already more than we initially wanted to do :)

android:layout_marginBottom="10dp"
android:paddingLeft="12dp"
android:paddingRight="12dp"
android:background="@drawable/conversation_item_update_background"
Copy link
Collaborator

Choose a reason for hiding this comment

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

The button somehow looks too... grey to me; the following looks better IMO but not great still:

Suggested change
android:background="@drawable/conversation_item_update_background"
android:background="@color/delta_primary"
android:textColor="@color/white"
tools:visibility="visible"

Copy link
Member Author

Choose a reason for hiding this comment

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

yip, the button could need some tweaking, however, also outgoing buttons need tweaking, and by just saying @color/delta_primary, the rounded corners are lost.

Copy link
Collaborator

Choose a reason for hiding this comment

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

For me it looks equally good with round and square corners, but yes, if we want to preserve rounded corners, this needs some more work.

What do you mean with "outgoing buttons"?

Copy link
Member Author

@r10s r10s Jan 28, 2021

Choose a reason for hiding this comment

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

What do you mean with "outgoing buttons"?

the ones defined in res/layout/conversation_item_sent.xml. we have two layout for the bubbles, one incoming, one outgoing. you suggestion would only change the incoming ones (on the left side).

however, to move forward, i'd like to put that layout discussion into another pr, current state is not bad (maybe in darkmode it is a bit dark :)

this makes it easier for the user to find the option
if one sees, images are missing in the document.
@r10s
Copy link
Member Author

r10s commented Jan 27, 2021

i've changed the wording from "Load remote content" to "Load remote images" - for the latter, it seems much easier for the user to understand the option as one sees that images are missing (the option in the menu, not visible on the screenshot, is also called Load remote images).

not sure, if it is really needed to point out that this also affects styles or other content, however, i've added that bit.

the second note is that user are not thinking that it is a bug, when they are seeing images if the option is disabled (i've seen that complain for other mua in some threads and forums)

@link2xt
Copy link
Contributor

link2xt commented Jan 27, 2021

Shorter the messages are easier to read and some users have small screens. Maybe "Remote images may be used to track you" for the first sentence?

Typical WYSIWYG users probably will not understand what "remote styles" are, unless they are familiar with CSS and the concept of separate styles. I would use "fonts and other content" instead.

Bullets are useless here, this is not a list. I would even merge it into a single paragraph: "The setting also allows loading fonts and other content. Even if it is disabled, you may still see embedded or cached images."

@r10s
Copy link
Member Author

r10s commented Jan 27, 2021

now it looks like that, much cleaner, thanks a lot!

EDIT: first "may" is replaced by "can", according to recent irc discussions, see following commit for reasoning.

Comment on lines +5 to +8
<item android:title="@string/search"
android:id="@+id/menu_search_web_view"
app:actionViewClass="androidx.appcompat.widget.SearchView"
app:showAsAction="collapseActionView|never" />
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nitpicking: Maybe move this to the end, as it's shown on the right? (I was a little confused first why the "0/0" text doesn't come first

Copy link
Member Author

@r10s r10s Jan 28, 2021

Choose a reason for hiding this comment

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

we are using the same order already at other places, so i would not mix that - and i do not want to change other places in scope of this pr.

i read it as menu_search_web_view introduces the sequence of needed search buttons :)

src/org/thoughtcrime/securesms/FullMsgActivity.java Outdated Show resolved Hide resolved
src/org/thoughtcrime/securesms/FullMsgActivity.java Outdated Show resolved Hide resolved
src/org/thoughtcrime/securesms/LocalHelpActivity.java Outdated Show resolved Hide resolved
@@ -282,7 +282,7 @@ public void openConversation(int chatId, int startingPosition) {
final ApplicationDcContext dcContext = DcHelper.getContext(this);
if (isForwarding(this) && dcContext.getChat(chatId).isSelfTalk()) {
SendRelayedMessageUtil.immediatelyRelay(this, chatId);
Toast.makeText(this, "✔️ " + getString(R.string.saved), Toast.LENGTH_SHORT).show();
Toast.makeText(this, DynamicTheme.getCheckmarkEmoji(this) + " " + getString(R.string.saved), Toast.LENGTH_SHORT).show();
Copy link
Member Author

Choose a reason for hiding this comment

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

this is not related to html-view, however, during review it turns out that the "✔️" emoji does not contrast nicely to dark-mode background-colors and we introduced a function therefore - and used it at places where "✔️" was already used before.

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

Successfully merging this pull request may close these issues.

4 participants