Bug 52846

Created attachment 80557 [details] Proposed patch

I will be happy to follow your advices to make the patch accepted. Thank you... Raffaele

The old behavior is clearly wrong and the new behavior is clearly correct. rniwa is probably best to confirm whether the code change is the best one.

View in context: https://bugs.webkit.org/attachment.cgi?id=80557&action=review > LayoutTests/editing/execCommand/insert-list-inside-editable-paragraph-containing-noneditable-elements.html:3 > +<div id="test2" contenteditable="true"><p id="test2-p">This is another editable paragraph that contains a <span contenteditable="false">non-editable</span> span.</p></div> Why the extra ps, rather than just using the containing div? Anyway, rather than repeating the same test case, it would be nice to test some different cases, such as - non-editable span at the start of the paragraph - non-editable span at the end of the paragraph - non-editable span in the middle somewhere (as you have) - multiple lines (with non-editable spans), no selection - multiple lines (with non-editable spans), with the selection across multiple lines (i.e. use getSelection().modify("extend", "forward", "line") or something) > LayoutTests/editing/execCommand/insert-list-inside-editable-paragraph-containing-noneditable-elements.html:15 > +Markup.dump("test1", "The whole paragraph - non-editable part included - must become a list item"); I'd pull this common logic into a function. Also, if there is a way to just print "PASS ..." or "FAIL ..." then that would be more concise than using Markup and easier to understand regressions.

Hmm, I don't actually have permission to comment on the patch so I did it in a roundabout way, and it didn't seem to work in context.

Here is a similar bug: https://bugs.webkit.org/show_bug.cgi?id=53409

Comment on attachment 80557 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=80557&action=review > Source/WebCore/editing/InsertListCommand.cpp:331 > - VisiblePosition start = startOfParagraph(originalStart); > - VisiblePosition end = endOfParagraph(start); > + VisiblePosition start = startOfParagraph(originalStart, CanCrossEditingBoundary); > + VisiblePosition end = endOfParagraph(start, CanCrossEditingBoundary); How do we ensure that the entire paragraph is editable if we allow start/end to cross editing boundary? For example, if we had <div contenteditable="false">hello <span contenteditable="true">world</span></div>, and the caret is inside the span, we shouldn't be listifying "hello world". Because of deep equivalent positions, I don't think we're ready to fix this kind of bugs properly yet. Also, please don't just fix InsertUnorderedList. At least InsertOrderedList must also be fixed. And I'm sure the same bug exists for IndentOutdentCommand and FormatBlockCommand. If you're interested in working on InsertListCommand, please fix crash/hang bugs we have as they have a higher priority.

> How do we ensure that the entire paragraph is editable if we allow start/end to cross editing boundary? > For example, if we had <div contenteditable="false">hello <span contenteditable="true">world</span></div>, > and the caret is inside the span, we shouldn't be listifying "hello world". I'm aware of this kind of problem but I consider it largely independent of this bug. Even now (without my patch), in a situation like the one you described, the behavior of 'insertList' is wrong. If you put an editable span inside a non-editable paragraph, you clearly want the user allowed to modify the content of the span, not the nature of the paragraph (from plain paragraph to list item) nor the nature of the span itself (from span to list item or worst inserting a list inside it where only phrasing content is allowed). In such a situation "insertList" command should not be available at all (queryCommandEnabled should return false). I think when 'insertList' is invoked we must look for the nearest non-phrasing content ancestor of the element containing the caret. That is the element 'insertList' will be acting on. Now, if its parent element is editable the execution can go on, otherwise it stops without making changes to the document. To realize the above project we must have a way to determine if an element is of phrasing content category*. For that reason, within the discussion relative to another WebKit bug (https://bugs.webkit.org/show_bug.cgi?id=45866), I proposed to add a new method to the HTMLElemet class: isPhrasingContent(). Unfortunately I have got virtually no feedback on my idea. > Also, please don't just fix InsertUnorderedList. At least InsertOrderedList must also be fixed. My patch already fixes either 'InsertUnorderedList' or 'InsertOrderedList'. * At the moment the paragraph element which 'insertList' will be acting on is determined by looking for the nearest ancestor with css style property 'display' set to 'block'. That is not correct because there can be elements with 'display' property set to 'block' that are in fact children of a paragraph. For example display math equations are displayed as block but they don't break the paragraph where they lie in. Raffaele

(In reply to comment #8) > I'm aware of this kind of problem but I consider it largely independent of this bug. What I'm saying that your patch would have reproduced a regression or that it would have made the situation worse. > Even now (without my patch), in a situation like the one you described, the behavior of 'insertList' is wrong. > If you put an editable span inside a non-editable paragraph, you clearly want the user allowed to modify the content of the span, not the nature of the paragraph (from plain paragraph to list item) nor the nature of the span itself (from span to list item or worst inserting a list inside it where only phrasing content is allowed). > In such a situation "insertList" command should not be available at all (queryCommandEnabled should return false). I agree and we should take care of that instead of ignoring it. In fact, there seems to be a dependency that we need to fix the said bug before being able to fix this one. > I think when 'insertList' is invoked we must look for the nearest non-phrasing content ancestor of the element containing the caret. That is the element 'insertList' will be acting on. This isn't sufficient nor necessary condition. For example, non-phrasing element may be inline (display: inline;) and it may not make sense for us to indent against such an element from user's perspective. > To realize the above project we must have a way to determine if an element is of phrasing content category*. > For that reason, within the discussion relative to another WebKit bug (https://bugs.webkit.org/show_bug.cgi?id=45866), I proposed to add a new method to the HTMLElemet class: isPhrasingContent(). > Unfortunately I have got virtually no feedback on my idea. Yes, I'm aware of that bug and I gave you the feedback that we must be careful when we're adding a function to Node. And Adam pointed out that the parser may know which element is a phrasing element. I'm more than happy to give you more feedback if you can tell us what kind of feedback you'd like to receive. > > Also, please don't just fix InsertUnorderedList. At least InsertOrderedList must also be fixed. > > My patch already fixes either 'InsertUnorderedList' or 'InsertOrderedList'. But your patch didn't have any tests. How can we know that the bug is fixed?

I can't reproduce in Chrome 15.0.849.0 dev. The testcase from comment #0 works as expected: everything gets made into a list.

I am able to observe following behavior: *** Safari 15.5 on macOS 12.5 *** Placing caret in any place except non-editable make it "list" upon clicking button, non-editable span does not do anything. Upon listified, the caret remain at the same position and <div contenteditable> remain focused (like you can continue typing etc. and how rich text formatting behave). *** Firefox Nightly 104 *** Placing caret in any place except non-editable make it "list" upon clicking button, non-editable span does not do anything. Upon listified, the focus is lost from <div contenteditable> and it is more like one click magic and now poof. (NOT Ideal for Rich text formatting) *** Chrome Canary 106 *** Placing caret in any place except non-editable make it "list" upon clicking button, non-editable span does not do anything. Upon listified, the caret jumps to start of text line or paragraph and <div contenteditable> remain focused (like you can continue typing etc. and how rich text formatting behave). Although jumping of caret is not ideal for editing document. __________ I think Safari behavior is the most ideal one here and there is no issue of content being moved as mentioned in Comment 0. I think this can be marked as "RESOLVED WONTFIX" or "RESOLVED CONFIGURATION CHANGED". Thanks!