android-app icon indicating copy to clipboard operation
android-app copied to clipboard

Improve readability of wide content

Open azrdev opened this issue 6 years ago • 28 comments

Issue details

Duplicate?

Have you searched the issues of this repository if your issue is already known? yes

Actual behaviour

Look at content that is wider than the screen, e.g.:

Such content is not squeezed to fit into the screen width, instead one can scroll horizontally to see all of it. However, the scrolling is difficult because swiping to much to the left goes to the next article.

Expected behaviour

Either squeeze the content (which might look bad) or [offer to] disable going the the next article by swiping. Maybe there's some other solution, too.

Environment details

  • wallabag app version: 1.0.2 (106)
  • wallabag app installation source: f-droid
  • Android OS version: 7.1
  • Android ROM (e.g. stock, LineageOS, SlimRom,…): custom, LineageOS
  • Android hardware: golden / Galaxy S3mini
  • wallabag server version: 2.3.1

Your experience with wallabag Android app

Have you had any luck using wallabag Android app before?

Yes, works great, thank you!

azrdev avatar Sep 27 '18 08:09 azrdev

I have been wanting to implement a fix for this. I have noticed a general lack of responsiveness when dealing with the horizontal scrolling as I read a fair bit of articles with wide tables/code in them.

The lack of responsiveness in horizontal scrolling seems to come from the fact that there is a WebView nested within a ScrollView. To fix on the branch I have been playing around with, I have removed the bottom bar with the mark as read/unread and next/previous article buttons, and just have the WebView of the article as the primary scrolling view. This greatly improves the horizontal scrolling and allows moving diagonally as well.

So is the bottom bar really necessary? We have the mark as read/unread button on the top action bar already and next/previous article can be dealt with using gestures. IMO the bottom bar presents redundant functionality.

Regarding the swipe gestures, I believe double tap on the left/right for previous/next article to be a decent alternative that would probably result is less false positives.

What do the devs think? I have already implemented most of the current functionality without the nested scrollview, minus the bottom bar in my own branch. I just need to look into writing some code to enable smooth scrolling.

bezmi avatar Sep 30 '18 15:09 bezmi

@bezmi looks good to me, I personally don't use the bottom bar so I won't miss it (more a user opinion than a dev one).

Kdecherf avatar Sep 30 '18 17:09 Kdecherf

Please fix this. Any article showing code (with a <pre> tag) is unreadable, as you end up going to the next article. Being able to disable the swipe feature (which I personally never use) would be enough imo.

erdnaxeli avatar Nov 17 '18 11:11 erdnaxeli

You could maybe add an option to allow for the wrapping of code? It's perhaps easier?

techexo avatar Dec 02 '18 23:12 techexo

Hi, is there any new work on this issue? Reading quite a lot of articles containing code, it's almost unusable on Android because each time one try to swipe left to see content of a code excerpt, the previous article is charged... Wouldn't it be possible to force wrapping of long content?

techexo avatar Jul 23 '19 21:07 techexo

To rethink the idea of the author, would it be enough for everyone to add a setting to disable going to the next article by swiping to fix this issue? Because I could do this.

NWuensche avatar Aug 20 '19 09:08 NWuensche

I think, that even regardless of this issue, making the swiping optional is a good idea. Wide content should be addressed anyway though.

di72nn avatar Aug 20 '19 10:08 di72nn

I agree with di72nn, it can't hurt (and will probably be a workaround), but I think code wrapping is still a good idea to implement.

techexo avatar Aug 21 '19 03:08 techexo

As it seems that the display of content is managed with Android Webview, I'm wondering if altering the CSS to force-wrap pre elements wouldn't be enough.

Could someone competent with building the app be kind enough to test this CSS trick and see if code-containing articles (for example, the one explaining the trick!) would be better displayed?

techexo avatar Aug 21 '19 04:08 techexo

white-space: pre-wrap; does work, but I think a choice between pre-wrap and overflow: auto; should be added (currently there are neither). I'm not sure whether the swipe/fling feature is used at all, so I think these would be good defaults: swipe disabled, code blocks in overflow mode.

di72nn avatar Aug 21 '19 10:08 di72nn

Yup, I tested it (after understanding how to build an APK from source 😄) and adding

#article pre {
 white-space: pre-wrap;
}

in main.css does the job for me!

Should we make a PR just for that? I think it's really a minor tweak which goes a long way, but making an option to enable that in the settings is outside my skills 😉.

techexo avatar Aug 21 '19 10:08 techexo

@NWuensche when #808 is done, will you be willing to do optional swiping and that pre-wrap/overflow option (preferably as separate PRs)? Both already have similar existing features: the swiping thing should be implemented like "Tap-to-scroll" and the CSS thing as "serif" or "text alignment" options (however, I guess a ListPreference would be a better fit on the settings screen).

di72nn avatar Aug 21 '19 11:08 di72nn

I will try it out!

NWuensche avatar Aug 21 '19 13:08 NWuensche

