typst / typst

A new markup-based typesetting system that is powerful and easy to learn.
https://typst.app
Apache License 2.0
32.82k stars 881 forks source link

List item markers excluded from hiding and styling #619

Open PgBiel opened 1 year ago

PgBiel commented 1 year ago

(Possibly related to https://github.com/typst/typst/issues/520 and https://github.com/typst/typst/pull/587)

Lists (bulleted and enumerated, but not term-lists) seem to not be hiding/styling correctly. More specifically, you can't hide/style list items directly fully - styling their parent element (list or enum) seems to work (which styles/hides all items), but not when the listitem itself is affected (that is, attempting to style just one item), as the list item marker remains unaffected. For example, typing #hide[- b] among other elements in the same list will produce a hidden b, but the marker stays there. Likewise, #text(blue)[- b] only makes the "b" blue, but not the marker. However, #hide[#list[b]] will work, as that creates a separate list. (The same applies for enumerated lists, but not for term lists.)

I believe this is due to the fact that the list and enum types handle all markers by themselves, while term lists let their items handle their own markers (the term labels). I will attempt to elaborate further in another comment below (first, I'll show a reproducible example).

Here's sample typst code to reproduce this issue:

ff
#repr[- a]
#repr[#list[a]]

- f
#list["a"]
#hide[#list["a"]]
- #hide[a]
#hide[- a]
- g
#text(blue)[- i]
- h

ff

#hide[+ a]
#text(blue)[+ b]
+ b
+ c
#hide[#enum[d]]
+ e

/ a: b
/ c: d
#hide[/ d: e]
/ f: g
#hide[#terms(([a],[b]))]
/ e: h

This will produce (on latest main (at commit 085282c138899dd5aaa06bc6ae7bd2f79d75d7e1)): (notice how markers were unaffected when they were supposed to be, when attempting to hide/style individual items)

image

PgBiel commented 1 year ago

So, after doing some experiments, I believe (part of) the cause is located here, in ListBuilder:

https://github.com/typst/typst/blob/085282c138899dd5aaa06bc6ae7bd2f79d75d7e1/library/src/layout/mod.rs#L553-L600

When the individual list items are iterated over, for bulleted and enumerated lists, their styles (which seem to be the local variable in each iteration) are applied on the items' bodies only, while, for term lists, the style is applied to both the term (label) and the description (the equivalent of the "body").

This is because bulleted and enumerated list items only hold their own body, not their markers - markers are created by the list / enum elements when rendering at the end, such that they cannot be aware of the individual styles of each item.

Perhaps this could be solved by letting the individual items hold their styles in some way. This could also be solved by allowing the list items to hold their own markers (and, frankly, it makes sense to be able to customize markers for individual items), but that might be out of the scope of this issue (maybe another issue could be opened to discuss that possibility).

I'm willing to create a PR to fix this (I found out this info while trying to do so, actually, hehe).

PgBiel commented 1 year ago

Regarding the above, I did a quick patch to include a styles field for ListItem, and can confirm it does solve the problem entirely, for bulleted lists (as an example). Something like this:

diff --git a/library/src/layout/list.rs b/library/src/layout/list.rs
index 6cb1bc7e..911de955 100644
--- a/library/src/layout/list.rs
+++ b/library/src/layout/list.rs
@@ -133,7 +133,7 @@ impl Layout for ListElem {
         let mut cells = vec![];
         for item in self.children() {
             cells.push(Content::empty());
-            cells.push(marker.clone());
+            cells.push(marker.clone().styled_with_map(item.styles(styles)));
             cells.push(Content::empty());
             cells.push(item.body().styled(Self::set_depth(Depth)));
         }
@@ -165,6 +165,11 @@ pub struct ListItem {
     /// The item's body.
     #[required]
     pub body: Content,
+
+    /// The item's specific styles,
+    /// to be applied to its marker.
+    #[internal]
+    pub styles: Styles,
 }

 cast_from_value! {
diff --git a/library/src/layout/mod.rs b/library/src/layout/mod.rs
index d12ce158..9162f888 100644
--- a/library/src/layout/mod.rs
+++ b/library/src/layout/mod.rs
@@ -559,7 +559,10 @@ impl<'a> ListBuilder<'a> {
                     .iter()
                     .map(|(item, local)| {
                         let item = item.to::<ListItem>().unwrap();
-                        item.clone().with_body(item.body().styled_with_map(local.clone()))
+
+                        item.clone()
+                            .with_styles(local.clone())
+                            .with_body(item.body().styled_with_map(local.clone()))
                     })
                     .collect::<Vec<_>>(),
             )

will produce the desired output for the bulleted lists part of the reproduction example: image

PgBiel commented 1 year ago

So, the questions left are: Is this approach correct and/or appropriate? Should we use another name for that 'styles' field? Should we store the styles somewhere else (instead of the item structs themselves)? I'll gladly make a PR once there is a better idea of the best way to do this.

laurmaedje commented 1 year ago

How does your change behave if you have - *bold*? is the marker bold or does it work?

PgBiel commented 1 year ago

How does your change behave if you have - *bold*? is the marker bold or does it work?

Any styles applied directly to the body do not affect the marker, as seen below (upon applying my patch) - the marker is only affected when the entire list item is:

Compared to

- My list

strong[- My list]

text(blue)[- My list]



- Output (my patch):
![image](https://user-images.githubusercontent.com/9021226/230400861-208b8c5f-9bb0-41d2-a716-f306080b1de8.png)

- Output (without my patch):
![image](https://user-images.githubusercontent.com/9021226/230400946-261227b7-27b1-476d-9b15-efb444e5f55d.png)
laurmaedje commented 1 year ago

Instead of having the styles field, it might be cleaner to just apply the styles to the whole list item (with .styled()) and then in the list code apply the styles if necessary. You can look into meta/document.rs or layout/flow.rs where similar things are happening.

PgBiel commented 1 year ago

Instead of having the styles field, it might be cleaner to just apply the styles to the whole list item (with .styled()) and then in the list code apply the styles if necessary. You can look into meta/document.rs or layout/flow.rs where similar things are happening.

I see. The difference here seems to be that document and flow have a vector of Content as their children, which allow using styled like that. A list, however, has a vector of ListItem, that is, the already converted Content. And, from my tests, it seems that, if we use .styled() before converting the item to a ListItem (which is what makes it possible for it to be a ListElem child), it cannot be converted to a ListItem anymore, as it becomes a StyledElem. (Of course, you could turn it back to a ListItem, but then it would lose its style properties.)

So, maybe the solution is to have ListElem (and also for enum) to have a vector of Content as children (which would allow us to style them beforehand), and then do the conversion to ListItem only when the list itself is rendered (in ListElem::layout), using to_styled(), just like document and flow do it. Does that sound correct?

PgBiel commented 1 year ago

Alright, I got a working fix with that approach. Worth saying, though, that, when the list/enum items aren't provided using the shorthand syntax (- item or + item), but rather with the direct functions (#list[something] or #enum[something]), the content is converted to (wrapped in) ListItem / EnumItem inside the children loop in ListElem::layout / EnumElem::layout (see my commit here: https://github.com/PgBiel/typst/commit/fa91dd7c9d8c35fbe0d1dc629df5a0ba3cac036a). Is there a way to use #[parse(...)] with #[variadic] such that it converts Contents to ListItem / EnumItem, but keeps them type-erased, such that the ListElem/EnumElem children are converted to ListItem/EnumItem from the start?

laurmaedje commented 1 year ago

That's true, I didn't think of that. Changing to content seems okay, in principle. Note that you don't even have to construct the enum item / list item in the children loop since they are immediately deconstructed into their parts anyway.

PgBiel commented 1 year ago

Note that you don't even have to construct the enum item / list item in the children loop since they are immediately deconstructed into their parts anyway.

Oh wow, I don't know how I missed that, haha! I'll work on it, thank you :+1:

I guess the only significant change then will be to allow children other than ListItem/EnumItem for lists/enums, but that shouldn't be that bad of an issue anyway (since they are only used for shorthand-syntax lists, and all they do is hold their bodies)

CtrlC-Root commented 1 month ago

Purely for styling purposes my work-around for now is to create separate lists without any space between them:

#list(marker: [#text(red)[#sym.circle.filled]])[
  First item.
]
#list(marker: [#text(green)[#sym.circle.filled]])[
  Second item.
]

It would be great if I could do this through the #list() function's marker field using a function instead.