Bug 100453 – contentEditable: Style of List Item Leaks Through After List End

• Component: HTML Editing
• Status: NEW

[Haw-Bin Chai]

Created attachment 170804 [details]
Test page to repro issue.

Steps to repro:
1) Open the attached page contentEditable.html
2) Position the cursor after "Foo" in the editable region.
3) Press [Enter] twice to cause the list to be closed. Type "Bar".
4) Click "Update Markup" to display the HTML structure of the editable field.

Expected:
The CSS style of the list items should be not extend beyond the list.

Actual:
The text "Bar" is surrounded by <span>'s which explicitly set the line-height as for the list item style. The new style is sticky and can't be gotten rid of easily. The output markup in Chrome 22 (WebKit 537.4):

<ul><li>Foo</li></ul><div><span style="line-height: 25.600000381469727px;">Bar</span></div>

Output markup in FF 16:

<ul><li>Foo</li></ul><p>Bar<br></p> 

Output markup in IE 9:

<UL>
<LI>Foo</LI></UL>
<P>Bar</P>

[Vani Hegde]

Created attachment 208022 [details]
Patch

[Vani Hegde]

The issue here is while breaking out of empty <li>, the style applied to <li> bleeds out of it.

In CompositeEditCommand::breakOutOfEmptyListItem(), while breaking out of an empty list element, we save the style present in it and apply it to the newBlock irrespective of whether it is <li> or <div> or <p>.
IMO, there is no need of saving the style of empty <li> and applying it to newBlock. 

I may be wrong in thinking that there is no need to do this at all, nevertheless below is the explanation for why I think so.

The empty <li> might have any of the below styles associated with it
	1. Style particularly applied to it (Ex: <li style=color:red>br</li>)
	2. Style applied to all <li>s in the document (Ex: li{color:red;})
	3. Style applied to document as a whole (Ex: body{color:red;})

New block created can be either <li> or <div>/<p>.

Let's consider the case where newBlock created while breaking out is either <div> or <p>
    If <li> has style as per 1 or 2, it would be wrong to apply this style to newBlock (And we are doing it currently)
    If <li> has style as per 3, it would have been already handled while attaching style to element's renderer and we need not do anything.

Now, let's consider the case where newBlock is <li>
    If it's style due to case 1, I think it would be wrong to apply it to new <li>. In case we want to apply the style for this particular case we can do something like below
	if (!style->isEmpty() && newBlock->hasTagName(olTag)) //Applying style of empty <li> to newBlock only if it is an <li> again
	
	If <li> has style as per 2 or 3, it would have been already handled while attaching style to element's renderer and we need not do anything
	

With this change I have executed all layout tests and the only regression I found was editing/deleting/delete-first-list-item.html
This was added as part of changelist http://trac.webkit.org/changeset/15563 and I feel this test case is wrong in itself as it is expecting the style specifically applied to empty <li> to bleed out while breaking out.

We will have to rebaseline this test.

Eager to know your thoughts on this.

[Build Bot]

Comment on attachment 208022 [details]
Patch

Attachment 208022 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1332171

New failing tests:
editing/deleting/delete-first-list-item.html

[Build Bot]

Created attachment 208023 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.4

[Ryosuke Niwa]

Comment on attachment 208022 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=208022&action=review

> Source/WebCore/ChangeLog:25
> +        it would be wrong to apply this to new block which is not <li>.

Why?

> Source/WebCore/ChangeLog:31
> +        If style present in <li> is due to 1, it would be wrong to apply
> +        this to new <li>.

Why?

[Build Bot]

Comment on attachment 208022 [details]
Patch

Attachment 208022 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1336300

New failing tests:
editing/deleting/delete-first-list-item.html

[Build Bot]

Created attachment 208050 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.4

[Vani Hegde]

