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

Refactor compute_variable_dimension function to specifications #1890

Merged
merged 2 commits into from
May 29, 2023

Conversation

kygoh
Copy link
Contributor

@kygoh kygoh commented May 28, 2023

In 2012, when @SimonSapin implemented the page-margins layout, the specification was this one:
https://www.w3.org/TR/2006/WD-css3-page-20061010/#margin-dimension

[...]

This proposition was accepted, probably with some changes (I don't remember, maybe Simon does). So the first step is to find what's different between the current specification and the current implementation in compute_variable_dimension, and then fix these differences.

Originally posted by @liZe in #58 (comment)

I think the proposition was accepted without changes(?) because all tests passed without changes even after refactoring the code to "more closely follow" the specifications layout terminologies and algorithm flow.

@liZe
Copy link
Member

liZe commented May 29, 2023

Thanks! It seems to be safe to merge and to close #58.

One of the main differences between WeasyPrint and other tools (at least Prince) is that page margin boxes with defined widths are not resized to exactly cover the whole margin: they can overflow or leave empty spaces. I can’t find anything in the specification about this, did I miss something?

@liZe liZe added the bug Existing features not working as expected label May 29, 2023
@kygoh
Copy link
Contributor Author

kygoh commented May 29, 2023

page margin boxes with defined widths are not resized to exactly cover the whole margin: they can overflow or leave empty spaces. I can’t find anything in the specification about this, did I miss something?

If I understand the specification correctly, WeasyPrint is implemented according to the specification:

The rules for applying min-height, max-height, min-width, and max-width [CSS21] do apply to page-margin boxes and may imply a recalculation of the width, height, and/or margins if the dimensions resulting from the specified width or height violate their constraint. If the UA does not support the min-height or min-width properties then it must behave as if min-height and min-width were always zero.

The statement in italic-bold is what I was referring to and compute_fixed_dimension appears to match the rules in 5.3.3. Page-Margin Box Fixed Dimension Computation Rules.

I must admit I am not very familiar with the whole specification, so please point out if I've misunderstood it.

@liZe
Copy link
Member

liZe commented May 29, 2023

The statement in italic-bold is what I was referring to and compute_fixed_dimension appears to match the rules in 5.3.3. Page-Margin Box Fixed Dimension Computation Rules.

I think that the paragraph is only about recalculating width and height if they don’t follow the min-* and max-* properties.

My question was related to the size of the margin boxes when their sum is larger than the page. What happens for example when width: 400mm is set to @top-center when the page width is 210mm? Other tools always avoid overlapping and overflowing page margin boxes, but WeasyPrint doesn’t.

@kygoh
Copy link
Contributor Author

kygoh commented May 29, 2023

What happens for example when width: 400mm is set to @top-center when the page width is 210mm?

Do you mean that even though:

compute_fixed_dimension appears to match the rules in 5.3.3. Page-Margin Box Fixed Dimension Computation Rules

it wasn't handled correctly or the specification is silent on how that should be handled?

@liZe
Copy link
Member

liZe commented May 29, 2023

Do you mean that even though:

compute_fixed_dimension appears to match the rules in 5.3.3. Page-Margin Box Fixed Dimension Computation Rules

it wasn't handled correctly or the specification is silent on how that should be handled?

This part sets the height of the @top-center box, but I’m talking of its width. I’m afraid there’s nothing in the specification about that, and I was wondering if you’ve seen something I haven’t.

@kygoh
Copy link
Contributor Author

kygoh commented May 29, 2023

This part sets the height of the @top-center box, but I’m talking of its width. I’m afraid there’s nothing in the specification about that, and I was wondering if you’ve seen something I haven’t.

I think I understand what you mean. However, I still feel that WeasyPrint's implementation is better than the other tools by rendering the following two cases for the various tools in PrintCSS Playround:

<style>
@page {
  size: 800px;
  margin: 100px;
  padding: 42px;
  border: 7px solid blue;
  @top-center { content: 'the quick brown fox jumped over the lazy dog then the quick brown fox jumped over the lazy dog then the quick brown fox jumped over the lazy dog then the quick brown fox jumped over the lazy dog'; 
    width: 400px;
  }
  @top-right { content: 'the quick brown fox jumped over the lazy dog then the quick brown fox jumped over the lazy dog then the quick brown fox jumped over the lazy dog then the quick brown fox jumped over the lazy dog';
    width: 400px;
  }
}
</style>
<style>
@page {
  size: 800px;
  margin: 100px;
  padding: 42px;
  border: 7px solid blue;
  @top-center { content: 'the quick brown fox jumped over the lazy dog then the quick brown fox jumped over the lazy dog then the quick brown fox jumped over the lazy dog then the quick brown fox jumped over the lazy dog';
    width: 400px;
  }
  @top-right { content: 'the quick brown fox jumped over the lazy dog then the quick brown fox jumped over the lazy dog then the quick brown fox jumped over the lazy dog then the quick brown fox jumped over the lazy dog';
    width: auto;
  }
}
</style>

The two cases were rendered exactly the same on Prince: "beautiful" without overlapping and also "auto-cropping".

On the other hand, WeasyPrint rendered the two differently and also "uglily" with overlap as well as text running into page area.

However, I feel that the "beautiful" rendering is not "respecting" the intention of the designer😬. If the designer intends the boxes to overlap (although by mistake), then it should be rendered accordingly.

@liZe
Copy link
Member

liZe commented May 29, 2023

However, I feel that the "beautiful" rendering is not "respecting" the intention of the designer. If the designer intends the boxes to overlap (although by mistake), then it should be rendered accordingly.

I agree 😄. But the only rule we try to follow is the specification, not what the user finds useful 😄. If you too can’t find anything in the specification, then let’s do what we want (ie. keep what we have, in my opinion).

@liZe liZe merged commit ddd739c into Kozea:master May 29, 2023
@liZe liZe added this to the 60.0 milestone May 29, 2023
@kygoh
Copy link
Contributor Author

kygoh commented May 29, 2023

If you too can’t find anything in the specification, then let’s do what we want (ie. keep what we have, in my opinion).

To complete our discussion on this issue, the note in 5.3.2.2. Resolving auto widths states:

Note: The high-level goals are (in order of priority) to center the middle box (B) if it is generated, to minimize overflow and overlap, and to distribute space proportionally to the amount of content.

Though not stated, I think if these high-level goals are similarly applied to fixed dimension computation rules, WeasyPrint is staying true to the specification.

@liZe
Copy link
Member

liZe commented May 30, 2023

Though not stated, I think if these high-level goals are similarly applied to fixed dimension computation rules, WeasyPrint is staying true to the specification.

You’re right. "to minimize overflow and overlap" indicates that the boxes can actually overflow and overlap if the user wants, that’s cool!

netbsd-srcmastr referenced this pull request in NetBSD/pkgsrc Oct 15, 2023
Version 60.1
------------

Released on 2023-09-29.

Bug fixes:

* `#1973 <https://github.com/Kozea/WeasyPrint/issues/1973>`_:
  Fix crash caused by wrong UTF-8 indices


Version 60.0
------------

Released on 2023-09-25.

New features:

* `#1903 <https://github.com/Kozea/WeasyPrint/issues/1903>`_:
  Print form fields
* `#1922 <https://github.com/Kozea/WeasyPrint/pull/1922>`_:
  Add support for textLength and lengthAdjust in SVG text elements
* `#1965 <https://github.com/Kozea/WeasyPrint/issues/1965>`_:
  Handle <wbr> tag
* `#1970 <https://github.com/Kozea/WeasyPrint/pull/1970>`_:
  Handle y offset of glyphs
* `#1909 <https://github.com/Kozea/WeasyPrint/issues/1909>`_:
  Add a --timeout option

Bug fixes:

* `#1887 <https://github.com/Kozea/WeasyPrint/pull/1887>`_:
  Fix footnote-call displayed incorrectly for some fonts
* `#1890 <https://github.com/Kozea/WeasyPrint/pull/1890>`_:
  Fix page-margin boxes layout algorithm
* `#1908 <https://github.com/Kozea/WeasyPrint/pull/1908>`_:
  Fix IndexError when rendering PDF version 1.4
* `#1906 <https://github.com/Kozea/WeasyPrint/issues/1906>`_:
  Apply text transformations to first-letter pseudo elements
* `#1915 <https://github.com/Kozea/WeasyPrint/pull/1915>`_:
  Avoid footnote appearing before its call
* `#1934 <https://github.com/Kozea/WeasyPrint/pull/1934>`_:
  Fix balance before "column-span: all"
* `#1935 <https://github.com/Kozea/WeasyPrint/issues/1935>`_:
  Only draw required glyph with OpenType-SVG fonts
* `#1595 <https://github.com/Kozea/WeasyPrint/issues/1595>`_:
  Don’t draw clipPath when defined after reference
* `#1895 <https://github.com/Kozea/WeasyPrint/pull/1895>`_:
  Don’t ignore min-width when computing cell size
* `#1899 <https://github.com/Kozea/WeasyPrint/pull/1899>`_:
  Fix named pages inheritance
* `#1936 <https://github.com/Kozea/WeasyPrint/pull/1936>`_:
  Avoid page breaks caused by children of overflow hidden boxes
* `#1943 <https://github.com/Kozea/WeasyPrint/issues/1943>`_:
  Use bleed area for page’s painting area
* `#1946 <https://github.com/Kozea/WeasyPrint/issues/1946>`_:
  Use margin box of children to define available width for leaders
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Existing features not working as expected
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants