Reintroduce non leaf inline and block accessors to value

[outreach-soren]

Is this adding or improving a feature or fixing a bug?

Improving and adding.

What's the new behavior?

This reverts value.inlines and value.blocks to not filtering for only leaf nodes and adds specific value.leafInlines and value.leafBlocks.

The new behavior is that value.inlines and value.blocks return all respective nodes of those types and value.leafInlines and value.leafBlocks have been added to access leaf nodes specifically.

How does this change work?

This issue has a few different levels and I think this change is the most straight-forward way to resolve them. The main issue is that currently if the selection is in a node that is not a leaf of its type in the document tree, but the selection does not include the actual leaf nodes, value.inlines or '.blocks' will be completely empty even though the selection contains nodes of those types.

This is due to this line here where we check whether the node is a leaf node, but don't give the context of the desired range. Thus the described behavior of "bottom-most {nodes of type} in a range" is not really accurate, as it will only return inlines if they are leaf nodes in the context of the document tree as a whole, not just for the selection.

An alternate approach to make this behavior less confusing would be to pass the context of the range to the check for whether the node is a leaf node or not, but I think this raises some questions about the nature of leaf nodes and whether the context of a range should affect whether a node is considered a leaf node or not. Can a node be a leaf for just a current range? To me that seems kind of unintuitive. For example, if getLeafInlinesAtRange performed as it's described and returned the "bottom-most" nodes in the range then we could likely be returning "leaf" nodes that have children of their same object type in the document tree as a whole which seems very unintuitive.

Thus I think the solution that makes the most sense is to keep the functionality for getLeafInlinesAtRange as it is but not use it to source inlines for value.inlines, and instead add a separate property leafInlines to the value model. I think this should resolve confusion going forward.

On a higher level design note I also think it just plain doesn't make sense to have value.inlines and value.blocks make non-explicit assumptions about what sort of nodes are desired. Even without adding .leafInlines or .leafBlocks if the user wants only leaf or root nodes of those types than they can filter them as they wish! It is unintuitive to have a generic .inlines property and then only have it return a subset of the inlines.

Have you checked that...?

• The new code matches the existing patterns and styles.
• The tests pass with yarn test.
• The linter passes with yarn lint. (Fix errors with yarn prettier.)
• The relevant examples still work. (Run examples with yarn watch.)

Does this fix any issues or need any specific reviewers?

🤷‍♂

[outreach-soren]

@ianstormtaylor could I get some eyes on this?

[cameracker]

I'm having some trouble understanding this review, and I think it's because the term "leaf" in slate has historically referred to a node type that contains text and marks, and now "leaf" is taken to mean the bottom most nodes in the tree (like the standard leaf term).

As leaves have been removed, I can't say it's necessarily a bad thing to repurpose the term, but I do worry about folks who are having to do really big upgrades suddenly finding that leafs are no longer leaf nodes, but might actually be blocks and inlines.

Other than that, I think this functionality is useful and likely worthwhile to add to the API, but the name makes me a little wary

[outreach-soren]

Hey @CameronAckermanSEL thanks for the feedback. To give some context the main issue that this resolves for me is that prior to the change in this scenario -

<inlineOne>
  <anchor/>
  <text>sometext</text>
  <focus/>
  <inlineTwo/>
</inlineOne>

value.inlines would be empty.
after the change value.inlines would contain inlineOne which is what I would expect to happen based on the current description of "lowest depth inlines in the current selection".

[ianstormtaylor]

Hey @outreach-soren thank you for taking the time to work on this. Given the work I've been doing in #2809 though, I'm unsure I want to merge this as it takes us further in a direction I am moving away from. All of the getters on Value objects are going to be deprecated. And all of the get*{Inlines,Blocks} will be deprecated too. We're moving towards only using a handful of utility functions that implement the Iterable API to make things simpler.

[outreach-soren]

@ianstormtaylor ahh ok. I was not aware of that larger change. Looking forward to seeing that happen then. Thanks!

[ianstormtaylor]

Fixed by #3093.