Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix list IME support in Safari #3875

Merged
merged 3 commits into from
Sep 30, 2023
Merged

Fix list IME support in Safari #3875

merged 3 commits into from
Sep 30, 2023

Conversation

luin
Copy link
Member

@luin luin commented Sep 22, 2023

Closes #3837

It's a special behavior in Safari that when list item has position: relative, typing with an IME will insert the text directly in <ol> instead of in <li>:

<ol>
  <li>a</li></ol>

"我" will be removed by Quill because allowedChildren defined in list format.

Remove position: relative fixes the issue.

@luin luin marked this pull request as ready for review September 22, 2023 04:48
@luin luin force-pushed the zh-fix-list-ime branch 3 times, most recently from e327b88 to ae629f9 Compare September 22, 2023 08:16
assets/core.styl Outdated
Comment on lines 75 to 76
> .ql-ui
position: static;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's already a similar rule for .ql-ui https://github.com/quilljs/quill/blob/e33f352b9ede51759cb3d616835d1e1e80694251/assets/core.styl#L211-L212

Do we need to add this or just remove the position: relative?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The position: static is added here to override the position: absolute that you linked since we are removing position: relative from li. Otherwise the checklist icon will be positioned incorrectly:

CleanShot.2023-09-26.at.10.32.03.mp4

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay so we need it for other ql-ui's (ex code block's language selector) but not list items' - I would move the two rules closer to each other then so it's clear this is intentional:

diff --git a/assets/core.styl b/assets/core.styl
index 1182f18c..b16de126 100644
--- a/assets/core.styl
+++ b/assets/core.styl
@@ -72,9 +72,6 @@ resets(arr)
     list-style-type: none
     padding-left: LIST_STYLE_OUTER_WIDTH
 
-    > .ql-ui
-      position: static;
-
     > .ql-ui:before
       display: inline-block
       margin-left: -1*LIST_STYLE_OUTER_WIDTH
@@ -213,6 +210,10 @@ resets(arr)
   .ql-ui
     position: absolute
 
+  li
+    > .ql-ui
+      position: static;
+
 .ql-editor.ql-blank::before
   color: rgba(0,0,0,0.6)
   content: attr(data-placeholder)

@luin luin requested a review from jhchen September 26, 2023 02:45
assets/core.styl Outdated
Comment on lines 75 to 76
> .ql-ui
position: static;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay so we need it for other ql-ui's (ex code block's language selector) but not list items' - I would move the two rules closer to each other then so it's clear this is intentional:

diff --git a/assets/core.styl b/assets/core.styl
index 1182f18c..b16de126 100644
--- a/assets/core.styl
+++ b/assets/core.styl
@@ -72,9 +72,6 @@ resets(arr)
     list-style-type: none
     padding-left: LIST_STYLE_OUTER_WIDTH
 
-    > .ql-ui
-      position: static;
-
     > .ql-ui:before
       display: inline-block
       margin-left: -1*LIST_STYLE_OUTER_WIDTH
@@ -213,6 +210,10 @@ resets(arr)
   .ql-ui
     position: absolute
 
+  li
+    > .ql-ui
+      position: static;
+
 .ql-editor.ql-blank::before
   color: rgba(0,0,0,0.6)
   content: attr(data-placeholder)

@luin luin merged commit 50628a7 into develop Sep 30, 2023
5 checks passed
@luin luin deleted the zh-fix-list-ime branch September 30, 2023 03:50
@lefex
Copy link

lefex commented Oct 18, 2023

The situation has improved, but I have conducted some tests and found the following issues when the project is not using React: @luin @jhchen

Issue 1: When deleting, the cursor position may behave abnormally. You can see this in the video.

Screen.Recording.2023-10-18.at.2.54.42.PM.mov

Issue 2: After typing in Chinese characters, deletion is not possible.

Screen.Recording.2023-10-18.at.2.56.46.PM.mov

The DOM:
image

Note: These issues may not occur if the developer debugging tool is open. Please make sure it is closed to replicate the problem.

My test code use vite:

import Delta from 'quill-delta';
import Quill from '../src/quill/quill';

const rootId = 'editor-view';

let quill = new Quill(`#${rootId}`, {
    // theme: 'snow'
});

window.quill = quill;

const addBulletList = (delta: Delta) => {
    const items = [
        '一种平行于DOM的结构',
        '实现了OT算法'
    ];
    items.forEach(item => {
        delta.insert(item);
        delta.insert('\n', {
            list: 'bullet'
        });
    });
};

const addOrderList = (delta: Delta) => {
    const items = [
        '有序列表1',
        '有序列表2'
    ];
    items.forEach(item => {
        delta.insert(item);
        delta.insert('\n', {
            list: 'ordered'
        });
    });
};

const delta = new Delta();
addBulletList(delta);
addOrderList(delta);

quill.setContents(delta);
<!DOCTYPE html>
<html lang="en">
<head>
    <meta charset="UTF-8">
    <meta name="viewport" content="width=device-width, initial-scale=1.0">
    <link rel="stylesheet" href="../src/quill/assets/core.styl" />
    <script src="https://unpkg.com/vconsole@latest/dist/vconsole.min.js"></script>
    <script>
    var vConsole = new window.VConsole();
    </script>
    <title>PureQuill</title>
    <style>
        * {
            padding: 0;
            margin: 0;
        }
        #editor-view {
            width: 100vw;
            height: 100vh;
        }
    </style>
</head>
<body>
    <div id="editor-view"></div>
    <script src="./index.ts" type="module"></script>
</body>
</html>

@luin
Copy link
Member Author

luin commented Nov 20, 2023

@lefex Can you check out the latest commit in develop branch and see if you can reproduce it in http://localhost:9000/standalone/full? I didn't reproduce it locally in Safari 17.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Slab has a very serious issue that needs to be fixed as soon as possible
3 participants