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

Implement smart block (bottom) margins #1515

Closed
mojavelinux opened this issue Jan 24, 2020 · 5 comments
Closed

Implement smart block (bottom) margins #1515

mojavelinux opened this issue Jan 24, 2020 · 5 comments
Assignees
Milestone

Comments

@mojavelinux
Copy link
Member

mojavelinux commented Jan 24, 2020

Block margins should only be applied between block elements. The simplest way to do this is to use top margins and only apply them if the block is not the first child. (We can come back later and implement bottom margins as an alternative).

The only tricky part are list items. List items should have a built-in line-height, so the spacing is still applied below the last item. Additionally, more space should be applied between the end of a complex block and the start of the next list item. We can try to match what we have now, though more control may be needed in the future.

One of the benefits of this change is that a block inside of another blocks (such as a block inside of an AsciiDoc table cell) will no longer have a gap below it.

@mojavelinux mojavelinux added this to the v2.0.0 milestone Jan 24, 2020
@mojavelinux mojavelinux self-assigned this Jan 24, 2020
@mojavelinux
Copy link
Member Author

Since this change will likely break existing themes that customize block margins, it will need to be scheduled for 2.0.0.

@mojavelinux
Copy link
Member Author

It may be possible to support smart margins on either side, which would mean we can avoid breaking compatibility. Regardless, it will still affect some themes since there would no longer be extra space below blocks nested inside of other blocks.

@mojavelinux
Copy link
Member Author

mojavelinux commented Apr 8, 2022

After thinking a lot about how this would work, we've decided to focus on using the bottom margin of a block to strategically control the space between blocks. Before applying the bottom margin, the converter will determine whether the block has a next block within the same enclosure (i.e., next enclosed block). Only if it does will the bottom margin be applied to offset the block from the next adjacent block.

The boundary of an enclosure, as defined in this context, is where we would want the bottom margin of the last block to be shaved off so we don't end up with a compounding margin. For example, a sidebar is an enclosure. A top-level list is also an enclosure. The enclosure itself will have its own bottom margin. That's why a top-level list is considered an enclosure, but not a nested list.

A section is not considered an enclosure in this context. That's because the next section can be a sibling, parent, or ancestor of the current block. Instead, the margin handling will consider when a block is followed by a section, then add a consistent margin (currently the block_margin_bottom value) to the bottom of that block so the spacing above a section is stable.

A cornerstone of the implementation for this issue is the logic to compute the next enclosed block. That logic will need to be properly unit tested to ensure that we are covering all likely permutations.

@mojavelinux
Copy link
Member Author

mojavelinux commented Apr 8, 2022

Here's some more background on how we arrived at this approach:


First, we reviewed the existing logic to confirm what margins were being added or collapsed. As expected, the logic is very inconsistent and, in many cases, uses guesswork patches to achieve the right spacing.

Ordered and unordered lists, especially when getting into nested lists, is particularly complicated. For example, a list item with primary content only (not multi-paragraph or block content) uses different spacing than a list item with paragraphs or blocks. This spacing complexity expands when one considers the content combinations of (great-great, great-, grand)parent, sibling, and child list items, and whether a list item is the last item in a list. As a person viewing a list, it's easy to identify the last list item in a list (i.e., the list terminus) visually. However, programmatically, the converter has to work through the AST until it identifies the greatest parent of an item and then that there are no other parents, siblings, or children list items following what may be the last list item. Description lists also have unique spacing requirements that include nested items and the possibility of terms without descriptions.

Then, we worked through what operations the converter would do to determine:

  • What object owns a margin, assuming we apply bottom margins only?
  • Which bottom margins should get collapsed depending on the next enclosed block?

For example, when a list item has complex content and is the last item in a list and the next object is a section heading is:

  • the bottom margin of the last block that is part of the list item collapsed?
  • the list item's bottom margin collapsed?
  • the list's bottom margin collapsed?
  • none of the bottom margins collapsed?

We then did the same exercise using top margins only. Both exercises included discussing what the possible results and problems would be if, in the future, we added keys for controlling some of the margins.

After working through a number of scenarios, we came up with the following plan:

  1. Implement smart bottom margins
    a. Add new unit tests and update existing tests; our goal is to match the current spacing as closely as possible so the disruption to existing themes is minimal (but there will be visual changes when people switch to 2.0)
    b. Removing bottom padding hacks in default theme; padding can now be applied consistently around the content
    c. Remove top margins (since they don't take into account the context and thus behave inconsistently)
  2. Implement more consistent list item spacing for ordered, unordered, and description lists
    a. Determine if compute+compare or recursion is more suitable for determining list terminus
    b. Ensure that any application of a deferred bottom margin (i.e., dlist terms that don't have a description but are followed by another term) doesn't break the arrange_block logic.
  3. In a future issue, implement smart top margins using a parallel strategy

@mojavelinux
Copy link
Member Author

Here's a list of the bottom margins that need to be adjusted and reviewed:

  • admonition
  • example
  • sidebar
  • quote/verse (also margin below last block preceding attribution line)
  • listing/literal
  • table
  • ulist/olist (nested lists need work)
  • dlist
  • dlist (horizontal; a variation of a table)
  • dlist (qanda)
  • colist
  • preamble (basic)
  • abstract
  • styled paragraphs (sidebar, example, admonition, etc)
  • thematic break
  • image (also handles video w/ automatic image)
  • audio
  • video (with poster)
  • last child in open block (should look as though open block is not there)
  • AsciiDoc table cell
  • discrete heading (margin below last preceding block)
  • section title (margin below last preceding block)
  • footnotes (top margin)

We need to look closely at nested list permutations as well. We might not get them all the first time, but the existing tests should still pass with any necessary modifications.

@mojavelinux mojavelinux changed the title Implement smart block margins Implement smart block (bottom) margins Apr 13, 2022
mojavelinux added a commit to mojavelinux/asciidoctor-pdf that referenced this issue Apr 13, 2022
…vent extra space below blocks, particularly nested blocks
mojavelinux added a commit to mojavelinux/asciidoctor-pdf that referenced this issue Apr 13, 2022
…vent extra space below blocks, particularly nested blocks
mojavelinux added a commit to mojavelinux/asciidoctor-pdf that referenced this issue Apr 14, 2022
…vent extra space below blocks, particularly nested blocks
mojavelinux added a commit to mojavelinux/asciidoctor-pdf that referenced this issue Apr 14, 2022
…vent extra space below blocks, particularly nested blocks

additionally,

resolves asciidoctor#1513
resolves asciidoctor#1845
mojavelinux added a commit to mojavelinux/asciidoctor-pdf that referenced this issue Apr 14, 2022
…vent extra space below blocks, particularly nested blocks

additionally,

resolves asciidoctor#1513
resolves asciidoctor#1845
mojavelinux added a commit to mojavelinux/asciidoctor-pdf that referenced this issue Apr 14, 2022
…vent extra space below blocks, particularly nested blocks

additionally,

resolves asciidoctor#1513
resolves asciidoctor#1845
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants