readium / readium-css

🌈 A set of reference stylesheets for EPUB Reading Systems, starting with Readium Mobile
https://readium.org/readium-css/
BSD 3-Clause "New" or "Revised" License
89 stars 20 forks source link

Remove responsive columns to make pagination more reliable #143

Open JayPanoz opened 2 months ago

JayPanoz commented 2 months ago

I'm submitting a bug report.

Short description of the issue/suggestion:

At the moment ReadiumCSS’ logic for responsive columns is targeting mobile devices via min-device-width and max-device-width in landscape orientation.

https://github.com/readium/readium-css/blob/583011453612e6f695056ab6c086a2c4f4cac9c0/css/src/modules/ReadiumCSS-pagination.css#L130-L140

In config, the max-device-width is set to 47em, or 752px with the default font-size of 16 pixels. But iPhone 15 Pro Max is 932 pixels for instance.

https://github.com/readium/readium-css/blob/583011453612e6f695056ab6c086a2c4f4cac9c0/css/ReadiumCSS-config.css#L17-L22

We had to investigate larger devices years ago, but couldn’t while I was consulting on the project. As far as I can tell, given it’s coming back to bite us in the back, it would be preferable to drop device-specific logic.

HadrienGardeur commented 2 months ago

I'm not sure if this is related, if not, let me know and I'll open a separate issue.

I've had a foldable devices (Pixel Fold) for a few months now and I've noticed that when using the two-column mode on the inner screen of the device (the foldable one), I end up with:

I would've expected the opposite behavior, where landscape mode would display two columns but portrait would default to a single column given the ratio/space available.

Screenshots ![Landscape mode](https://github.com/readium/readium-css/assets/90989/5568261b-8a12-47b7-ad6e-96d3cb5232f5) ![Portrait mode](https://github.com/readium/readium-css/assets/90989/1cbd6746-d5f5-481a-8f53-fc7f9b59f90b)
JayPanoz commented 2 months ago

@HadrienGardeur That is probably the root cause as currently the pagination module is targeting a range of CSS device widths (crudely 576px → 752px) in landscape to apply 2 columns automatically for mobile devices, and a viewport of 960px (60em * 16px) for other cases.

There’s consequently a black hole in which the responsive columns disappear between 752 and 960.

That was initially designed to prevent having two columns (“2-page spread”) on mobile devices in portrait mode but as your example shows, it’s now failing at achieving that on some newer form-factors and devices.

A short-term solution would be bumping 752px to a bigger number in develop to at least handle a range of larger devices, but it wouldn’t necessarily be future-proof, and clearly we need something more robust cos’ it could become quite complicated quite quickly if we add conditions to target devices specifically.

JayPanoz commented 2 months ago

Note to self: Primer on foldables’ media queries as there might be some fine-tuning to do in the worst-case scenario.

JayPanoz commented 2 months ago

Proposed resolution: Remove responsive columns entirely.

It’s something that is trying to be too clever for its own good, and breaks the expectations of consumers/implementers. It makes it unreliable right now cos the setting for 1 or 2 columns/pages or the width of columns may not work depending on some conditions.

It should just work as expected by consumers/implementers when setting custom properties, without any opinionated implementation in CSS.

That moves control over breakpoints (i.e. when to switch from 1 to 2), when to enable/disable number of columns, etc. at the app level, and ReadiumCSS should only apply styles (custom properties) the Reading System or user sets without restriction.

mickael-menu commented 2 months ago

In favor of proposed resolution 👍

HadrienGardeur commented 2 months ago

I agree with the proposed resolution and I would go even beyond that.

Over the years, we've received a lot of feedback from implementers or users who do not want the optimal line length at the core of Readium CSS handling of columns. An optimal line length remains a very useful opinionated default, but for those who just want "a screen full of text" should be able to get it too.

This might also have some impacts on the Preferences API and the concept of margins, which seems confusing for a lot of users when used alongside an optimal line length.

JayPanoz commented 2 months ago

@HadrienGardeur Ah right, that would somehow impact and possibly simplify a proposed resolution for #130.

In hindsight, there are opinionated design decisions that should be removed from ReadiumCSS so that apps/implementers can make theirs – or not make. It’s become pretty clear with all the feedback recently that ReadiumCSS should be a simple and reliable library, and that’s it i.e. it shouldn’t throw curve balls.

JayPanoz commented 1 month ago

Proposed resolution: Responsive columns will be remove so that it just works as expected by consumers/implementers when setting custom properties, without any opinionated implementation in CSS.

This moves control over breakpoints (i.e. when to switch from 1 to 2), when to enable/disable number of columns, etc. at the app level, and ReadiumCSS should only apply styles (custom properties) the Reading System or user sets without restriction.

Impact:

App developers and/or toolkits will have to re-implement the current behavior programmatically (1-col in portrait, 2-col in landscape) if they want to keep it AS-IS. Otherwise it will be 1-col in both.