microsoft / roosterjs

roosterjs is a framework-independent javascript rich text editor.
Other
1.15k stars 156 forks source link

Expectations for Bold functionality #1739

Open stonebk opened 1 year ago

stonebk commented 1 year ago

Describe the bug

We are currently trying to add support for headings and we noticed that when you toggle a heading, bold is also toggled.

rooster bold heading

To Reproduce

Steps to reproduce the behavior:

  1. Go to the RoosterJs Demo Site
  2. Add a heading
  3. Notice the Bold icon is highlighted

Expected behavior

My expectations are that headings and bold are two different states -- heading is a block state and bold is an inline state. The two states are stylistically similar, but they are semantically different. Adding a new heading should only enable the heading state and not the bold state (although a user can choose to also toggle the bold state for all or part of the heading if they so choose).

Additional context

I think styles and semantics are being conflated here. Visually, headings might appear to be bold, but that is a presentational concern of the heading element. Some clients, for example, might choose to render headings as semibold, normal, or thin. Some clients might even choose to give headings an underline (like the headings in this GitHub bug report). The format state for headings shouldn't be based on any of these presentational styles, rather it should be based on the semantic markup.

Here is an example where I change the default styles of the h1 element:

demonstration

Notice that the heading no longer appears to be bold but bold is toggled in the toolbar. The heading also appears to be italic and underline, but those toolbar icons are not toggled (as I would expect).

Prior art

I've checked this behavior in many other JavaScript rich text editors and they all treat bold as a separate state from heading. Note that not all of them will have a visual distinction, but the markup differentiates the two.

I also tested the behavior in some other clients, and they also treat bold as a separate state from heading.

Microsoft Word Screen Shot 2023-04-19 at 1 05 27 PM

Google Docs Screen Shot 2023-04-19 at 1 06 44 PM

romanisa commented 1 year ago

@jvillalobos What are your thoughts on this? Today "Bold" control is shown as selected for Heading because Heading style has bold. We can make the change as asked here, but if we do that and then user selects "Bold", it will be a no-op.

stonebk commented 1 year ago

@jvillalobos What are your thoughts on this? Today "Bold" control is shown as selected for Heading because Heading style has bold. We can make the change as asked here, but if we do that and then user selects "Bold", it will be a no-op.

Note, it will only be a visual no-op, and that is only if headings use the default bold styling -- for systems that use anything else, like semibold, the bold would visually be distinct. Semantically, selecting bold would never be a no-op.

jvillalobos commented 1 year ago

@romanisa @stonebk

JiuqingSong commented 1 year ago

@jvillalobos , I think this issue is about another topic. The problem is, for a <H1> tag (without any other styles), should we treat it as bold? Should we highlight bold button when focus on it? Because when text is under <h1> (same with h2-6), the heading text is actually showing as bold, and user can actually unbold it.

Bold is a build in style of <h1>-<h6>. And we show it as bold due to https://github.com/microsoft/roosterjs/pull/1224

So my suggestion is to provide a way to allow customizing this behavior.

jvillalobos commented 1 year ago

for a <H1> tag (without any other styles), should we treat it as bold?

I think that depends on the styling that is associated to that tag. If an H1 is supposed to have bolded text, then Rooster should reflect that and show the text as being bold, so a user can remove the bold from it (while keeping the H1). If the H1 isn't bold, then it shouldn't appear as bold and the user should be able to make it bold in addition to it being an H1. The same would be true for italics and other styles.

I expect different consumers of Rooster could have different styles for headings, so that's something we should consider making configurable, if that isn't the case already.

JiuqingSong commented 1 year ago

@jvillalobos For a simple H1 tag, it doesn't have explicit B tag in HTML, but it will show as bold. This is the build-in behavior or H1 (as well as H2,..., H6). For example

<h1>test</h1>

See it only has H1 tag without any other tags and styles. It will be rendered as: image

And if user select the text and unbold (existsing behavior in roosterjs), The HTML will change to:

<h1><span style="font-weight: normal;">test</span></h1>

And the render result will be: image

That is what I mean for "whether we should highlight bold button for header", but not for "having different styles for headings". So let's only discuss about pure H1 tag without any other styles. And for the original issue, it has nothing to do with the "style and heading" feature of roosterjs, but just for pure HTML H1 tag.

stonebk commented 1 year ago

@JiuqingSong

For a simple H1 tag, it doesn't have explicit B tag in HTML, but it will show as bold. This is the build-in behavior or H1 (as well as H2,..., H6).

