Bug 26871 – Can't unbold text in div in font-weight span

• Component: HTML Editing
• Status: RESOLVED FIXED

[Annie Sullivan]

STEPS TO REPRODUCE:
1. Go to midas demo and enter the following HTML:
<span style="font-weight: 900;"> <div>text</div> </span>
2. Select "text" and hit bold button

ACTUAL RESULT:
Text is not unbolded, no matter how many times bold button is hit

EXPECTED RESULT:
Text unbolded

I know this is a pretty low priority bug, but we had a user that managed to paste this into their document, so I figured I'd file it.

[Ryosuke Niwa]

It seems like this problem is caused by http://trac.webkit.org/browser/trunk/WebCore/editing/ApplyStyleCommand.cpp#L949 where it obtains removeStart = start.upstream().  Because upstream will respect block elements, we cannot delete span whose end is visually different from the different of div inside of it.

By the way, in investing this problem, I found a weird behavior in applyInlineStyle.

    bool splitEnd = splitTextElementAtEndIfNeeded(start, end);
    if (splitEnd) {
        start = startPosition();
        end = endPosition();
        endDummySpanAncestor = dummySpanAncestorForNode(end.node());
    }

splits an element even when it's not needed.  e.g. <div id="test" style="font-weight: 900;">world</div> was wrapped by style spans and splited to <div id="test" style="font-weight: 900;"><span class="Apple-style-span">w</span><span class="Apple-style-span">orld</span></div>.  I don't think this behavior is intended and may be filed as a bug.

[Ryosuke Niwa]

Typos:
visually different from the end* of div inside.
while* investigating* this problem

Maybe what we need is to push down all styles regardless of whether it's text decoration or not.  Consider the following HTML:
<div style="font-weight: bold;">hello<div id="test">world</div></div>

By toggling bold on "world", we get:
<div style="font-weight: bold;">hello<div id="test"><span class="Apple-style-span" style="font-weight: normal;">world</span></div></div>


On the other hand, if we had:
<div style="text-decoration: underline;">hello<div id="test">world</div></div>

and toggled underline on "world", we get:
<div style=""><u>hello</u><div id="test">world</div></div>

because we push-down any text decorations and re-apply styles.  Except the style attribute, which should go away entirely, this markup looks much cleaner.  There might be some conflict of interets in terms of preserving original elements and keeping the markup simple & clean.

[Ryosuke Niwa]

Created attachment 63621 [details]
work in progress

[Ryosuke Niwa]

This patch still requires cleanup and most of it should be merged with pushDownTextDecorationStyleAroundNode.  It also requires rebaselining editing/deleting/delete-4038408-fix.html, editing/deleting/delete-br-011.html, editing/execCommand/5770834-1.html, and editing/execCommand/format-block-from-range-selection.html.

[Ryosuke Niwa]

Created attachment 63657 [details]
work in progress 2

It turned out that we don't even need to special case the text-decoration.  This patch will use applyInlineStyleIfNeed to push down all the inline styles regardless of whether it's text-decoration or not.  This patch still adds a superfluous font tag at the beginning so I'll need to work on it a little more.

[Ryosuke Niwa]

I'm stuck with mail blockquote here.

In editing/deleting/delete-4038408-fix.html, we start deleting paragraphs from the very end:
	DIV	0x1129c0f30 CLASS=editing
		#text	0x1129c1280 "\n"
		DIV	0x1129c12e0
			BR	0x1129c15e0 CLASS=khtml-block-placeholder
		#text	0x1129c17d0 "\n"
		DIV	0x1129c1830
			#text	0x1129c19c0 "\n"
			BLOCKQUOTE	0x1129c1b40 STYLE=color:blue;
				#text	0x1129c1f50 "\n"
				DIV	0x1129c1fb0
					#text	0x1129c2200 "Here is some reply text"
				#text	0x1129c22e0 "\n"
				DIV	0x1129c2340
					#text	0x1129c2510 "It should have the reply text style"
				#text	0x1129c2620 "\n"
				DIV	0x1129c2680
					BR	0x1129c2910 CLASS=khtml-block-placeholder
				#text	0x1129c2b00 "\n"
				DIV	0x1129c2b60
					BR	0x1129c2dd0 CLASS=khtml-block-placeholder
				#text	0x1129c2f50 "\n"
				DIV	0x1129c2fb0
*					BR	0x1129cac20
			#text	0x1129c3960 "\n"
		#text	0x1129c39f0 "\n\n"

This is DOM when deleting the empty paragraph right after blockquote.  Here, ApplyStyleCommand is called in the moveParagraphs because it's an empty paragraph, and it's trying to preserve the style (moveParagraphs is called by moveParagraph, which is called by mergeParagraphs, which in turn is called by DeleteSelectionCommand).

Here, the problem is that if we push inline style that's conflicting with our current style, we end up losing color property from blockquote and adding a whole bunch of font tags to each of the paragraphs we have.  Is this behavior what we want?  If not, then the only fix I can give to this bug is to produce: <span style="font-weight: 900;"> <div><span style="font-weight: normal;">text</span></div> </span>.  It's basically a trade off.  We either allow push-down of styles everywhere or we live up with ugly markup whenever visual presentation is right.

[Ojan Vafai]

> Here, the problem is that if we push inline style that's conflicting with our current style, we end up losing color property from blockquote and adding a whole bunch of font tags to each of the paragraphs we have.  Is this behavior what we want?

I don't think so.

>  If not, then the only fix I can give to this bug is to produce: <span style="font-weight: 900;"> <div><span style="font-weight: normal;">text</span></div> </span>.  It's basically a trade off.  We either allow push-down of styles everywhere or we live up with ugly markup whenever visual presentation is right.

Is it not possible to just push-down the specific styles that we're modifying?

[Ryosuke Niwa]

(In reply to comment #7)
> > Here, the problem is that if we push inline style that's conflicting with our current style, we end up losing color property from blockquote and adding a whole bunch of font tags to each of the paragraphs we have.  Is this behavior what we want?
> 
> I don't think so.

I thought so.

> >  If not, then the only fix I can give to this bug is to produce: <span style="font-weight: 900;"> <div><span style="font-weight: normal;">text</span></div> </span>.  It's basically a trade off.  We either allow push-down of styles everywhere or we live up with ugly markup whenever visual presentation is right.
> 
> Is it not possible to just push-down the specific styles that we're modifying?

No, the problem is that color is the specific style we're modifying in DeleteSelectionCommand::mergeParagraphs.  What's happening is that we're merging an empty paragraph to the previous paragraph and moveParagraphs special cases empty paragraphs to copy the style (ReplaceSelectionCommand doesn't preserve the style in this case).

But I have a fix for this in mergeParagraphs, which is to special case the empty paragraph in mergeParagraphs and avoid preserving style in moveParagraphs. Because DeleteSelectionCommand sets the typing style to that of the previous paragraph after merging the empty paragraph, we didn't need to preserve the style of the empty paragraph in the first place.  I'll upload the patch with this fix as soon as https://bugs.webkit.org/show_bug.cgi?id=43699 is fixed.

[Ryosuke Niwa]

Created attachment 63971 [details]
work in progress 3

[Ryosuke Niwa]

Comment on attachment 63971 [details]
work in progress 3

It seems like I can fix the bug soon.

> +//    if (isSpanWithoutAttributesOrUnstyleStyleSpan(element))
> +//        removeNodePreservingChildren(element);

I would really like to enable this but there's a failing test right now.
Will investigate more.

[Ryosuke Niwa]

Created attachment 64058 [details]
fixes the bug

[Ryosuke Niwa]

