Bug 25086 – Can't unbold bolded list item when list is surrounded by <b> tag

• Component: HTML Editing
• Status: RESOLVED FIXED

[Annie Sullivan]

Steps to reproduce:
1. Go to midas demo: http://www.mozilla.org/editor/midasdemo/
2. Enter the following html:
<b><ul><li><b>a</b></li></ul></b>
3. Uncheck "View HTML Source" and highlight "a".
4. Press Bold button.

Actual result:
On first press of bold button, only inner <b> is removed. No visual change. Resulting HTML:
<b><ul><li>a</li></ul></b>
On second press of bold button, a font-weight: normal span is applied to the text. It looks un-bolded, but this adds a lot of cruft to the html:
<b><ul><li><span class="Apple-style-span" style="font-weight: normal;">a</span></li></ul></b>

Expected result:
<b> tag surrounding list should be removed (this is what Firefox does):
<ul><li>a</li></ul>

[Eric Seidel (no email)]

You should definitely be able to unbold the text, but I'm not sure we should remove the bold tags around the <li>, since they're outside of the current selection, and I think we generally try to avoid modifying content outside of the current selection when not explicitly required by the execCommand.

[Ryosuke Niwa]

The given HTML is malformed for which we're not responsible.

[Ryosuke Niwa]

WebKit produces visually correct markup. Of course, it'll be nice if we could remove b tag and fixes invalidity of the document, but since we don't make HTML less valid (the spec. requires this much), WebKit's behavior should be acceptable.

[Ojan Vafai]

I think we do have to be responsible for malformed markup due to copy-paste and innerHTML injection.

The correct user-visible behavior is clear. The first time you hit the bold button, the text should unbold. The second time you hit the bold button, it should become bold again.

In terms of markup, I don't see why we shouldn't remove the bold tag around the UL since all it's text is contained in the selection. Here's a more general example though:

<b>
  <ul>
    <li>foo</li>
    <li>bar</li>
  </ul>
</b>

If you select the "f" and then execCommand('bold'), I think the ideal behavior would be to end up with the following markup:
<ul>
  <li>f<b>oo</b></li>
  <li><b>bar</b></li>
</ul>

The inline bold tag should be brought into the blocks that it wraps and then we should unbold like we normally do for valid markup.

I'm open to arguments for other markup though. It's hard to say what markup is "correct" here.

[Ryosuke Niwa]

> In terms of markup, I don't see why we shouldn't remove the bold tag around the
> UL since all it's text is contained in the selection.

But I don't see why we must insist on removing b either.  Of course, if it was inside LI, then we should definitely remove it.

>Here's a more general
> example though:
> 
> <b>
>   <ul>
>     <li>foo</li>
>     <li>bar</li>
>   </ul>
> </b>
> 
> If you select the "f" and then execCommand('bold'), I think the ideal behavior
> would be to end up with the following markup:
> <ul>
>   <li>f<b>oo</b></li>
>   <li><b>bar</b></li>
> </ul>
> 
> The inline bold tag should be brought into the blocks that it wraps and then we
> should unbold like we normally do for valid markup.

So you're suggesting that we treat b and other things more like text decoration in that we remove all b ancestors, and reapply them.  I think, then we should preprocess the HTML.  e.g. we can first convert:
<b>
  <ul>
    <li>foo</li>
    <li>bar</li>
  </ul>
</b>
to
<ul>
  <li><b>foo</b></li>
  <li><b>bar</b></li>
</ul>
Then WebKit does what user expects to do.

[Justin Garcia]

Do any editors make this kind of markup?

[Annie Sullivan]

