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

List item markers excluded from hiding and styling #619

Open
PgBiel opened this issue Apr 6, 2023 · 11 comments
Open

List item markers excluded from hiding and styling #619

PgBiel opened this issue Apr 6, 2023 · 11 comments
Labels
bug Something isn't working styling About set and show rules or style properties

Comments

@PgBiel
Copy link
Contributor

PgBiel commented Apr 6, 2023

(Possibly related to #520 and #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 085282c)): (notice how markers were unaffected when they were supposed to be, when attempting to hide/style individual items)

image

@PgBiel
Copy link
Contributor Author

PgBiel commented Apr 6, 2023

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

fn finish(self) -> (Content, StyleChain<'a>) {
let (items, shared) = self.items.finish();
let item = items.items().next().unwrap();
let output = if item.is::<ListItem>() {
ListElem::new(
items
.iter()
.map(|(item, local)| {
let item = item.to::<ListItem>().unwrap();
item.clone().with_body(item.body().styled_with_map(local.clone()))
})
.collect::<Vec<_>>(),
)
.with_tight(self.tight)
.pack()
} else if item.is::<EnumItem>() {
EnumElem::new(
items
.iter()
.map(|(item, local)| {
let item = item.to::<EnumItem>().unwrap();
item.clone().with_body(item.body().styled_with_map(local.clone()))
})
.collect::<Vec<_>>(),
)
.with_tight(self.tight)
.pack()
} else if item.is::<TermItem>() {
TermsElem::new(
items
.iter()
.map(|(item, local)| {
let item = item.to::<TermItem>().unwrap();
item.clone()
.with_term(item.term().styled_with_map(local.clone()))
.with_description(
item.description().styled_with_map(local.clone()),
)
})
.collect::<Vec<_>>(),
)
.with_tight(self.tight)
.pack()
} else {
unreachable!()
};
(output, shared)
}

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
Copy link
Contributor Author

PgBiel commented Apr 6, 2023

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
Copy link
Contributor Author

PgBiel commented Apr 6, 2023

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.

@PgBiel PgBiel changed the title List item markers excluded of hiding and styling List item markers excluded from hiding and styling Apr 6, 2023
@laurmaedje
Copy link
Member

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

@PgBiel
Copy link
Contributor Author

PgBiel commented Apr 6, 2023

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:

  • Source code:
- *My list*
- #text(blue)[This is blue]

Compared to

*- My list*
#strong[- My list]
#text(blue)[- My list]
  • Output (my patch):
    image

  • Output (without my patch):
    image

@laurmaedje
Copy link
Member

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.

@laurmaedje laurmaedje added bug Something isn't working styling About set and show rules or style properties labels Apr 6, 2023
@PgBiel
Copy link
Contributor Author

PgBiel commented Apr 6, 2023

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
Copy link
Contributor Author

PgBiel commented Apr 6, 2023

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: PgBiel@fa91dd7). 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
Copy link
Member

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
Copy link
Contributor Author

PgBiel commented Apr 6, 2023

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 👍

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
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working styling About set and show rules or style properties
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants