Fix table selector and resizer #1919
Conversation
| @@ -41,12 +47,13 @@ export default function createTableSelector( | |||
| div.id = TABLE_SELECTOR_ID; | |||
| div.style.width = `${TABLE_SELECTOR_LENGTH}px`; | |||
| div.style.height = `${TABLE_SELECTOR_LENGTH}px`; | |||
| document.body.appendChild(div); | |||
| table.insertAdjacentElement('beforebegin', div); | |||
There was a problem hiding this comment.
So this is to insert the handle DIV into editor content, is that right? If an auto save happens while the resize handle is showing, it will be saved as part of the content
There was a problem hiding this comment.
Maybe it is ok to insert here. But we need to remove it when getContent() is called, by handling ExtractContentWithDomEvent event
…u/acampostams/fix-table-selector-and-resizer
| @@ -85,6 +85,8 @@ export default class TableResize implements EditorPlugin { | |||
| case PluginEventType.ContentChanged: | |||
| case PluginEventType.Scroll: | |||
| case PluginEventType.ZoomChanged: | |||
| case PluginEventType.ExtractContentWithDom: | |||
There was a problem hiding this comment.
This is not what I mean. You need to handle extractContentWithDom event separately, it will pass you a DocumentFragment, and you just need to remove the selector from that fragment, but no need to remove it from current editor. (The passed-in fragment is a cloned DOM tree of editor, so operating on that tree won't imapct editor)
That means, we just don't want the table selector to be saved, but from current editor, it should still show. User's resizing action should not be interrupted by auto save.
There was a problem hiding this comment.
How can I test auto save?
There was a problem hiding this comment.
To simulate an auto save, you can add code to call window.setTimeout() with a delayed time, say 10sec. And in the callback you can call editor.getContent(), which will trigger this event.
| @@ -35,7 +35,7 @@ export default function createTableResizer( | |||
|
|
|||
| div.style.width = `${TABLE_RESIZER_LENGTH}px`; | |||
| div.style.height = `${TABLE_RESIZER_LENGTH}px`; | |||
| document.body.appendChild(div); | |||
| table.insertAdjacentElement('afterend', div); | |||
There was a problem hiding this comment.
do we also need to do the same change for other table resize helper elements? Actually there are 6:
- Whole table selector
- Whole table resizer (these 2 are visible as a gray square)
- horizontal resizer
- vertical resizer
- new column inserter (a circle with "+" sign when hover above a table)
- new row inserter (a circle with "+" sign when hover on left side (or right side when RTL) of a table)
3,4 are invisible, but it will change mouse cursor when hover
5,6 will be visible when hover.
I can see your change fixed 1 and 2, so we also need same fix for 3-6. And maybe we should have a unified function for all of them to reduce duplicated code.
There was a problem hiding this comment.
I noticed that but wanted to do that on a separate PR. I think a better approach would be to do something like Image Edit does, where the helpers are in in a span.
There was a problem hiding this comment.
It's ok to do it separately, and please open an issue to track it.
I think it should be fine to use a SPAN for all the resizer UI, although this seems like a larger change.
…u/acampostams/fix-table-selector-and-resizer
…//github.com/microsoft/roosterjs into u/acampostams/fix-table-selector-and-resizer
packages/roosterjs-editor-plugins/lib/plugins/TableResize/TableResize.ts
Outdated
Show resolved
Hide resolved
packages/roosterjs-editor-plugins/lib/plugins/TableResize/editors/TableResizer.ts
Outdated
Show resolved
Hide resolved
…u/acampostams/fix-table-selector-and-resizer
packages/roosterjs-editor-plugins/lib/plugins/TableResize/editors/TableEditor.ts
Show resolved
Hide resolved
This reverts commit 5be7a7d.
Description:
The Table selector and resizer are currently inserted on the document, thus putting them on a different container to the table they belong to. This creates issues in some scenarios such as when a dialog is created above the document with a high z-index. They also have a fixed position which causes issues in some scrolling scenarios, where they stayed fixed in place instead of moving or disappearing.
Fix
Changed the insertion point of where the squares are located. The Selector is inserted before the table and the Resizer after it. Also changed its position from fixed to absolute and made changes to the CSS.
Change
Fixed scenarios:
Before:
After:
Warnings and implications