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

in scroll mode, "page width" (line length) is too short #130

Closed
danielweck opened this issue Jan 30, 2024 · 19 comments
Closed

in scroll mode, "page width" (line length) is too short #130

danielweck opened this issue Jan 30, 2024 · 19 comments
Labels
Feature request Indicates the issue is a feature request Proposed solution Issues or feature requests with a solution in reach

Comments

@danielweck
Copy link
Member

See edrlab/thorium-reader#2061

Relevant code and doc: https://github.com/search?q=repo%3Areadium%2Freadium-css%20RS__maxLineLength&type=code

屏幕截图 2024-01-24 160428

@danielweck
Copy link
Member Author

PS: my opinion (as a user, not specifically a developer) has always been that configuring "margin sizes" is the wrong approach, to me it makes a lot more sense to allow customisation of the "line length" (optimal text run width depending on user preferences / abilities, available screen real estate, etc.). With this perspective in mind, the "margin size" is then just a byproduct / side effect of configuring the "page width" (not the other way around, as was the case in Readium "1" if I remember correctly).

Side note: the implementation of margin indicators (e.g. highlights / annotations), previous/next icon buttons (+ various other reading system affordances) must factor in the fact that margins are dependent on (derived from) the chosen / preferred / default line width.

@mickael-menu
Copy link
Member

👍 I'm also in favor of adding an explicit line-length setting. This would allow using the native font size setting on Android, which works even with publisher styles enabled.

https://github.com/readium/kotlin-toolkit/blob/7649378a0a6b924abedf8b72372c3808fe9b992f/readium/navigator/src/main/java/org/readium/r2/navigator/epub/EpubNavigatorFragment.kt#L136-L150

I would still keep an explicit horizontal-margins setting. It doesn't have to be a user setting, but rather a "static" RS setting. This would ensure a safe area for margin indicators, as you mentioned.

@JayPanoz
Copy link
Collaborator

Thanks Past Me for documenting design decisions, etc. as it helped catching up on this issue quite a lot.

So @danielweck pointed the variable pagination/scroll are using internally e.g. --RS__maxLineLength. Due to pagination using CSS multicolumn, it should be mentioned it’s impacted by --RS__pageGutter to mimic inline margins – so left and right for horizontal writing, top and bottom for vertical writing. In other words, the largest the margins, the shortest the line length. See Auto pagination model.

As a side note, it was also used to force a reflow in webkit-based views/browsers. I’ll have to check whether that bug still occurs as a side quest.

While scroll mode is obviously not using CSS multicolumn, it’s re-using these variables – either explicitly or through the cascade.

/* Make sure line-length is limited in all configs */
:root:--scroll-view body {
--RS__maxLineLength: 40rem !important;
}

And the user setting for page margins (--USER__pageMargins) is relying on --RS__pageGutter:

/* Page Margins: the user margin is a factor of the page gutter e.g. 0.5, 0.75, 1, 1.25, 1.5, etc. */
:root[style*="--USER__pageMargins"] body {
padding: 0 calc(var(--RS__pageGutter) * var(--USER__pageMargins)) !important;
}

This also points out an issue in documentation as these 2 are explicitly listed under pagination while they can be applied in
scroll mode.

Anyways, I guess adding a --USER__lineLength overriding --RS__maxLineLength should more or less do the trick as a first implementation to iterate over. Then it should be easier to see what could help make things predictable – in hindsight, we probably tried to be too smart for our own good in ReadiumCSS on occasion.

@JayPanoz
Copy link
Collaborator

TODO: define how a --USER__lineLength setting should behave.

One could complain it only works in scroll mode while it could also be applied in single column – but in that case how should it behave in 2 columns (pseudo spread).

Also, how it should interact with --USER__pageMargins.

@JayPanoz
Copy link
Collaborator

Draft for a proposed resolution.

  • create a user setting submodule and expose --USER__lineLength as the custom property for line length
  • --USER__lineLength should override --RS__maxLineLength
  • --USER__lineLength should apply to single column pagination and scroll
  • --USER__pageMargins should still apply if they are set by the user

