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>
Created attachment 208022 [details] Patch
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.
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
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
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?
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
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
(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.
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?
(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?
Could you please clarify, whether we want to align our behavior with those of firefox & IE or is the existing behavior just fine? Thanks.
+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.
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.
(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?
(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.
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.
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.
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!
So this sounds like a regression, not a progression. We did not want to change this.