(In reply to comment #6)
> Do any editors make this kind of markup?

I've seen this HTML in several Google Docs where the user had not hand-edited the HTML, and the doc had only been edited in WebKit (Chrome/Safari). Google Docs JS code doesn't ever generate HTML like this, but I can't repro exactly how the HTML got there. My best guess is copy/paste, since I've seen similar things happen on copy/paste like in bug 26937.

[Ryosuke Niwa]

(In reply to comment #7)
> I've seen this HTML in several Google Docs where the user had not hand-edited
> the HTML, and the doc had only been edited in WebKit (Chrome/Safari). Google
> Docs JS code doesn't ever generate HTML like this, but I can't repro exactly
> how the HTML got there. My best guess is copy/paste, since I've seen similar
> things happen on copy/paste like in bug 26937.

We should fix 26937 instead then.  Because this behavior (not deleting b outside of ul) doesn't seem to be a problem if we didn't produce invalid HTML in the first place.

[Annie Sullivan]

(In reply to comment #8)
> (In reply to comment #7)
> > I've seen this HTML in several Google Docs where the user had not hand-edited
> > the HTML, and the doc had only been edited in WebKit (Chrome/Safari). Google
> > Docs JS code doesn't ever generate HTML like this, but I can't repro exactly
> > how the HTML got there. My best guess is copy/paste, since I've seen similar
> > things happen on copy/paste like in bug 26937.
> 
> We should fix 26937 instead then.  Because this behavior (not deleting b
> outside of ul) doesn't seem to be a problem if we didn't produce invalid HTML
> in the first place.

It also seems that if a web page contains invalid HTML (e.g. <b><ul><li>b</li></ul></b>), and a user copy/pastes from that page, the invalid HTML gets pasted as-is into the contenteditable area. Should I file that as a 3rd bug?

But even if that gets fixed as well, a user could still do the same thing in Firefox or IE, and then a WebKit user would be unable to unbold the content later.

[Ryosuke Niwa]

(In reply to comment #9)
> It also seems that if a web page contains invalid HTML (e.g.
> <b><ul><li>b</li></ul></b>), and a user copy/pastes from that page, the invalid
> HTML gets pasted as-is into the contenteditable area. Should I file that as a
> 3rd bug?

Thank you to bring that up.  Yes, that could be filed as a 3rd bug.  Since spec. says we need to be as valid as the original document was, if the document to which the content is copied into was, say HTML4.01, then we definitely want to avoid such markups.

But this brings up another, more general issue.  Say user was editing HTML4.01 then he copy & paste a part of it into XHTML1.01.  Then we need to be extra careful about what kind of markup we're inserting.  Because while <ul><li><b>hello</li></ul>world is a valid HTML4.01 that results in bolded hello followed by "normal" world, it isn't even a valid XML markup so that it completely fails to be valid in XHTML1.01.  Do we handle such cases?

What if we were editing HTML5 and then copy & paste a content that contains video tag to HTML4.01?  Should we delete video tag?  There are so many possibility here.

> But even if that gets fixed as well, a user could still do the same thing in
> Firefox or IE, and then a WebKit user would be unable to unbold the content
> later.

Users can unbold the text, can't they?  We don't delete b but we add style="font-weight: normal;"  As far as I checked, the text isn't bolded.

[Ojan Vafai]

(In reply to comment #10)
> (In reply to comment #9)
> > It also seems that if a web page contains invalid HTML (e.g.
> > <b><ul><li>b</li></ul></b>), and a user copy/pastes from that page, the invalid
> > HTML gets pasted as-is into the contenteditable area. Should I file that as a
> > 3rd bug?
> 
> Thank you to bring that up.  Yes, that could be filed as a 3rd bug.  Since
> spec. says we need to be as valid as the original document was, if the document
> to which the content is copied into was, say HTML4.01, then we definitely want
> to avoid such markups.

Does the spec say this? I'm not sure I really agree with this. That's a separate discussion from this bug though and should be discussed there.

> What if we were editing HTML5 and then copy & paste a content that contains
> video tag to HTML4.01?  Should we delete video tag?  There are so many
> possibility here.

I think it would be fine to just include the video tag. A UA that only supports HTML 4.01 will just ignore it.

> > But even if that gets fixed as well, a user could still do the same thing in
> > Firefox or IE, and then a WebKit user would be unable to unbold the content
> > later.
> 
> Users can unbold the text, can't they?  We don't delete b but we add
> style="font-weight: normal;"  As far as I checked, the text isn't bolded.

But you have to click *twice* to get it unbolded. The first click should unbold it. The second should make it bold again.

(In reply to comment #8)
> (In reply to comment #7)
> > I've seen this HTML in several Google Docs where the user had not hand-edited
> > the HTML, and the doc had only been edited in WebKit (Chrome/Safari). Google
> > Docs JS code doesn't ever generate HTML like this, but I can't repro exactly
> > how the HTML got there. My best guess is copy/paste, since I've seen similar
> > things happen on copy/paste like in bug 26937.
> 
> We should fix 26937 instead then.  Because this behavior (not deleting b
> outside of ul) doesn't seem to be a problem if we didn't produce invalid HTML
> in the first place.

While I agree that we should fix bug 26937, the way that copy-paste in WebKit wraps the copied content in a span with styling will, in some cases, result in the span being left behind. That's be design! Why else wrap it in a span? So, you could construct a case where copy-pasting from one page into another would give this sort of markup only using WebKit.

(In reply to comment #5)
> > In terms of markup, I don't see why we shouldn't remove the bold tag around the
> > UL since all it's text is contained in the selection.
> 
> But I don't see why we must insist on removing b either.  Of course, if it was
> inside LI, then we should definitely remove it.

I don't see why it makes a difference if it's inside or outside the LI.

[Ryosuke Niwa]

> But you have to click *twice* to get it unbolded. The first click should unbold
> it. The second should make it bold again.

Oh, that's a serious bug.

[Ryosuke Niwa]

(In reply to comment #12)
> > But you have to click *twice* to get it unbolded. The first click should unbold
> > it. The second should make it bold again.
> 
> Oh, that's a serious bug.

This having-to-click-twice bug should be fixed in http://trac.webkit.org/changeset/50172.

[Tony Chang]

Should this bug be closed now?  It seems like pressing bold once in the original example unbolds, although the HTML is a bit ugly:
<b><ul><li><span class="Apple-style-span" style="font-weight: normal;">a</span></li></ul></b>

The HTML can't be bolded again in Firefox 3.6 :(  It just tries to add styles to the <ul> which aren't able to override the span around the 'a'.

[Ryosuke Niwa]

(In reply to comment #14)
> Should this bug be closed now?

No, we shouldn't because

> It seems like pressing bold once in the
> original example unbolds, although the HTML is a bit ugly:
> <b><ul><li><span class="Apple-style-span" style="font-weight:
> normal;">a</span></li></ul></b>

This is the exact behavior this bug is addressing. I once closed this bug with the same reasoning but it has been argued that we should fix this bug, and produce nicer markup.

[Annie Sullivan]

I'd really like to keep this bug opened, because the type of cross-browser problem you describe below is extremely difficult for developers to track down and fix.

> The HTML can't be bolded again in Firefox 3.6 :(  It just tries to add styles
> to the <ul> which aren't able to override the span around the 'a'.

[Ryosuke Niwa]

http://trac.webkit.org/changeset/66324 solved this issue.  We just need to check in a test.

[Ryosuke Niwa]

Created attachment 66476 [details]
adds a regression test

[Kent Tamura]

Comment on attachment 66476 [details]
adds a regression test

ok.

[Kent Tamura]

Comment on attachment 66476 [details]
adds a regression test

> +
> +//document.body.removeChild(testContainer);
> +

Commented out code should be removed.  However, I like removing testContainer.

[Ryosuke Niwa]

Thanks for the review.

(In reply to comment #20)
> (From update of attachment 66476 [details])
> > +
> > +//document.body.removeChild(testContainer);
> > +
> 
> Commented out code should be removed.  However, I like removing testContainer.

Sorry about that.  I commented out for debugging and forgot to re-enable it.  I'll enable it before landing.  This will also remove "hello world webkit" after TEST COMPLETE as well.

[Ryosuke Niwa]

Committed r66743: <http://trac.webkit.org/changeset/66743>

[Ryosuke Niwa]

Created attachment 66528 [details]
test fix

Fixed the test for non-Mac platforms.  The problem was that I wasn't setting selection properly on non-Mac platforms.

[Zhenyao Mo]

The test is failing on chromium linux.  Add it to test_expectations.txt for now.  See r66754.

[WebKit Review Bot]

http://trac.webkit.org/changeset/66743 might have broken Qt Linux Release
The following changes are on the blame list:
http://trac.webkit.org/changeset/66742
http://trac.webkit.org/changeset/66743

[Ryosuke Niwa]

Test fixed in http://trac.webkit.org/changeset/66764 and http://trac.webkit.org/changeset/66764.