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

Dont clip page margins on account of body overflow #1141

Merged
merged 4 commits into from Jun 22, 2020

Conversation

Tontyna
Copy link
Contributor

@Tontyna Tontyna commented Jun 14, 2020

If we want the viewport_overflow restriction being applied to the <BlockBox html> only, the instructions for draw_stacking_context() are:

  1. never clip when a PageBox is rendered
  2. do the clip when drawing a <BlockBox html> whose page's overflow isn't visible

Good thing is:
We can rely upon the with stacked(context) statement to save and restore the context state automagically.

fixes #35

Though the `overflow` on the root element must be propagated to the
viewport we mustn't cut off the page margins in `draw_stacking_context()`

1. never clip when a PageBox is rendered
2. do the proposed clip when drawing the <BlockBox html>

fixes Kozea#35
@Tontyna
Copy link
Contributor Author

Tontyna commented Jun 14, 2020

Any idea how a unit-test would look like? Checking pixels of a write_png?

if box.style['overflow'] != 'visible':
# dont clip the PageBox, see #35
if box.style['overflow'] != 'visible' and not isinstance(
box, boxes.PageBox):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@@ -184,6 +184,15 @@ def draw_stacking_context(context, stacking_context, enable_hinting):
# See http://www.w3.org/TR/CSS2/zindex.html
with stacked(context):
box = stacking_context.box

# apply the viewport_overflow to the html box, see #35
if box.element_tag == 'html' and (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn’t we use box.is_for_root_element instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wasn't aware that there is a box.is_for_root_element.

Though, I always wondered about the # No root element fallback in build_formatting_structure(). Never ever managed to construct an HTML/element_tree without a 'html' root node.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though, I always wondered about the # No root element fallback in build_formatting_structure(). Never ever managed to construct an HTML/element_tree without a 'html' root node.

Good question, I don’t know if html5lib can give a document with no root html tag. Coverage says that this code is tested, so I suppose that it’s useful 😄.

By the way, it’s easier to have an html tag nested in another html tag, that’s another good reason to use is_for_root_element.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

have an html tag nested in another html tag

I tried that, too. Various scaring nestings of html and body. Creating multiple and/or nested BlockBox body is possible.
But always ended up with one single BlockBox html root element.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😱

@assert_no_logs
@pytest.mark.parametrize('html', (
'<html style="display: none">',
'<html style="display: none">abc',
'<html style="display: none"><p>abc',
'<body style="display: none"><p>abc',
))
def test_display_none_root(html):
box = parse_all(html)
assert box.style['display'] == 'block'
assert not box.children

@liZe
Copy link
Member

liZe commented Jun 21, 2020

We can rely upon the with stacked(context) statement to save and restore the context state automagically.

That’s good news.

Any idea how a unit-test would look like? Checking pixels of a write_png?

👍

@liZe
Copy link
Member

liZe commented Jun 22, 2020

Thank you, thank you, thank you…

@liZe liZe merged commit f333cef into Kozea:master Jun 22, 2020
@liZe liZe added this to the 52 milestone Jun 22, 2020
@Tontyna Tontyna deleted the clipped-margins branch June 22, 2020 23:15
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

Successfully merging this pull request may close these issues.

Page margins are broken with "body { overflow: auto }" style
2 participants