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

Regression: Margins aren't big enough on desktop monitors #101

Closed
ryanackley opened this issue Jul 25, 2014 · 11 comments
Closed

Regression: Margins aren't big enough on desktop monitors #101

ryanackley opened this issue Jul 25, 2014 · 11 comments

Comments

@ryanackley
Copy link
Contributor

I noticed this while getting the automated tests running. Commit a40d84f causes a regression

See http://idpf.org/forum/topic-1401

Basically having fixed size margins is a problem on larger screens because it makes the text hard to read.

Therefore, the margin settings didn't actually set the margin size but the content size based on a ratio of the window size. This is a simple algorithm that works well enough to handle a large possible range of screen sizes. This is how the old chrome extension did it and the ratios are taken directly from there.

@danielweck
Copy link
Member

The left/right margins on either side of the reflowable document (two-page spread or single page) were too big in some instances, squashing the text into narrow columns. I'll create a screenshot to demonstrate the issue.
Furthermore, the application of margins were creating box-model calculation issues in readium-shared-js (which the "transparent border" trick helped alleviate).

@danielweck
Copy link
Member

Oh, and I forgot to say: there are users for whom eye-scanning the entire screen width isn't an issue, they use very large fonts so the word density per line is decreased (accessibility).

@danielweck
Copy link
Member

One quick example:

margins_1
margins_2

@ryanackley
Copy link
Contributor Author

Readium on my desktop

capture

@ryanackley
Copy link
Contributor Author

I understand why you may have decided to do it this way. However, if you still feel like it's the right way after I've pointed out the reasons for the implementation you removed, I disagree.

It's quite obviously wrong to me. The only direct user feedback we have on the margins is someone complaining about the implementation you added back (fixed size margins). That user feedback also happens to make a lot of sense. He gives practical and concrete examples of why it's not the right way. The implementation you removed has been around forever since it was migrated from the old chrome extension (after someone complained about fixed sized margins) without any other complaints.

Discuss it with Ric if you feel strongly about this. He directed me to that forum post after the initial release of the new chrome extension.

@danielweck
Copy link
Member

Regarding your screenshot: I assume "single page view" is forced in user preferences (via the options dialog)? (in other words, two-page spread is explicitly disabled?)

Obviously, I totally agree (who wouldn't?) that the lines of text are way too wide! However, the concept of "margins" as "uniform borders around pages of content" is different from "optimum / maximum horizontal text scanning distance" or "line-word density". Let's address the latter issue separately, so that it benefits both the scroll view (when rendering reflowable documents) and the column-paginated view.

Note that there are in fact e-readers that split documents into 3 "columns" (using viewport width detection via CSS media queries or Javascript, as Readium does), in order to maintain manageable widths for "lines / runs of text". Margins are a separate issue.

Also note that as it stands now, Readium automatically switches to "single page/column view" when the viewport width becomes smaller than its height (portrait aspect ratio), thereby addressing some device-rotation and window-resizing interactions. However, this strikes me as incomplete "responsive UI design", on the basis that the actual line width / eye-scanning distance is not taken into account.

@ryanackley should we rename this issue accordingly? (I agree about "priority blocking", by the way)

Related issue:
#94

@danielweck
Copy link
Member

Another way to put it:
The responsive design that determines the width of a "column" (eye-scanning distance whilst reading a line of text) is a function of both font-size and viewport width, and is not directly related to margins.

Thus why I am suggesting addressing this issue properly, rather than preserving an algorithm from the old Readium implementation which (as per my screenshot) does not result in a satisfactory experience.

As an additional point of reference: Apple iBooksX resizes content more appropriately, by making use of the available screen real estate, whilst using balanced margins + a responsive single/double-page transition that is based on optimum readable line width.

danielweck added a commit that referenced this issue Jul 25, 2014
…and #94 (responsive "page" / "column" width for readable runs / lines of text)
@ryanackley
Copy link
Contributor Author

What it comes down to is that this issue revolves around personal taste and nobody is complaining about this. Most likely because they are perfectly happy with the default setting or can find a margin setting they are perfectly happy with. Keep in mind that this is a setting, so they can adjust the margin to their tastes. I look at your top screenshot with the margins maxed out and you know what? it's perfectly readable. It's actually more readable than the next screenshot with less margin. The excessive whitespace looks kind of silly but someone might prefer readability over their visceral impression.

I agree that the optimal column width is is a function of both font-size and viewport width. However, the way the previous extension did it was a nice balance between simplicity and the optimal solution. I don't think it's worth investing time and limited resources into improving it because there have been no complaints from actual users. In other words, I think it would either go completely unnoticed or make someone angry because they liked the way it used to work.

@danielweck
Copy link
Member

  1. "nobody is complaining about this"
    An't I? And just to be clear: in some cases the left/right margins waste screen real estate as the lines of text are too short (IMO, FWIW). I could also mention people who reported this issue to me during conferences / workshops, but in fairness this doesn't hold much weight as I am the one publicly discussing it now.

  2. "I look at your top screenshot with the margins maxed out"
    Clarification (just in case): both screenshots are taken with the same amount of user-set margins (default option value in the preferences dialog). The top screenshot shows the original algorithm (multi-tier margins calculations depending on viewport width), the bottom one shows the unmodified margins (baring in mind that in the latter case, the issue of optimum reading / line width must be addressed separately).

  3. "I don't think it's worth investing time and limited resources into improving it because there have been no complaints from actual users"
    The Readium project has a strong mandate to meet accessibility requirements. On that basis alone I would want to make sure that viewing options cater for the needs of everybody (font size, page width, etc.). The lack of negative feedback should not be taken as a indication of "implicit agreement", as this is not a vote. Issue Default for Reflowable Content is Spreads; should be Singles #94 demonstrates that there isn't even a consensus within the Readium group.

Daniel

@ryanackley
Copy link
Contributor Author

Take a step back, and look at this issue objectively. Are you really this passionate about page margins? Can't a page margin deep-dive wait until after 1.0? Like I said talk to Ric and Bill if you really want to do this. I'm against it though and I think arguing about further isn't constructive.

danielweck added a commit that referenced this issue Jul 27, 2014
reverses commit a40d84f
(will produce pull request to fix the issues separately, see #107 #106 #105 #103)
@danielweck
Copy link
Member

Closing this issue, code rollback:
1c70d43

Problems pertaining to the old getWidthFactor() code (now restored in "develop" branch) will be addressed separately.

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

No branches or pull requests

2 participants