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

Improve readability of wide content #734

Open
azrdev opened this issue Sep 27, 2018 · 28 comments
Open

Improve readability of wide content #734

azrdev opened this issue Sep 27, 2018 · 28 comments

Comments

@azrdev
Copy link

azrdev commented Sep 27, 2018

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!

@bezmi
Copy link

bezmi commented Sep 30, 2018

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.

@Kdecherf
Copy link
Member

@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).

@erdnaxeli
Copy link

erdnaxeli commented Nov 17, 2018

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.

@techexo
Copy link
Contributor

techexo commented Dec 2, 2018

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

@techexo
Copy link
Contributor

techexo commented Jul 23, 2019

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?

@NWuensche
Copy link
Contributor

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.

@di72nn
Copy link
Member

di72nn commented Aug 20, 2019

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

@techexo
Copy link
Contributor

techexo commented Aug 21, 2019

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
Copy link
Contributor

techexo commented Aug 21, 2019

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?

@di72nn
Copy link
Member

di72nn commented Aug 21, 2019

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.

@techexo
Copy link
Contributor

techexo commented Aug 21, 2019

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 😉.

@di72nn
Copy link
Member

di72nn commented Aug 21, 2019

@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).

@NWuensche
Copy link
Contributor

I will try it out!

@NWuensche
Copy link
Contributor

@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?

@di72nn
Copy link
Member

di72nn commented Aug 22, 2019

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.

@NWuensche
Copy link
Contributor

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

@Strubbl
Copy link
Contributor

Strubbl commented Aug 23, 2019

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

@di72nn
Copy link
Member

di72nn commented Aug 23, 2019

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.

@NWuensche
Copy link
Contributor

NWuensche commented Aug 24, 2019

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?

@Strubbl
Copy link
Contributor

Strubbl commented Aug 24, 2019

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

Yes, overflow: auto

@di72nn
Copy link
Member

di72nn commented Aug 24, 2019

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.

@NWuensche
Copy link
Contributor

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.

@techexo
Copy link
Contributor

techexo commented Aug 25, 2019

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
Copy link
Contributor

techexo commented Aug 25, 2019

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

@di72nn
Copy link
Member

di72nn commented Aug 25, 2019

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
Copy link
Member

di72nn commented Aug 25, 2019

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.

@NWuensche
Copy link
Contributor

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
Copy link
Contributor

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?

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

No branches or pull requests

8 participants