(In reply to comment #5)
> (From update of attachment 208022 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=208022&action=review
> 
> > Source/WebCore/ChangeLog:25
> > +        it would be wrong to apply this to new block which is not <li>.
> 
> Why?

Since the style has been specified particularly for <li>, wouldn't it be wrong to apply it to non <li> elements?

This example will describe it better
http://jsfiddle.net/rpkMM/
Please observe the behavior in firefox and IE.

> 
> > Source/WebCore/ChangeLog:31
> > +        If style present in <li> is due to 1, it would be wrong to apply
> > +        this to new <li>.
> 
> Why?
Example 1:
http://jsfiddle.net/JWdxC/

When we consider this example
<ul style=color:blue>
	<li>1</li> <ul style=color:red><li>a</li><li><br></li></ul>
	<li>2</li>
</ul>

Outer <ul> has a different style than that of inner <ul>. When we break out of an empty <li> present in inner <ul>, we will be creating new <li> which would be now part of outer <ul>. Shouldn't we apply the style of outer <ul> to this newly created <li>?
This is the behavior we see in firefox and this seems right.
On the other hand, we still retain the style of inner <ul> for new <li>, which apparently is part of outer <ul>

In this scenario, not passing on the style seems more appropriate.

Example 2:
But if we consider below example,
<ul>
	<li>1</li> <ul><li>a</li><li style=color:red><br></li></ul>
	<li>2</li>
	
</ul>

it becomes little tricky.  Here the behavior is not quite obvious and is debatable.

[Ryosuke Niwa]

I disagree. As far as user is concerned, it shouldn't matter whether the style is specified on li or on an inline element.

Specifically,
<li style="color:red">hello</li>
and
<li><span style="color:red">hello</span></li>
look identical for users.

Why should the fact the style is specified on li matter at all?

[Vani Hegde]

(In reply to comment #9)
> I disagree. As far as user is concerned, it shouldn't matter whether the style is specified on li or on an inline element.
> 
> Specifically,
> <li style="color:red">hello</li>
> and
> <li><span style="color:red">hello</span></li>
> look identical for users.
> 
I agree that the above two cases do not make difference for the user.
But I believe here we are speaking about specific case of breaking out of an empty <li>.
So here we are either creating
1) <div>/<p>
      Should we apply the style of <li> to <div>/<p>?
2)or a new <li>
     Which will be part of different <ul>. If this <ul> has a different style property, shouldn't we apply that?
If the <ul> to which we are adding does not have any style, then we can consider applying the style present in empty <li>.


> Why should the fact the style is specified on li matter at all?
It definitely should matter, how can we apply the style specified for <li>, to
a normal text node?

Again, both http://jsfiddle.net/rpkMM/ and http://jsfiddle.net/JWdxC/ are examples. What do you think is the right behavior in the above examples. The one we currently have or firefox & IE behavior?

Do you mean we should stick on to the current behavior we have?

[Vani Hegde]

Could you please clarify, whether we want to align our behavior with those of firefox & IE or is the existing behavior just fine? Thanks.

[Haw-Bin Chai]

+1 to fixing this. Thanks for offering to do this Vani.

There's no question in my mind that the current behavior is buggy. It results in list editing that becomes literally unusable; unexpected styles are introduced with no way to clear them. I've seen this in Google Sites and Wordpress editor. I've never had a problem when editing with a non webkit-based browser.

[Ryosuke Niwa]

In the first example (http://jsfiddle.net/rpkMM/), we should be removing the color if and only if the list item is empty.

In the second example (http://jsfiddle.net/JWdxC/), we should be preserving the color.

[Vani Hegde]

(In reply to comment #13)
> In the first example (http://jsfiddle.net/rpkMM/), we should be removing the color if and only if the list item is empty.
> 
> In the second example (http://jsfiddle.net/JWdxC/), we should be preserving the color.

Thanks rniwa.
I am not quite getting why we have to preserve the style in second example(http://jsfiddle.net/JWdxC/), as the style is applied on <ul> from which we are breaking out and the new <ul> into which we are inserting the <li> has a conflicting style.

Are there some general set of rules which we can use to arrive at whether or not to preserve style(specifically while breaking out of empty list item)?

Is it something like if we are breaking out of a nested list (in which case we would create new <li>), we need to retain style?

[Haw-Bin Chai]

(In reply to comment #13)
> In the first example (http://jsfiddle.net/rpkMM/), we should be removing the color if and only if the list item is empty.

Why only if the list item is empty? It seems obvious to me that once you've broken out of the list, the list style should no longer apply.
 
> In the second example (http://jsfiddle.net/JWdxC/), we should be preserving the color.

Again, I can't understand why you would want to preserve the color from the inner list when you've broken out of it. Please explain your position.

(In reply to comment #13)
> In the first example (http://jsfiddle.net/rpkMM/), we should be removing the color if and only if the list item is empty.
> 
> In the second example (http://jsfiddle.net/JWdxC/), we should be preserving the color.

[Ryosuke Niwa]

The rule of thumb in WebKit is to follow whatever TextEdit on Mac does.

In general, differentiating styles of block elements and inline elements is a bad idea.

[Darin Adler]

Comment on attachment 208022 [details]
Patch

We don't agree that we want this change to editing behavior. Editing tries to match user expectations rather than HTML expert expectations.

[Ahmad Saleem]

Created attachment 459912 [details]
Safari 15.5 matches other browsers

I was not able to reproduce this bug in Safari 15.5 on macOS 12.4 and now behaviour is consistent across all browsers as shown in the picture. I have tested all other JSFiddle in this bug and for all, the behaviour is consistent.

Appreciate if this can be marked as "RESOLVED CONFIGURATION CHANGED". Thanks!

[Alexey Proskuryakov]

So this sounds like a regression, not a progression. We did not want to change this.