Bold style for an H1 tag is not universal though, it is entirely dependent on the reset and styles the page is using. Look at the H1 for this page:

Screen Shot 2023-05-10 at 10 15 46 AM

Github uses a 400 font-weight for its H1, so setting font-weight: normal on this heading does not change anything. However, if I were to set this h1 to bold, you will see that it renders differently:

Screen Shot 2023-05-10 at 10 17 30 AM

This is why we need to treat headings and bold as separate states -- we can't assume that headings are always visually bold.

stonebk commented 1 year ago

Regarding #1224, I don't think that is a bug at all, rather it is expected behavior. Again, since headings aren't necessarily styled as bold, we can't assume that isBold should be true.

stonebk commented 1 year ago

If we do not treat it as bold, what should we do if user select some text and click bold button? Adding <b> tag doesn't have any visual change.

FWIW, this is how every other editor works.

I don't think it should be up to Rooster to determine if the heading is bold or not, rather I think it should be up to the page to determine the styling for headings and bold headings.

stonebk commented 1 year ago

I think at a high level, Rooster can't be making assumptions about visual styles because every page can render tags differently. Rooster is not an editor of CSS styles, it is an editor of semantics tags.

Maybe another way to think about this is to think of this from the perspective of a screen reader. A screen reader would not announce these tags any differently:

<h1>hello world</h1>
<h1><span style="font-weight: normal;">hello</span> world</h1>

A screen reader might however decide to announce these tags differently:

<h1>hello world</h1>
<h1><b>hello</b> world</h1>

Yes it's uncommon for screen readers to announce <b> (not uncommon for <strong>), however, some screen readers (such as JAWS) can be configured to point out bold.

JiuqingSong commented 1 year ago

I think the best way to make everyone satisfied is to allow this behavior to be customized.

We can simply provide a boolean flag to disable all implicit format, or we can also allow provide a customized getFormatState() function so that you can customize all formats.

stonebk commented 1 year ago

I think the best way to make everyone satisfied is to allow this behavior to be customized.

We can simply provide a boolean flag to disable all implicit format, or we can also allow provide a customized getFormatState() function so that you can customize all formats.

Sure, we can work with that. I would really prefer not to have to write a custom getFormatState to achieve our expected behavior though -- I truly believe that the current behavior is less correct than what we are suggesting. A flag to disable implicit formatting sounds pretty great.

stonebk commented 1 year ago

@JiuqingSong wanted to check in and see if there has been any progress on this? Is there a flag now to disable implicit formatting?

JiuqingSong commented 1 year ago

Let me see if I can get some time tomorrow

JiuqingSong commented 1 year ago

@stonebk do you have a detailed list of the expected result? For Bold, do you expect true or false result for the situation below:

For Italic

For Underline

For Strikethrough

For Superscript

For Subscript

In general, all these conditions can affect these styles visually, so we need a functional spec to define what is the expected result then we can change the code, and write test cases according to the spec.

JiuqingSong commented 1 year ago

I have a draft PR here https://github.com/microsoft/roosterjs/pull/2021 to only ignore H1-H6 for bold, but keep others not changed. Please review if that is ok, or please fill the answers for questions above so that I can make change accordingly.

JiuqingSong commented 1 year ago

@stonebk Would you please take a look at #2021 and test it if possible? See if it has the expected result, or please let me know what is not expected.

stonebk commented 1 year ago

@JiuqingSong sorry for the slow response!

I think true/false should be based on the presence of explicit tags and not implicit styles, i.e. ignore inline styles, and user agent styles. These would be my expectations:

For Bold

true in the presence of <B> or <STRONG>, false otherwise

true

B tag STRONG tag

H1-H6 under B (because of the presence of B) B under H1-H6 (because of the presence of B) font-weight: normal under B (because of the presence of B)

false

H1-H6 tag font-weight: bold/bolder font-weight: 700+ font-weight: bold under H1-H6

For Italic

true in the presence of <I> or <EM>, false otherwise

true

I tag EM tag

false

font-style: italic

For Underline

true in the presence of <U>, false otherwise

true

U tag

text-decoration none under U tag (because of the presence of U)

false

text-decoration has underline A tag text-decoration underline under A tag text-decoration none under A tag

For Strikethrough

true in the presence of <S>, <STRIKE>, and <DEL>, false otherwise

true

S tag STRIKE tag

false

text-decoration has line-through

For Superscript

true in the presence of <SUP>, false otherwise

true

SUP tag

false

vertical-align: super

For Subscript

true in the presence of <SUB>, false otherwise

true

SUB tag

false

vertical-align: sub