prosemirror/prosemirror-view
1
2
Fork 0
Code Pull requests Activity

Fix selection when Firefox creates multi range selection across svg. #173

Closed
jasonklym wants to merge 1 commit from master into master
pull from: master
merge into: prosemirror:master
Conversation 10 Commits 1 Files changed 1 +10
jasonklym commented 2024-09-18 17:34:53 +02:00 (Migrated from github.com)
Copy link

Firefox selection breaks and restarts when dragging across an svg.

This issue was previous report in an unanswered thread and a JavaScript solution was written in this commit. Rewrite that JS commit in TS.

Firefox selection breaks and restarts when dragging across an `svg`. This issue was previous report in an unanswered [thread](https://discuss.prosemirror.net/t/selection-issue-in-firefox-with-certain-dom-elements-in-node-view/1817) and a JavaScript solution was written in this [commit](https://github.com/paulfab/prosemirror-view/commit/d57834eb454292b641419154887093a76ed2adea). Rewrite that JS commit in TS.
marijnh commented 2024-09-18 19:13:01 +02:00 (Migrated from github.com)
Copy link

This code seems to make some assumptions that seem dodgy — why would taking the head from the first range and the anchor from the old selection give the appropriate range? When selecting from bottom to top that sounds like it won't work.

This code seems to make some assumptions that seem dodgy — why would taking the head from the first range and the anchor from the old selection give the appropriate range? When selecting from bottom to top that sounds like it won't work.
jasonklym commented 2024-09-19 00:22:40 +02:00 (Migrated from github.com)
Copy link

Unfortunately I am not the original author of the patch. But what I observed when I tested the original fix is that when the selection breaks to become a multi-range selection, the range order does not change whether selecting from bottom to top, or from top to bottom.

Unfortunately I am not the original author of the patch. But what I observed when I tested the original fix is that when the selection breaks to become a multi-range selection, the range order does not change whether selecting from bottom to top, or from top to bottom.
marijnh commented 2024-09-19 08:54:35 +02:00 (Migrated from github.com)
Copy link

Can you still reproduce the issue on this link? With Firefox 130 Linux I don't see anything odd happening when dragging selections there.

Can you still reproduce the issue on [this link](https://glimmer-ambulance.glitch.me/)? With Firefox 130 Linux I don't see anything odd happening when dragging selections there.
jasonklym commented 2024-09-19 18:20:57 +02:00 (Migrated from github.com)
Copy link

That link was super helpful and I was able to narrow it down more. The selection only becomes mutli-range in Firefox for nodes that are atom: true—this happens for both the svg and button, as it turns out.

I don't know how I forgot that fact—both times I've run into this someone was rendering uneditable labels with svg type icons.

That link was super helpful and I was able to narrow it down more. The selection only becomes mutli-range in Firefox for nodes that are `atom: true`—this happens for both the `svg` and `button`, as it turns out. I don't know how I forgot that fact—both times I've run into this someone was rendering uneditable labels with `svg` type icons.
marijnh commented 2024-09-19 18:23:23 +02:00 (Migrated from github.com)
Copy link

You mean you can reproduce it on that link with Firefox Mac, or that something had to be changed before it reproduced? Your videos aren't loading for me (but I prefer words anyway).

You mean you can reproduce it on that link with Firefox Mac, or that something had to be changed before it reproduced? Your videos aren't loading for me (but I prefer words anyway).
jasonklym commented 2024-09-19 18:37:35 +02:00 (Migrated from github.com)
Copy link

One line change to render as though it were atom: true:

  const dom = document.createElement('div')
  dom.contentEditable = 'false'

... and it immediately starts breaking on Firefox as the selection crosses each button.

One line change to render as though it were `atom: true`: ``` const dom = document.createElement('div') dom.contentEditable = 'false' ``` ... and it immediately starts breaking on Firefox as the selection crosses each `button`.
marijnh commented 2024-09-19 19:52:24 +02:00 (Migrated from github.com)
Copy link

That'll break more than just selection, though—creating editable islands like this inside the content managed by ProseMirror isn't supported by this library. Nodes with a contentDOM should keep all ancestors of that element editable.

That'll break more than just selection, though—creating editable islands like this inside the content managed by ProseMirror isn't supported by this library. Nodes with a `contentDOM` should keep all ancestors of that element editable.
jasonklym commented 2024-09-19 21:39:02 +02:00 (Migrated from github.com)
Copy link

It is the non-editable islands that are the problem, and people make those in ProseMirror all the time. Particularly for non-editable links.

For example, when atom: true the toDOM result will set contenteditable="false" on an a tag. So we could imagine this node:

atomic_link: {
  attrs: { label: {}, href: {} },
  group: "inline",
  inline: true,
  draggable: false,
  selectable: false,
  atom: true,
  toDOM: note => ["a", node.attrs.href || node.attrs.label || "Unknown"]
}

... creating this DOM element:

<a contenteditable="false" class="">Unknown</a>

If we update toDOM to insert an svg into the a like so:

atomic_link: {
  attrs: { label: {}, href: {} },
  group: "inline",
  inline: true,
  draggable: false,
  selectable: false,
  atom: true,
  // toDOM: note => ["a", node.attrs.href || node.attrs.label || "Unknown"]
  toDOM: node => {
    const domNode = document.createElement("a");
    domNode.href = node.attrs.href || "#";
    if (node.isAtom) {
      domNode.contentEditable = "false";
    }

    const svg = document.createElementNS("http://www.w3.org/2000/svg", "svg");
    svg.setAttribute("width", "16");
    svg.setAttribute("height", "16");
    svg.setAttribute("viewBox", "0 0 16 16");

    let path = document.createElementNS("http://www.w3.org/2000/svg", "path");
    path.setAttribute(
      "d",
      "M9.64409 1.74899L9.66238 6.999L12.1653 6.99998C12.8375 6.99998 13.233 7.67956 12.8503 8.17697L7.85106 14.675C7.38603 15.2795 6.33285 14.9832 6.33285 14.248L6.33174 9.004L3.83465 9.00439C3.16343 9.00439 2.76771 8.32653 3.14846 7.82896L8.12469 1.32594C8.58786 0.720669 9.64148 1.01403 9.64409 1.74899Z"
    );

    svg.appendChild(path);

    domNode.appendChild(svg);
    domNode.append(node.attrs.label || node.attrs.href || "Label");

    return domNode;
  },
}

... creating this DOM:

<a href="#" contenteditable="false"><svg width="16" height="16" viewBox="0 0 16 16"><path d="M9.64409 1.74899L9.66238 6.999L12.1653 6.99998C12.8375 6.99998 13.233 7.67956 12.8503 8.17697L7.85106 14.675C7.38603 15.2795 6.33285 14.9832 6.33285 14.248L6.33174 9.004L3.83465 9.00439C3.16343 9.00439 2.76771 8.32653 3.14846 7.82896L8.12469 1.32594C8.58786 0.720669 9.64148 1.01403 9.64409 1.74899Z"></path></svg>Unknown</a>

... we end up with a DOM that breaks the selection in Firefox. And the only difference is the presence of the svg.

Note: I didn't use the button example, as the semantics of a button inside a a is questionable, and who knows what the browser would do.

It is the non-editable islands that are the problem, and people make those in ProseMirror all the time. Particularly for non-editable links. For example, when `atom: true` the `toDOM` result will set `contenteditable="false"` on an `a` tag. So we could imagine this node: ``` atomic_link: { attrs: { label: {}, href: {} }, group: "inline", inline: true, draggable: false, selectable: false, atom: true, toDOM: note => ["a", node.attrs.href || node.attrs.label || "Unknown"] } ``` ... creating this DOM element: ``` <a contenteditable="false" class="">Unknown</a> ``` If we update `toDOM` to insert an `svg` into the `a` like so: ``` atomic_link: { attrs: { label: {}, href: {} }, group: "inline", inline: true, draggable: false, selectable: false, atom: true, // toDOM: note => ["a", node.attrs.href || node.attrs.label || "Unknown"] toDOM: node => { const domNode = document.createElement("a"); domNode.href = node.attrs.href || "#"; if (node.isAtom) { domNode.contentEditable = "false"; } const svg = document.createElementNS("http://www.w3.org/2000/svg", "svg"); svg.setAttribute("width", "16"); svg.setAttribute("height", "16"); svg.setAttribute("viewBox", "0 0 16 16"); let path = document.createElementNS("http://www.w3.org/2000/svg", "path"); path.setAttribute( "d", "M9.64409 1.74899L9.66238 6.999L12.1653 6.99998C12.8375 6.99998 13.233 7.67956 12.8503 8.17697L7.85106 14.675C7.38603 15.2795 6.33285 14.9832 6.33285 14.248L6.33174 9.004L3.83465 9.00439C3.16343 9.00439 2.76771 8.32653 3.14846 7.82896L8.12469 1.32594C8.58786 0.720669 9.64148 1.01403 9.64409 1.74899Z" ); svg.appendChild(path); domNode.appendChild(svg); domNode.append(node.attrs.label || node.attrs.href || "Label"); return domNode; }, } ``` ... creating this DOM: ``` <a href="#" contenteditable="false"><svg width="16" height="16" viewBox="0 0 16 16"><path d="M9.64409 1.74899L9.66238 6.999L12.1653 6.99998C12.8375 6.99998 13.233 7.67956 12.8503 8.17697L7.85106 14.675C7.38603 15.2795 6.33285 14.9832 6.33285 14.248L6.33174 9.004L3.83465 9.00439C3.16343 9.00439 2.76771 8.32653 3.14846 7.82896L8.12469 1.32594C8.58786 0.720669 9.64148 1.01403 9.64409 1.74899Z"></path></svg>Unknown</a> ``` ... we end up with a DOM that breaks the selection in Firefox. And the only difference is the presence of the `svg`. Note: I didn't use the `button` example, as the semantics of a `button` inside a `a` is questionable, and who knows what the browser would do.
marijnh commented 2024-09-19 22:12:07 +02:00 (Migrated from github.com)
Copy link

It is the non-editable islands that are the problem, and people make those in ProseMirror all the time. Particularly for non-editable links.

Non-editable islands work fine. It's editable islands inside non-editable nodes that don't.

But I can reproduce this now. If I directly return <svg> elements for leaf nodes, the browser doesn't do this, but if I wrap them in an additional element, it does.

Does attached patch look reasonable to you?

> It is the non-editable islands that are the problem, and people make those in ProseMirror all the time. Particularly for non-editable links. Non-editable islands work fine. It's editable islands inside non-editable nodes that don't. But I can reproduce this now. If I directly return `<svg>` elements for leaf nodes, the browser doesn't do this, but if I wrap them in an additional element, it does. Does attached patch look reasonable to you?
jasonklym commented 2024-09-19 22:33:50 +02:00 (Migrated from github.com)
Copy link

Reverted my local patch for yours and it works perfectly.

Thank you for your patience and assistance. 🙇

Reverted my local patch for yours and it works perfectly. Thank you for your patience and assistance. 🙇

Pull request closed

This pull request cannot be reopened because the branch was deleted.
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
prosemirror/prosemirror-view!173
No description provided.