Inconsistent style in table cells with some monospaced text

[Larhzu]

Test case with the default stylesheet:

= Title

The `cook` command supports the following options:

[%autowidth]
|===
| `--cold`    | Allow `cook` to make cold foods.
| `--hot`     | Allow `cook` to make hot foods. Cannot be combined with `--cold`.
| `--sweet`   | Allow `cook` to make desserts. _{empty}_
|===

On the first row, cook doesn't have the gray background. On the other two rows it has the background.

It happens because of this:

p.tableblock > code:only-child {
  background: none;
  padding: 0;
}

I get that it can make the first column in the table look better. The current implementation makes semantics and style mix a bit though because one may need to add an empty extra element to get the correct style when there is also non-monospace content in the table cell.

It's only a minor thing that is easy to work around once one is aware of it. The small details just grab one's attention when everything else looks very polished already. :-)

[mojavelinux]

It happens because of this:

That selector is there for a reason. The background on an isolated code span in a table cell looks terrible, so the stylesheet intentionally overrides this.

On the first row, cook doesn't have the gray background.

When a column has only monospace text, it should have the m style instead of applying monospace to the text of every cell in it. See https://docs.asciidoctor.org/asciidoc/latest/tables/format-cell-content/#cell-styles-and-their-operators I mentioned that not just because it saves typing, but it's the primary reason why this CSS rule exists. If it doesn't, then the monospace style on a column causes the background to appear on all text in that column, which looks gaudy.

Unfortunately, based on the HTML that Asciidoctor produces, there's no way to differentiate between a normal table cell with a single code span and a monospace table cell. However, what I might consider is only applying this rule if the code span has no CSS class (no role). That would allow you to preserve the inline styling of the code span by adding a role.

| [.span]`--sweet`

The name of the role wouldn't matter. This seems like a reasonable compromise to consider.

[Larhzu]

That selector is there for a reason. The background on an isolated code span in a table cell looks terrible, so the stylesheet intentionally overrides this.

I get that and I didn't mean to suggest removing it. Sorry if my message could be interpret that way.

When a column has only monospace text, it should have the m style instead of applying monospace to the text of every cell in it.

Yes, I just felt the example was simpler this way. Both methods produce identical HTML.

However, what I might consider is only applying this rule if the code span has no CSS class (no role). That would allow you to preserve the inline styling of the code span by adding a role.

That would be prettier in the HTML output than _{empty}_ but otherwise these workarounds are somewhat equal in sense that user has to know about the stylesheet quirk.

What do you think about adding a class for table cells that use the monospace (m) style, and then changing the stylesheet to drop the background in this case?