At the moment it’s still unclear to me how this should behave in 2-columns as I need some practical implementation to see what’s possible.

@JayPanoz
Copy link
Collaborator

JayPanoz commented May 6, 2024

Note to self: some authors’ styles may have a max-width applied to body or a div for body copy so it should be overridden in the user setting submodule.

@JayPanoz
Copy link
Collaborator

Proposed resolution: While page margins can be kept for Reading System purposes and even as a user setting – if only for legacy/deprecation, a new USER variable should be created for line length so that it can be set.

It’s still unclear what would be best as a value e.g. CSS units (length), multiplier as is the case with page-margins, etc. so please feel free to record your preference in this issue.

This line length setting should override authors’ max-width as well, if set.

Documentation should be updated accordingly.

@JayPanoz JayPanoz added Feature request Indicates the issue is a feature request Proposed solution Issues or feature requests with a solution in reach labels May 16, 2024
@danielweck
Copy link
Member Author

The problem with "margins" in "pages" is that this tangible / physical concept does not technically map to column gap very well, as the whitespace is uniformly distributed across columns. This equidistant property is an inherent characteristic of the layout model which we can't fix (and it has its benefits anyway, so better live with it) but this is a problem on any widescreen, let me post screenshots which are easier than words :)

@danielweck
Copy link
Member Author

Screenshot 2024-05-16 at 19 09 01

@danielweck
Copy link
Member Author

Screenshot 2024-05-16 at 19 11 48

@danielweck
Copy link
Member Author

danielweck commented May 16, 2024

The image above is a mockup, that's what I'd like to achieve in Thorium navigator ... technically it's a bit more complicated than it looks due to viewport calculations and applying masks (to hide the before/next CSS columns beyond the current page spread)

The margin indicators for annotations would still work, but there would be a number of changes in the arithmetics involved in computing character-level visibility, etc. It's a lot trickier than it sounds due to the current layout model.

To be honest, the more I think about it, the more I believe the easiest (less bug-prone) solution would be to shrink the actual viewport (out of process iframe, in Electron) and possibly collaborate from the navigator with the application to achieve the desired rendering layout.

@mickael-menu
Copy link
Member

To be honest, the more I think about it, the more I believe the easiest (less bug-prone) solution would be to shrink the actual viewport (out of process iframe, in Electron) and possibly collaborate from the navigator with the application to achieve the desired rendering layout.

Yes, that's my sentiment as well. But it gets tricky to have proper page-turn animations. Maybe using snapshots we can provide a better experience with slide or fade-out pages.

@JayPanoz
Copy link
Collaborator

JayPanoz commented May 17, 2024

The problem with "margins" in "pages" is that this tangible / physical concept does not technically map to column gap very well, as the whitespace is uniformly distributed across columns. This equidistant property is an inherent characteristic of the layout model which we can't fix (and it has its benefits anyway, so better live with it) but this is a problem on any widescreen

Ah yeah sorry I’ve just noticed that the wording of the resolution wasn’t super clear so I’ll try my best to clarify for the benefit of everyone reading this conversation first.

So I’d be more than happy to remove --USER__pageMargins (which is body element left and right padding) entirely if there is no request to keep it or no deprecation period needed to that it’s more comfortable to migrate.

And I guess that with a new --USER__lineLength (body element max-width) you could achieve the behaviour of the existing page margins anyway, if implemented as something like this pseudo CSS code off the top of my head:

body {
  --computedLineLength : multiplier * 100%;
  /* 100% being the width of the column, 
      multiplier a number between 0 and 1 */
  
 max-width : --computedLineLength;
 margin : 0 auto; /* to center */
}

However, that doesn’t fundamentally change how things currently work, it just replaces page margins with users’ line length (or the default one).

So question.

Would removing any inline margin, padding, (gut feeling is not zero-ing the column gap as otherwise there wouldn’t be any whitespace between columns when several are displayed), and relying entirely on column-width be worth exploring and experimenting with? This also means the number of visible columns won’t be constrained, and up to the app to manage.

@danielweck
Copy link
Member Author

A couple of thoughts:

  1. I think that this round of ReadiumCSS updates shouldn't shy away from introducing breaking changes. I believe this is the perfect opportunity to release a major revision that revisits some key design decisions. That being said, maybe no breaking changes at all are necessary to achieve the envisioned fixes / improvements ... I haven't read the whole document yet so I must educate myself more to better understand the scope of planned evolutions.

  2. I personally see "pagination" (using CSS columns under the hood) from a high-level API consumer perspective as a minimal set of controls: (1) gap between 2 consecutive columns, which in a 2-page spread is the fold area, twice the width of left and right "margins", and (2) maximum length of lines of text. The auto-switch from 2 columns to a single one can probably be handled by the application itself (maybe even 3 or more columns on very wide displays) so I suppose that's a third control :)
    In scroll mode, the "maximum length of text / line width" is also important, though I personally like the idea of defaulting to "no max" and let the user resize the Thorium window to suit their needs/taste. For users who love their app window in full screen, then a configurable "max line length" is essential (opt-in, I would say).

@JayPanoz
Copy link
Collaborator

I think that this round of ReadiumCSS updates shouldn't shy away from introducing breaking changes.

Yeah to be fair the mention of “experimental features” in #139 was already a hint that maybe we would draw the conclusion a new major version may be warranted.

In any case in hindsight it’s become clear we should remove the pain points for implementers and keep the library as an unopinionated tool.

When reviewing issues, I became aware that a lot were maybe symptoms of a bigger problem – something I tried to explain in the doc.

So that may well change the perception of pagination and how it’s designed. We’re already removing “responsive columns” so that apps/toolkits can implement that aspect as they see fit. Going the extra mile and trying another approach wouldn’t be that surprising to be honest.

@mickael-menu
Copy link
Member

This also means the number of visible columns won’t be constrained, and up to the app to manage.

Would you mind explaining what does that entail?

@JayPanoz
Copy link
Collaborator

@mickael-menu I may create a Codepen demo so that it’s a little bit easier to experience directly in a browser and can serve as an illustration but basically, there’s no limit to the number of columns visible in the viewport.

If it’s large enough and it can fit 3 columns (3 * column-width + 2 * column-gap), or 4, or 5, etc. they will be visible and up to the app to hide/mask depending on the user-setting (1 or 2-col/pages) or default.

What column-count does in CSS when it is set is that it limits the number of columns visible, and column-width can grow to accommodate for that.

So that’s why I’m not sure whether it could be a better option, especially as CSS Multicol is one of the rare specs that doesn’t ship with a JavaScript API, and you can’t access easily the number of columns it has created, the dimensions of the “paginated document”/fragmented box, the dimensions of the columns, etc. Things may become overly complex real fast, or maybe it could actually help as the app would be in control of the entire thing. I don’t personally have an answer to that question.

@mickael-menu
Copy link
Member

How would an app hide the extra visible columns in this case? If it's displaying a blank view above them, that might be an issue for the page-turn animations (scrolls mostly).

@danielweck
Copy link
Member Author

yeah my naive description / wishlist above blissfully ignored the fact that the CSS column layout heuristics create some kind of push/pull tension between "what I ideally want" and "what the rendering engine decides to do".
I mean, it's kind of a "catch 22" situation where the RS can decide to set its iframe / Webview width to a certain dimension that ideally accommodates the 2-page spread and its "margins" (doing this means no need for masking the columns before and after) ... but in order to do this I need to know the layout decisions that CSS column makes when giving it constraints like "max line width" and column gap.

JayPanoz added a commit that referenced this issue Jun 26, 2024
Part of issue #130 resolution
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature request Indicates the issue is a feature request Proposed solution Issues or feature requests with a solution in reach
Projects
None yet
Development

No branches or pull requests

3 participants