(In reply to comment #11)
> Created an attachment (id=64058) [details]
> fixes the bug

My current patch has an issue of generating style attributes for sibling block nodes even when styleWithCSS=false but this should probably be addressed in another bug.  To fix that, we need to clean up applyInlineStyleToRange or its substitute and use it in applyInlineStyleToPushDown.

I wish I could split the patch into smaller pieces but it does not seem possible at the moment.

[Tony Chang]

Comment on attachment 64058 [details]
fixes the bug

> Index: WebCore/editing/ApplyStyleCommand.cpp
> +    for (size_t i = 0; i < properties.size(); i++) {
> +        RefPtr<CSSValue> property = style->getPropertyCSSValue(properties[i]);
> +        if (property && !equalIgnoringCase(property->cssText(), "none"))

As discussed in person, I would remove the check for "none" just in case some style uses "none".

> Index: LayoutTests/editing/style/script-tests/push-down-inline-styles.js
> +testSingleToggle("bold", '<span style="font-weight: 900;"> <div>text</div> </span>', ' <div>text</div> ');
> +testSingleToggle("bold", '<span style="font-weight: 900;"><div>text</div></span>', '<div>text</div>');
> +testSingleToggle("bold", '<span style="font-weight: 900;"><div id="test">hello</div><div>world</div></span>', '<div id="test">hello</div><div style="font-weight: 900; ">world</div>');

These tests are great.  Can you add some tests that have extra css properties to show that they're not accidentally removed.  For example, apply bold to <span style="font-style: italic">hello</span> or apply bold to <span style="font-style: italic; font-weight:900">hello</span>.  Also, what happens with different font weights (600, 300)?  Can you also add some tests for line through (strike)?

The rest looks fine to me, but perhaps Ojan should also take a look.

[Ryosuke Niwa]

Created attachment 64166 [details]
fixed per tony's comment and also supported text-decoration propagation

[Ryosuke Niwa]

Thanks for the feedback.

(In reply to comment #13)
> (From update of attachment 64058 [details])
> > Index: WebCore/editing/ApplyStyleCommand.cpp
> > +    for (size_t i = 0; i < properties.size(); i++) {
> > +        RefPtr<CSSValue> property = style->getPropertyCSSValue(properties[i]);
> > +        if (property && !equalIgnoringCase(property->cssText(), "none"))
> 
> As discussed in person, I would remove the check for "none" just in case some style uses "none".

Done.

> > Index: LayoutTests/editing/style/script-tests/push-down-inline-styles.js
> > +testSingleToggle("bold", '<span style="font-weight: 900;"> <div>text</div> </span>', ' <div>text</div> ');
> > +testSingleToggle("bold", '<span style="font-weight: 900;"><div>text</div></span>', '<div>text</div>');
> > +testSingleToggle("bold", '<span style="font-weight: 900;"><div id="test">hello</div><div>world</div></span>', '<div id="test">hello</div><div style="font-weight: 900; ">world</div>');
> 
> These tests are great.  Can you add some tests that have extra css properties to show that they're not accidentally removed.  For example, apply bold to <span style="font-style: italic">hello</span> or apply bold to <span style="font-style: italic; font-weight:900">hello</span>.  Also, what happens with different font weights (600, 300)?  Can you also add some tests for line through (strike)?

The logic for line-through is identical to that of underline since both uses text-decoration but I added one test.  I also supported the propagation of text-decoration.

[Ryosuke Niwa]

Thanks for the feedback.

(In reply to comment #13)
> (From update of attachment 64058 [details])
> > Index: WebCore/editing/ApplyStyleCommand.cpp
> > +    for (size_t i = 0; i < properties.size(); i++) {
> > +        RefPtr<CSSValue> property = style->getPropertyCSSValue(properties[i]);
> > +        if (property && !equalIgnoringCase(property->cssText(), "none"))
> 
> As discussed in person, I would remove the check for "none" just in case some style uses "none".

Done.

> > Index: LayoutTests/editing/style/script-tests/push-down-inline-styles.js
> > +testSingleToggle("bold", '<span style="font-weight: 900;"> <div>text</div> </span>', ' <div>text</div> ');
> > +testSingleToggle("bold", '<span style="font-weight: 900;"><div>text</div></span>', '<div>text</div>');
> > +testSingleToggle("bold", '<span style="font-weight: 900;"><div id="test">hello</div><div>world</div></span>', '<div id="test">hello</div><div style="font-weight: 900; ">world</div>');
> 
> These tests are great.  Can you add some tests that have extra css properties to show that they're not accidentally removed.  For example, apply bold to <span style="font-style: italic">hello</span> or apply bold to <span style="font-style: italic; font-weight:900">hello</span>.  Also, what happens with different font weights (600, 300)?  Can you also add some tests for line through (strike)?

The logic for line-through is identical to that of underline since both uses text-decoration but I added one test.  I also supported the propagation of text-decoration.

[Tony Chang]

Comment on attachment 64166 [details]
fixed per tony's comment and also supported text-decoration propagation

Ojan, do you also want to review?

[Ryosuke Niwa]

Committed r65208: <http://trac.webkit.org/changeset/65208>

[Ryosuke Niwa]

*** Bug 31691 has been marked as a duplicate of this bug. ***