I had never written any Ruby before but this change to html5.rb line 910 appears to do something useful (although I suspect it's not good as is since it only modifies the first <p>):

-                cell_content = (cell_content = cell.content).empty? ? '' : %(<p class="tableblock">#{cell_content.join '</p>
+                cell_content = (cell_content = cell.content).empty? ? '' : %(<p class="tableblock#{' monospaced' if cell.style == :monospaced}">#{cell_content.join '</p>

The CSS selector would then become p.tableblock.monospaced > code.

This wouldn't be perfectly backwards compatible because tables that don't use the m style (like in my first message) would get the background even if the cell only contains monospaced text. So I suppose this wouldn't be OK for v2.0.x. Could something like this be OK for 2.1.0? Somehow I fear I have missed something important.

[mojavelinux]

That would be prettier in the HTML output than {empty} but otherwise these workarounds are somewhat equal in sense that user has to know about the stylesheet quirk.

And I think they should because it is not a quirk, it's a design. A cell with just monospace text should be displayed not as a code span, but as monospace text. If they want it to appear as a code span, then they need to specify it as such. This is how you can get two different appearances. Logically, there's just no other way it can be.

What do you think about adding a class for table cells that use the monospace (m) style, and then changing the stylesheet to drop the background in this case?

That's a significant change (since it modifies the HTML) and I don't think we can rely on it until at least a minor version (as you suggested), perhaps even a major one (in the latter case, it would be more suitable for the new HTML converter in development).

With that said, I still think that a normal cell with a single code span should look the same as a monospace table cell. There are cases when you need to use that form to achieve something (such as to add a note). That's why I think using the role is the more adequate solution. It allows you to control which style you get all of the time.

[Larhzu]

I apologize if I'm wrong but I suspect that my main point hasn't been understood. In my first message, the first column of the table looks great without the gray background; I don't want to change the style in the first column. However, in the second column, there are three monospace cook strings but only two of them have a gray background.

I originally noticed this in a table where the cells had a few sentences of text. In one cell the gray background was missing from a monospaced word in the middle of a sentence. In the second cell, a monospaced word in a similar sentence did have the gray background. The generated HTML looked similar for both cells and I was confused for a moment. Then I realized that the word in the second cell had the gray background because another sentence in the same cell had an emphasized word in it. It wasn't intuitive that emphasizing a word in one sentence affects the style of a word in another sentence.

Is it an intentional and desired part of the design that only two of the three cook strings have a gray background in the example table?

If yes, then your suggestion about applying the style only when the code span has no role might be a good workaround (it's better than using _{empty}_ to avoid matching :only-child). It would still feel a bit sad considering how much attention to small details the design has in general.

With that said, I still think that a normal cell with a single code span should look the same as a monospace table cell.

I agree now. The idea in my previous message isn't good.

(The rest of this message was written with the assumption that the three cooks example is not desired behavior and that it's worth fixing.)

Below is another attempt at code. It would use the same p.tableblock.monospaced > code selector as my previous prototype.

--- a/lib/asciidoctor/converter/html5.rb
+++ b/lib/asciidoctor/converter/html5.rb
@@ -907,8 +907,8 @@ def convert_table node
               when :literal
                 cell_content = %(<div class="literal"><pre>#{cell.text}</pre></div>)
               else
-                cell_content = (cell_content = cell.content).empty? ? '' : %(<p class="tableblock">#{cell_content.join '</p>
-<p class="tableblock">'}</p>)
+                cell_content = cell.content.map {|e| %(<p class="tableblock#{' monospaced' if e.match?(/^<code[> ]((?!<\/code>).)*<\/code>$/m)}">#{e}</p>) }
+                cell_content = cell_content.join("\n")
               end
             end

It checks if a paragraph in a cell consist of a single <code> element. Other elements inside the <code> are allowed so that emphasis works:

| `command _argument_`

It checks that there is no </code> in the middle to keep the gray backgrounds if the content begins and ends in monospace text:

| `foo` or `bar`

That checks rejects nested <code> tags too but perhaps that's OK. The current stylesheet uses a gray background in that case but omits padding from the outermost <code> element. So there is a tiny difference in this odd corner case. Nested <code> can occur at least if a cell has m style and backticks are still used although I suspect this might be considered invalid AsciiDoc syntax:

m| `--option`

Changing the HTML output by adding the monospaced class shouldn't affect compatibility with old custom stylesheets. An old stylesheet just wouldn't get the style fix to the three cooks example.

A new stylesheet would have the new selector. If a new stylesheet was used with HTML from an old Asciidoctor version then unwanted gray backgrounds would appear.

I don't see other compatibility considerations now but it doesn't mean there aren't any.

Is this kind of change wanted at all? Is using a regex to match the <code> tags too hackish?

I apologize for being so insistent if my thoughts on this actually are on a wrong track. Thanks for reading!

[mojavelinux]

Is it an intentional and desired part of the design that only two of the three cook strings have a gray background in the example table?

Yes, because given the tools we have available in CSS, there's no way to distinguish between a code span that is the only child and a code span that is preceded or followed by text. It's a limitation of CSS. It thinks an element surrounded by only text is an only child (which is clearly wrong, but CSS is just broken in this case).

With my proposal, if you want to ensure there is a background, you have to add a role to the code that is surrounded by only text (for example, [.span]`code`)

Below is another attempt at code. It would use the same p.tableblock.monospaced > code selector as my previous prototype.

We could add a CSS class to the cell that is a monospace style, but that's a (minor) breaking change and can only be done in a minor or major release. This may be inconvenient, but it's just where we are.

Changing the HTML output by adding the monospaced class shouldn't affect compatibility with old custom stylesheets.

Changing the HTML itself is a breaking change.

[mojavelinux]

One possible docinfo workaround in the interim is to use JavaScript to add the empty span in the case both a code span and text are detected in the same cell. Sometimes, JavaScript is just needed to patch what CSS gets wrong.

[mojavelinux]

<script>
document.querySelectorAll('#content p.tableblock > code:first-child:only-child').forEach((el) => {
  if (el.parentNode.childNodes.length > 1) el.parentNode.appendChild(document.createElement('span'))
})
</script>

[mojavelinux]

I do understand what you're saying and I assure you that you're not being misunderstood. Your messages are clear. However, due to compatibility issues, the path forward is just not clear or easy. This is one of those situations where a CSS bug has broken an assumption made when designing the HTML (which was done over a decade ago). The assumption is that CSS can differentiate between a monospace cell and a default cell with monospace in it.

I think it's perfectly reasonable to add more information into the HTML to avoid this kind of situation, which we can do in the new HTML converter, as I mentioned.

[Larhzu]

It's a limitation of CSS.

Unfortunately yes.

Firefox has -moz-first-node and -moz-last-node which together would do the right thing, but it's useless here because it would be bad to have different appearence in different browsers. Here is some discussion about these selectors.

One possible docinfo workaround in the interim is to use JavaScript

Thanks! I will keep that in mind. The other workarounds are starting to feel OK now that I understand the overall issue better.

With my proposal, if you want to ensure there is a background, you have to add a role to the code that is surrounded by only text (for example, [.span]`code`)

In theory someone might have a document with [.green]`--green-option` in a table cell so adding :not([class]) to CSS would make the gray background appear. I suppose any change has some chance of backward incompatibility. For example, :not(.keep-background) has a small chance of namespace conflict.

Using [.span]#`code`# is two more characters to type. It increases the HTML size more but this needs no stylesheet changes. (_{empty}_ `code` has smaller HTML size but it's otherwise a little odd. __ __ `empty` would be the simplest to type but it affects asciidoctor-pdf output so this method shouldn't be used.)

Since there already are workarounds with similar-enough complexity as your suggestion, I guess it's not worth changing the stylesheet solely to add a new workaround variant.

Custom table roles can be a workaround in some cases. The current :only-child magic can be overriden for specific columns in docinfo:

table.keep-bg-1 > tbody > tr > td:nth-child(1) > p > code,
table.keep-bg-2 > tbody > tr > td:nth-child(2) > p > code,
table.keep-bg-3 > tbody > tr > td:nth-child(3) > p > code,
table.no-bg-in-first-only > tbody > tr > td:nth-child(n+2) > p > code {
  padding: .1em .5ex;
  background: #f7f7f8;
}

This may be nicer than needing to mark individual monospace strings in the cells.

We could add a CSS class to the cell that is a monospace style, but that's a (minor) breaking change and can only be done in a minor or major release. This may be inconvenient, but it's just where we are.

Hasty frequent changes would be far more inconvenient. :-) The stability of the HTML output is appreciated.

I think it's perfectly reasonable to add more information into the HTML to avoid this kind of situation, which we can do in the new HTML converter, as I mentioned.

Great! I know it will take time until the new HTML converter is out but it's good to hear that this detail has been taken into consideration.

Thanks!