Add support for CSS width: fit-content by reuerendo · Pull Request #647 · koreader/crengine

reuerendo · 2026-01-29T09:10:01Z

The implementation of the fit-content keyword (written using GPT-5.2, no coding experience)

This provides support in cases where alignment should be applied to the text block itself rather than the text inside the block: poetry, epigraphs, quotations, etc.

For example, according to classical book typography rules, poems are aligned to the center of the page relative to their longest line. Without fit-content, this could only be achieved using display: table (which removed margin inside the block) or display: inline-block (which prevented the block from breaking across pages).

This change is

poire-z

I hope you can follow my comments and do the changes yourself (it's not really code, it's just reorganization, understanding the current logical branchs and moving blocks around taking care of indentations and { } logical balances.
And comments :)

poire-z · 2026-01-29T20:20:30Z

                                    bool accept_unspecified=false,
                                    bool accept_contain_cover=false,
                                    bool accept_cr_special=false,
-                                    bool is_font_size=false );
+                                    bool is_font_size=false,
+                                    bool accept_fit_content=false );


I'd rather have accept_fit_content just before is_font_size, so all the accept_* are together (so, you need to reorder them in a few places below.

Also, below, there are a lot of:
if ( parse_number_value(str, val, false, false, false, false, false, false, false, false, false, false) ) {
The AI didn't get that above, there are default values: if we don't specify all the params to the function when we call it, these default params wil be use. So, this serie of false, false, false, false, false, false, false, false, false, false can just be left out, as it was before.

poire-z · 2026-01-29T20:23:23Z

+        // Check for fit-content in height: treat as auto (no fixed height)
+        bool height_fit_content = (style_height.type == css_val_unspecified && 
+                                   style_height.value == css_generic_fit_content);
+        // Convert fit-content to auto so normal height logic applies
+        if ( height_fit_content ) {
+            style_height.value = css_generic_auto;
+        }


It looks to me like we do nothing when css_val_unspecified, so auto or fit-content won't have anything done.
(Moreover, I think this is confirmed by the fact you assign to the local css_length_t style_height = style->height, which soon gets out of scope and disappear, so even if we would later use style->height (the long-living one we copy the value from, we would still have style->height.value = css_generic_fit_content).
So, no need have any code here, You can just let a comment.
// If css_generic_fit_content, nothing special to do

poire-z · 2026-01-29T20:28:33Z

+            // Check for fit-content in max-width
+            if ( style_max_width.type == css_val_unspecified && style_max_width.value == css_generic_fit_content ) {
+                style_max_width.value = css_generic_auto; // treat as no max-width constraint
+            }


Same comment as for height above.
(If it is css_val_unspecified , you change the value for the local style_max_width , but below, you just check and do stuff if ( style_max_width.type != css_val_unspecified ) , so it doesn't have any impact whether you change it or not.

poire-z · 2026-01-29T20:31:13Z

Given the only test we do in this branch (if ( style_width.type != css_val_unspecified && ...), no need to build and check a has_fit_content_width. A comment is enough
// (if fit-content, nothing to do, we'll get the intrisic width of the element)

poire-z · 2026-01-29T20:42:06Z

+        bool is_fit_content = false;  // Track if we're using fit-content
        css_length_t style_width = style->width;
+        // Check for fit-content: treat as shrink-to-content width
+        bool has_fit_content_width = (style_width.type == css_val_unspecified && 
+                                      style_width.value == css_generic_fit_content);
+		// For fit-content, we need to get the content width similar to floats
+		if ( has_fit_content_width ) {


The computation looks ok, but how all this fits into the logic (when reading) is totally misplaced and have side effects (you would apply something to a table, while the comment that was there says // table sub-elements widths are managed by the table layout algorithm. We don't want to have fit-content take precedence.
(The AI took the first place at our table without caring for how things were.)

So, I spent some minutes looking at our code.
There is a first branch that sets apply_style_width, that's where we decide if we should.
So, I guess there you should add as the last else if:

// Also only if ENSURE_STYLE_WIDTH as we may prefer having // full width text blocks to not waste reading width with blank areas. else if ( style_width.type != css_val_unspecified && style_width.value == css_generic_fit_content ) { if ( BLOCK_RENDERING(flags, ENSURE_STYLE_WIDTH) ) apply_style_width = true; }

Then, in the if ( apply_style_width ) {:

if ( apply_style_width ) { if ( style_width.type != css_val_unspecified && style_width.value == css_generic_fit_content ) { [ ... your code ... ] auto_width = true; else { // the original width = lengthToPx( enode, style_width, container_width );

and I think your else if ( !is_fit_content ) { can go.

poire-z · 2026-01-29T20:44:56Z

+			int max_content_width = 0;
+			int min_content_width = 0;
+			int rend_flags = flags | BLOCK_RENDERING_ENSURE_STYLE_WIDTH | BLOCK_RENDERING_ALLOW_STYLE_W_H_ABSOLUTE_UNITS;
+			// getRenderedWidths() returns width including padding/margin of children,
+			// but we need to ignore margin of this element itself (ignoreMargin=true)
+			// Note: getRenderedWidths() already includes padding/margin/border of children
+			getRenderedWidths(enode, max_content_width, min_content_width, direction, true, rend_flags);
+
+			// getRenderedWidths() returns the content width including padding/margin/border
+			// of children, but for fit-content we want the shrink-to-fit width.
+			// The returned max_content_width should be the width needed by the content.
+			// We need to ensure it doesn't exceed available width (container minus our margins).
+			int available_width = container_width - margin_left - margin_right;
+
+			// Use the maximum of content width, but not exceeding available width
+			width = max_content_width;
+			if (width > available_width) {
+				width = available_width;
+			}
+
+			auto_width = false;


This is [ ... your code ... ].
The 2 3-4 lines comments sounds redundant, you can probably summarize it all in one comment.
You can remove the blank lines.

It sounds like chinese to me, but I think I got it

poire-z

Yes, you got it !
And you also got more review comments :)

poire-z · 2026-01-30T14:56:26Z

-                                    bool is_font_size )
+                                    bool is_font_size,
+                                    bool accept_fit_content )


You need to change the order here too (it should match what's in lvstsheet.h)

poire-z · 2026-01-30T14:57:17Z

                    if ( prop_code==cssd_padding_left || prop_code==cssd_padding_right )
                        accept_cr_special = true;
-                    if ( parse_number_value( decl, len, accept_percent, accept_negative, accept_auto, accept_none, accept_normal, accept_unspecified, false, accept_cr_special, is_font_size) ) {
+                    if ( parse_number_value( decl, len, accept_percent, accept_negative, accept_auto, accept_none, accept_normal, accept_unspecified, false, accept_cr_special, is_font_size, accept_fit_content) ) {


You need to change the order here too (we should pass the argument to that function in the same order they were defined).

poire-z · 2026-01-30T14:58:26Z

        // We always use the style height for <HR>, to actually have a height to fill
        // with its color (as some of our css files render them via height)
        css_length_t style_height = style->height;
+        // If css_generic_fit_content, nothing special to do


My previous suggestion was a bit lazy.
Actually here, you can write:
// If css_generic_fit_content, nothing to do, it is our default behaviour

poire-z · 2026-01-30T14:59:55Z

+            // Also only if ENSURE_STYLE_WIDTH as we may prefer having
+            // full width text blocks to not waste reading width with blank areas.
+            else if ( is_fit_content_width ) {
+                if ( BLOCK_RENDERING(flags, ENSURE_STYLE_WIDTH) )


Move the comment inside the else if, above the if ( BLOCK_RENDERING(flags, ENSURE_STYLE_WIDTH) ).
(So it does not disrupt the reading of the outer branches, and the comment is actually about the if ( BLOCK_RENDERING(flags, ENSURE_STYLE_WIDTH) )

poire-z · 2026-01-30T15:00:43Z

+                int rend_flags = flags | BLOCK_RENDERING_ENSURE_STYLE_WIDTH |
+                                 BLOCK_RENDERING_ALLOW_STYLE_W_H_ABSOLUTE_UNITS;


Make it on one line (just contactenate these 2 lines), easier to read.

poire-z · 2026-01-30T15:01:19Z

+                // Compute shrink-to-fit width from rendered content, ignoring this element's
+                // own margins, and clamp it to the available container width.
+                getRenderedWidths(enode, max_content_width, min_content_width,
+                                  direction, true, rend_flags);
+                int available_width = container_width - margin_left - margin_right;
+                width = max_content_width;
+                if ( width > available_width )
+                    width = available_width;
+                auto_width = false;
+            }


Perfect comment and code !

poire-z · 2026-01-30T15:02:29Z

+            else { // the original
+                width = lengthToPx( enode, style_width, container_width );


It's "the original" for you and the AI while making the PR. For future reader, it will not be :)
else { // Fixed specified width

poire-z · 2026-01-30T15:04:56Z

+            // Check for fit-content in min-width
+            if ( style_min_width.type == css_val_unspecified && style_min_width.value == css_generic_fit_content ) {
+                style_min_width.value = css_generic_auto; // treat as no min-width constraint
+            }


I added this comment on max-width in my previous review, but it also applies there for min-width:

Same comment as for height above.
(If it is css_val_unspecified , you change the value for the local style_max_width , but below, you just check and do stuff if ( style_max_width.type != css_val_unspecified ) , so it doesn't have any impact whether you change it or not.

poire-z · 2026-01-30T15:10:59Z

+        // Check for fit-content: treat as shrink-to-content width (don't use fixed width)
+        bool has_fit_content_width = (style_width.type == css_val_unspecified && 
+                                      style_width.value == css_generic_fit_content);
+        if ( BLOCK_RENDERING(rendFlags, ENSURE_STYLE_WIDTH) && !has_fit_content_width ) {


You can make it one line, like you did above:
bool has_fit_content_width = (style_width.type == css_val_unspecified && style_width.value == css_generic_fit_content);
A probably better comment (because there is fixed width to use, and it is actually what this function does :)
// ('width: fit-content' is actually what this function returns as maxWidth)

poire-z

So your commit names countdown 3 .. 2 .. can reach 1 and may be 0.

poire-z · 2026-01-30T16:01:55Z

            }
+
+            else if ( is_fit_content_width ) {


Uneeded blank line.

poire-z · 2026-01-30T16:03:00Z

    else { // regular element (non-float)
        bool apply_style_width = false;
        css_length_t style_width = style->width;
+        bool is_fit_content_width = ( style_width.type == css_val_unspecified && style_width.value == css_generic_fit_content );


No need for the parens ( ... ) around that.

poire-z · 2026-01-30T16:04:13Z

        css_length_t style_width = style->width;
-        if ( BLOCK_RENDERING(rendFlags, ENSURE_STYLE_WIDTH) ) {
+        // ('width: fit-content' is actually what this function returns as maxWidth)
+        bool has_fit_content_width = (style_width.type == css_val_unspecified && style_width.value == css_generic_fit_content);


No need for the parens ( ... ) around that.

poire-z · 2026-01-30T16:04:33Z

+                getRenderedWidths(enode, max_content_width, min_content_width,
+                                  direction, true, rend_flags);


You can keep it all on one line.

reuerendo · 2026-01-30T16:20:29Z

So your commit names countdown 3 .. 2 .. can reach 1 and may be 0.

This is the number of modified files :)

poire-z · 2026-04-19T17:10:07Z

                // In crengine, the width we deal with is the border box, so we
                // just add paddings and borders to the width we got from styles.
                width += padding_left + padding_right;
            }


Any reason for us to have added this && !is_fit_content_width ?
There is some behaviour regarding fit-content reported at https://www.mobileread.com/forums/showthread.php?p=4580669#post4580669 that seems to indicate padding are not accounted, which feels like a bug, and removing the added condition fixes it.

(My line secltion should have included a few lines after that snippet.)

I think it should even be:

- else if ( style->box_sizing == css_bs_content_box && !is_fit_content_width ) { + else if ( style->box_sizing == css_bs_content_box || is_fit_content_width ) { // If W3C box model requested, CSS width specifies the width // of the content box. // In crengine, the width we deal with is the border box, so we // just add paddings and borders to the width we got from styles. width += padding_left + padding_right; }

I haven't looked at the spec, but if that aligns with other rendering engines presumably that's the way to go. ^_^

fit-content

10f34f7

poire-z reviewed Jan 29, 2026

View reviewed changes

3

53fe743

poire-z reviewed Jan 30, 2026

View reviewed changes

2

dcc1d9d

poire-z reviewed Jan 30, 2026

View reviewed changes

1

2f3a911

poire-z approved these changes Jan 30, 2026

View reviewed changes

poire-z changed the title ~~fit-content~~ Add support for CSS width: fit-content Jan 30, 2026

poire-z merged commit 48e69fe into koreader:master Jan 31, 2026
1 check passed

reuerendo deleted the master-fit-content branch February 2, 2026 15:31

This was referenced Feb 14, 2026

bump crengine: CSS enhancements and other fixes, improve text selection koreader/koreader-base#2270

Merged

bump base & crengine: CSS enhancements and other fixes koreader/koreader#14979

Merged

benoit-pierre pushed a commit to benoit-pierre/koreader that referenced this pull request Feb 17, 2026

Add support for CSS width: fit-content (koreader/crengine#647)

a23d7d7

poire-z reviewed Apr 19, 2026

View reviewed changes

poire-z mentioned this pull request Apr 22, 2026

CSS: first-line and fit-content fixes, improve getNodeByPoint() fallback #662

Merged

		int rend_flags = flags \| BLOCK_RENDERING_ENSURE_STYLE_WIDTH \|
		BLOCK_RENDERING_ALLOW_STYLE_W_H_ABSOLUTE_UNITS;

		else { // the original
		width = lengthToPx( enode, style_width, container_width );

		getRenderedWidths(enode, max_content_width, min_content_width,
		direction, true, rend_flags);

Uh oh!

Conversation

reuerendo commented Jan 29, 2026 • edited by Frenzie Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

poire-z left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

poire-z left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

poire-z left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

reuerendo commented Jan 30, 2026

Uh oh!

Uh oh!

poire-z Apr 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

reuerendo commented Jan 29, 2026 •

edited by Frenzie

Loading

poire-z Apr 19, 2026 •

edited

Loading