Fix unintended <br> insertion during bullet indentation #4684
Conversation
WalkthroughThe changes involve updates to several JavaScript files and the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Editor
participant ListHandler
participant DOM
User->>Editor: Indent list
Editor->>ListHandler: Process indentation
ListHandler->>DOM: Update DOM structure
DOM-->>ListHandler: Confirm update
ListHandler-->>Editor: Return updated state
Editor-->>User: Display updated list
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
src/js/editing/Bullet.js (1)
Line range hint
168-259: LGTM!The changes improve the readability and modularity of the code by using more concise syntax for mapping and wrapping list items and by using the
splitTreefunction from thedommodule. The updated implementation looks good.Please fix the indentation issues flagged by the static analysis tool:
- { - node: last.parentNode, - offset: dom.position(last) + 1, - }, - { - isSkipPaddingBlankHTML: true, - }, - ) + { + node: last.parentNode, + offset: dom.position(last) + 1, + }, + { + isSkipPaddingBlankHTML: true, + }, + )Tools
GitHub Check: build (20.x)
[warning] 194-194:
Expected indentation of 14 spaces but found 16
[warning] 195-195:
Expected indentation of 14 spaces but found 16
[warning] 196-196:
Expected indentation of 16 spaces but found 18
[warning] 197-197:
Expected indentation of 16 spaces but found 18
[warning] 198-198:
Expected indentation of 14 spaces but found 16
[warning] 199-199:
Expected indentation of 14 spaces but found 16
[warning] 200-200:
Expected indentation of 16 spaces but found 18
[warning] 201-201:
Expected indentation of 14 spaces but found 16
[warning] 202-202:
Expected indentation of 12 spaces but found 14
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- package.json (1 hunks)
- src/js/core/dom.js (1 hunks)
- src/js/editing/Bullet.js (9 hunks)
- test/base/module/Editor.spec.js (1 hunks)
Files skipped from review due to trivial changes (1)
- package.json
Additional context used
GitHub Check: build (20.x)
src/js/editing/Bullet.js
[warning] 194-194:
Expected indentation of 14 spaces but found 16
[warning] 195-195:
Expected indentation of 14 spaces but found 16
[warning] 196-196:
Expected indentation of 16 spaces but found 18
[warning] 197-197:
Expected indentation of 16 spaces but found 18
[warning] 198-198:
Expected indentation of 14 spaces but found 16
[warning] 199-199:
Expected indentation of 14 spaces but found 16
[warning] 200-200:
Expected indentation of 16 spaces but found 18
[warning] 201-201:
Expected indentation of 14 spaces but found 16
[warning] 202-202:
Expected indentation of 12 spaces but found 14
Additional comments not posted (8)
src/js/editing/Bullet.js (5)
Line range hint
27-53: LGTM!The changes simplify the logic for finding the previous list and appending paragraphs to it. The updated implementation looks good.
69-71: LGTM!The change ensures that the margin value is parsed as an integer with a base of 10. The updated implementation looks good.
98-106: LGTM!The changes improve the readability of the code by using a more concise syntax for filtering the list nodes. The updated implementation looks good.
129-130: LGTM!The changes simplify the logic for finding the previous and next lists and improve the readability of the code by using the
appendChildNodesfunction from thedommodule. The updated implementation looks good.Also applies to: 140-145
282-289: LGTM!The changes align the function name and implementation with the changes made in the
indentandwrapListfunctions. The use ofnextSiblingsimplifies the logic and improves consistency. The updated implementation looks good.test/base/module/Editor.spec.js (1)
140-140: Activated test case for list indentation and outdentation.The test case that was previously skipped using
it.skiphas now been activated by changing it toit. This indicates that the underlying issue related to list indentation and outdentation has likely been fixed.The activated test case thoroughly verifies the expected behavior of the editor when indenting and outdenting a list by:
- Inserting an ordered list.
- Indenting the list twice and verifying the expected HTML structure.
- Outdenting the list twice and verifying the expected HTML structure.
Activating this comprehensive test case helps ensure the correctness and reliability of the list indentation functionality.
src/js/core/dom.js (2)
423-423: Approve the addition of the new optional parameterisSkipPaddingBlankHTML.The new parameter provides flexibility to control the behavior of appending
<br>elements in specific scenarios while preserving the default behavior when the parameter is not provided.
428-428: Verify the impact of the new conditional check on the codebase.The new conditional check
!isSkipPaddingBlankHTMLis added to determine whether to append a<br>element before a list when the current node is a<li>element with no child nodes.Please ensure that the introduction of this check does not adversely affect any existing functionality or edge cases in the codebase where
appendChildNodesis used.Verification successful
The new conditional check has a limited and intended impact on list handling
The introduction of the
!isSkipPaddingBlankHTMLcheck in theappendChildNodesfunction appears to have a localized impact on the codebase. This change specifically addresses the case of appending a list to an empty<li>element, which is a unique scenario handled by this function.Based on the codebase analysis:
- The
appendChildNodesfunction is used in various parts of the code for DOM manipulation and list operations.- The new conditional check is only present in one implementation of
appendChildNodes.- No other occurrences of the specific pattern
isLi($_) && $_.firstChild === null && isList($_)were found, suggesting this is a targeted fix.The change doesn't seem to adversely affect existing functionality. However, to ensure the desired behavior:
- Manually test list-related operations, particularly scenarios involving nested lists and empty list items.
- Verify that the
<br>element is correctly added when appending a list to an empty<li>element, and that this doesn't cause any unintended side effects in the rendered output.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of the new conditional check in `appendChildNodes`. # Test 1: Search for occurrences of `appendChildNodes` function calls. # Expect: Identify places where the function is called and manually review if the new behavior is desirable. rg --type javascript -A 5 $'appendChildNodes\(' # Test 2: Search for `<li>` elements followed by `<ul>` or `<ol>` elements. # Expect: Identify scenarios where the new conditional check might affect the rendering. rg --type html $'<li>[^<]*</li>\s*<(ul|ol)>'Length of output: 129
Script:
#!/bin/bash # Find JavaScript files and search for appendChildNodes function calls echo "Searching for appendChildNodes function calls:" fd -e js -x rg -A 5 'appendChildNodes\(' # Use ast-grep to find appendChildNodes function calls echo "Using ast-grep to find appendChildNodes function calls:" ast-grep --lang javascript --pattern 'appendChildNodes($_)' # Use ast-grep to find list-related patterns echo "Searching for list-related patterns:" ast-grep --lang javascript --pattern 'isLi($_) && $_.firstChild === null && isList($_)'Length of output: 1883
|
Hello @hackerwins |
|
Hello, thank you for your feedback. After considering your comments, I'd like to summarize the current situation and propose a plan moving forward. I recognize that bullet.js has undergone several patches, with the main issues being unwanted The primary reason for rolling back the code, including previousElementSibling, was the lack of test code with skipping in the previous patch, making it difficult to understand the rationale behind the changes. This was a precautionary decision. https://github.com/summernote/summernote/pull/4679/files#r1756468136 I take your points about the previous version's issues with whitespaces between HTML elements and formatted HTML code seriously. I acknowledge that simply reverting to an earlier version may not be the best solution. I kindly request that you submit a new PR specifically for the changes related to previousElementSibling. Please include comprehensive test code with this PR to demonstrate the functionality and purpose of these changes. This will help us understand the rationale behind the switch from previousSibling to previousElementSibling and its impact on whitespace and HTML formatting issues. Once I receive your PR with the test code, I'll carefully review it and test it against the issues you've mentioned, particularly the problems with whitespaces and formatted HTML. Your expertise is invaluable in resolving these issues. We appreciate your insights, especially regarding the stability of version #4679. I look forward to your continued involvement and feedback throughout this process. |
|
I would like to add stabilization work related to bullet in v0.9.0. Is it possible? 
|
|
Hello @hackerwins
The fix for issue no 1 was: The causes of issue no 2 were more hidden or complicated. Now to the possible proceedings: Another possibility would be that I revert to the current head of the main branch and report HTML snippets that fail when using certain functions like indent/outdent/creating a line break etc. You would have control and an eye over the global architecture. Any thoughts about this? |
What does this PR do?
This PR fixes the issue of unintended
<br>insertion while indenting bullets.The problem has been noted in the test code where a
<br>is inserted within the indentation of bullets, disrupting the expected behavior. Although a PR by HoffmannTom previously attempted to address this issue, it seems to have not resolved the root cause.In this PR, I have reverted bullet.js to a version prior to HoffmannTom's fix and made direct amendments to the code in dom.js that triggers this problem.
@HoffmannTom
Can you please confirm whether the problems that previously occurs in this version?
Where should the reviewer start?
How should this be manually tested?
Any background context you want to provide?
What are the relevant tickets?
Screenshot (if for frontend)
Checklist