Heading elements and toolbar UI

[julienmonnard]

Hello! I was playing around with the editor, and I noticed a behaviour which I am not sure it is correct.

Plate.heading.behaviour.mov

I took a video of the behaviour, it's simpler than trying to explain it.
But to summarize, if the selection on a heading element is a range, the toolbar will wrongly indicate that it's a paragraph, which is confusing when we try to change the type.

Is it intentional or more likely a bug?

Thanks for your feedback 🙂

[12joan]

It certainly doesn't seem intuitive that expanding the selection within a single block should affect the turn-into menu. If you want to PR a fix, I would start by replacing the isCollapsed in turn-into-dropdown-menu.tsx with isRangeAcrossBlocks.

[julienmonnard]

Thanks for your answer! It helps, but if I use isRangeAcrossBlocks || isRangeInSameBlock.
I think this behaviour is overall a bit more complex than that though. As there is a lot of different ways to select and toggle these types.
For example, with the change above, if I select both the heading1 and the paragraph below, the element indicated in the button and in the dropdown will be "Heading 1", but then if I select "Paragraph" it does nothing, and if I select "Heading 1", it turns the whole selection into a paragraph.
This seems to be because the toggleNodeType function is checking if any of the selected nodes is matching the clicked button, and if yes, will consider the whole selection a being of that type, and will toggle it off

I think as a user, I would expect that no matter what types are in my selection, when I select an item in the dropdown menu, all my selection turns into that type.

So on top of having to replace isCollapsed with something else, I'm suspecting the toggleNodeType function to work well only as long as all the types in the selection are of the same type.

I didn't have time to deep-dive more in all this yet and it would take me some time before being able to free up enough time to do so

[12joan]

Isn't isRangeAcrossBlocks || isRangeInSameBlock pretty much always true? If that's what you want, we can just remove the condition.

I agree that the turn-into menu should work even for selections containing mixed block types. toggleNodeType should work for this case, I think?

[julienmonnard]

You're right, this || doesn't make much sense. And I start to understand what the idea behind the isCollapsed was. Basically, if only one type is selected, show it in the menu, else default to "Paragraph", because we don't know what to prioritize. So to keep the original intent, but make it work with range selection, it should probably be just isRangeInSameBlock. It still doesn't cover the case if multiple identical heading in different blocks are selected, but this is probably a very rare and unlikely case anyway.

Now for toggleNodeType, it's tricky. I spent a bit more time thinking about the alternative behaviours, and none feel really correct either, because it depends on the intent of the user. Was their goal to turn everything into the activeType or the opposite, to toggle it off for all? The current behaviour of toggleNodeType is the latter if the selection contains an activeType element, and the former if not.

So probably this behaviour should not be changed, because the confusion would still be here anyway, just maybe the other way around, depending on what the user's expectation is when the click happens.

TL;DR Probably replace isCollapsed with isRangeInSameBlock and leave the toggleNodeType behaviour as it is 🙂