@di72nn First request is ready. However what do you mean with the "serif" or "text alignment" option for the second request? I would have added the ListPreference with the options "none" xor "pre-wrap" xor "overflow". Or am I missing something?

NWuensche avatar Aug 22 '19 14:08 NWuensche

However what do you mean with the "serif" or "text alignment" option for the second request?

I only meant some implementation details like the way "serif" or "text alignment" is handled via additionalClasses in ReadArticleActivity.

I would have added the ListPreference

Right.

with the options "none" xor "pre-wrap" xor "overflow"

"none" == current == broken. I think "overflow" should be the default one with "pre-wrap" as the second option.

di72nn avatar Aug 22 '19 15:08 di72nn

Ok, but should this setting be added in a similar way as in #810 right now (i.e. with the *Changed flags) or not?

NWuensche avatar Aug 23 '19 18:08 NWuensche

For this setting and also the one in 810 there is no need to use the changed flags.

Strubbl avatar Aug 23 '19 19:08 Strubbl

It should be implemented very similarly to the "serif" option (except for ListPreference instead of a checkbox): you can literally copy-paste its implementation without extra additions.

di72nn avatar Aug 23 '19 22:08 di72nn

OK, I think I understand it now, thanks for explaining it again. However, I've noticed now that overflow is a separate option in itself. So what should it mean if the user wants overflow instead of pre-wrap? Which overflow option should be the one used? overflow: auto?

NWuensche avatar Aug 24 '19 08:08 NWuensche

We want to choose between overflow and pre-wrap. Default shall be overflow.

Yes, overflow: auto

Strubbl avatar Aug 24 '19 09:08 Strubbl

I think it should be something like adding:

.pre-overflow #article pre {
  overflow: auto;
}

.pre-prewrap #article pre {
  white-space: pre-wrap;
}

and then applying either one from the code. I'm not very familiar with CSS selectors, so don't be surprised if it needs correcting.

di72nn avatar Aug 24 '19 09:08 di72nn

Ok, so I tried this article: https://lkml.org/lkml/2018/9/16/167. Having light theme on and adding in main.css:

.pre-overflow #article pre {
  overflow: auto;
}

or

 .pre-prewrap #article pre {
   white-space: pre-wrap;
 }

or

#article pre {
 white-space: pre-wrap;
}

without any other changes does absolutely nothing for me. The text still goes far to the right. I am a total beginner with CSS, but from w3schools I thought that all of these three options should force the text to be wrapped to new lines.

NWuensche avatar Aug 24 '19 16:08 NWuensche

Unless I'm mistaken, the first two of your examples couldn't work (as there is a parent-child relationship expected between elements separated by spaces: the first one would expect a pre element included in anything with id="article", that one being also in an element with class="pre-overflow").

However, the last one should work just fine (at least from a CSS selector point-of-view). You might want to try also the properties overflow-wrap: anywhere; or overflow-wrap: break-word;, see if it fits better your expectations.

techexo avatar Aug 25 '19 08:08 techexo

And to complete my previous answer, the CSS selectors for a pre element of class="pre-prewrap" would be #article pre.pre-prewrap.

techexo avatar Aug 25 '19 08:08 techexo

If anyone is interested, here's how the article is formed in the app (it was like that for years, and it probably should be improved):

<html>
	<head>
		...
	</head>
	<div id="main">
		<body HERE_GO_CLASSES>
			<div id="content" class="w600p center">
				<div id="article">
					<header class="mbm">
						...
					</header>
					<article>
						HERE_GOES_CONTENT
					</article>
				</div>
			</div>
		</body>
	</div>
</html>

where HERE_GO_CLASSES is replaced with a list of additional classes (one of which is supposed to be pre-overflow for example) and HERE_GOES_CONTENT is the article content, with all the pres and stuff.

di72nn avatar Aug 25 '19 09:08 di72nn

Maybe there was a caching issue, because I couldn't reproduce my own results at first, but currently the situation is as follows.

There is

pre {
    white-space: pre-wrap;
}

in ratatouille.css which I reluctant to touch.

There is

#article pre {
  font-family: monospace;
  white-space: pre;
  text-justify: none;
}

in main.css. No changes here as well.

I added

.pre-overflow #article pre {
  overflow: auto;
}

.pre-prewrap #article pre {
  white-space: pre-wrap;
}

right after the previous #article pre {...} in main.css.

Now if I add pre-overflow or pre-prewrap to additionalClasses in ReadArticleActivity, the formatting seems to work as expected.

di72nn avatar Aug 25 '19 10:08 di72nn

I also found out that copying my third snippet to the end of the CSS file helps. Thanks to both of you for all of the help. I will try to upload the PR later today.

NWuensche avatar Aug 25 '19 11:08 NWuensche

So there is a PR out now. However, it does not handle large tables like https://de.wikipedia.org/wiki/Worte_des_Vorsitzenden_Mao_Tsetung#Struktur_und_Inhalt which was mentioned by the author. Can disabling swiping between articles from PR #810 be considered a solution for this or has anyone an idea on how to solve this?

NWuensche avatar Aug 25 '19 12:08 NWuensche