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

Expectations for Bold functionality #1739

Open
stonebk opened this issue Apr 19, 2023 · 18 comments
Open

Expectations for Bold functionality #1739

stonebk opened this issue Apr 19, 2023 · 18 comments
Assignees
Labels
VivaEngageAsk Yammer team asks

Comments

@stonebk
Copy link

stonebk commented Apr 19, 2023

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

romanisa commented May 1, 2023

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

stonebk commented May 1, 2023

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

@romanisa @stonebk

  • It looks like the current styles aren't matching the spec, because none of the heading styles should be bold
  • I agree that heading styles are more semantic in HTML. However, if a Heading 1 has a font size of 16, then the font size selector for Heading 1 text should show a size of 16. By that same standard, if a Heading 1 is bold, then the Bold button should be highlighted if that text is selected.
  • I don't think style and bold are independent in other editors, it's just that their heading styles aren't bold by default. And ours shouldn't either, based on the spec.

@JiuqingSong
Copy link
Collaborator

@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.

  • If we treat it as bold, but there is actually no <b> tag inserted into the doc
  • 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.

Bold is a build in style of <h1>-<h6>. And we show it as bold due to #1224

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

@jvillalobos
Copy link
Contributor

jvillalobos commented May 4, 2023

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

JiuqingSong commented May 4, 2023

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

stonebk commented May 10, 2023

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

stonebk commented May 10, 2023

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

stonebk commented May 10, 2023

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

stonebk commented May 10, 2023

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

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

stonebk commented May 11, 2023

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

stonebk commented Aug 10, 2023

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

Let me see if I can get some time tomorrow

@JiuqingSong
Copy link
Collaborator

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

  • B tag
  • H1-H6 tag
  • STRONG tag
  • font-weight: bold/bolder
  • font-weight: 700+
  • Combination, e.g.
    • H1-H6 under B
    • B under H1-H6
    • font-weight: normal under B
    • font-weight: bold under H1-H6

For Italic

  • I tag
  • EM tag
  • font-style: italic

For Underline

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

For Strikethrough

  • S tag
  • STRIKE tag
  • text-decoration has line-through

For Superscript

  • SUP tag
  • vertical-align: super

For Subscript

  • SUB tag
  • vertical-align: sub

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 added a commit that referenced this issue Aug 11, 2023
@JiuqingSong
Copy link
Collaborator

I have a draft PR here #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
Copy link
Collaborator

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

stonebk commented Aug 21, 2023

@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

@romanisa romanisa added the VivaEngageAsk Yammer team asks label Oct 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
VivaEngageAsk Yammer team asks
Projects
None yet
Development

No branches or pull requests

4 participants