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.

Sign up for GitHub

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

Jump to bottom

Fix table selector and resizer #1919

Merged
merged 11 commits into from
Jun 30, 2023
Merged

Fix table selector and resizer #1919

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
85f7c7d
css fix
Andres-CT98 Jun 27, 2023
9391f35
added plugin events
Andres-CT98 Jun 29, 2023
ef52556
Merge branch 'master' of https://github.com/microsoft/roosterjs into …
Andres-CT98 Jun 29, 2023
6f7ea4a
Merge branch 'master' into u/acampostams/fix-table-selector-and-resizer
Andres-CT98 Jun 29, 2023
7bc81f4
remove from editor if getContent()
Andres-CT98 Jun 29, 2023
d7ea4ee
Merge branch 'master' of https://github.com/microsoft/roosterjs into …
Andres-CT98 Jun 29, 2023
ea200aa
Merge branch 'u/acampostams/fix-table-selector-and-resizer' of https:…
Andres-CT98 Jun 29, 2023
01ca812
fix
Andres-CT98 Jun 29, 2023
1e5bd1a
made constants
Andres-CT98 Jun 29, 2023
7f14ae4
Merge branch 'master' of https://github.com/microsoft/roosterjs into …
Andres-CT98 Jun 29, 2023
52e4c34
added comment
Andres-CT98 Jun 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions packages/roosterjs-editor-plugins/lib/plugins/TableResize/TableResize.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,8 @@ export default class TableResize implements EditorPlugin {
case PluginEventType.ContentChanged:
case PluginEventType.Scroll:
case PluginEventType.ZoomChanged:
case PluginEventType.ExtractContentWithDom:
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How can I test auto save?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

case PluginEventType.SelectionChanged:
this.setTableEditor(null);
this.invalidateTableRects();
break;
Expand Down
13 changes: 6 additions & 7 deletions packages/roosterjs-editor-plugins/lib/plugins/TableResize/editors/TableResizer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ export default function createTableResizer(
const document = table.ownerDocument;
const createElementData = {
tag: 'div',
style: `position: fixed; cursor: ${
style: `position: absolute; cursor: ${
isRTL ? 'ne' : 'nw'
}-resize; user-select: none; border: 1px solid #808080`,
};
Expand All @@ -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);
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we also need to do the same change for other table resize helper elements? Actually there are 6:

  1. Whole table selector
  2. Whole table resizer (these 2 are visible as a gray square)
  3. horizontal resizer
  4. vertical resizer
  5. new column inserter (a circle with "+" sign when hover above a table)
  6. 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.


const context: DragAndDropContext = {
isRTL,
Expand Down Expand Up @@ -138,13 +138,12 @@ function onDragging(
}

function setResizeDivPosition(context: DragAndDropContext, trigger: HTMLElement) {
const { table, isRTL } = context;
const { table, isRTL, zoomScale } = context;
const rect = normalizeRect(table.getBoundingClientRect());

if (rect) {
trigger.style.top = `${rect.bottom}px`;
trigger.style.left = isRTL
? `${rect.left - TABLE_RESIZER_LENGTH - 2}px`
: `${rect.right}px`;
isRTL
? (trigger.style.marginRight = `${(rect.right - rect.left) / zoomScale}px`)
: (trigger.style.marginLeft = `${(rect.right - rect.left) / zoomScale}px`);
}
}
22 changes: 16 additions & 6 deletions packages/roosterjs-editor-plugins/lib/plugins/TableResize/editors/TableSelector.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,13 @@
import DragAndDropHandler from '../../../pluginUtils/DragAndDropHandler';
import DragAndDropHelper from '../../../pluginUtils/DragAndDropHelper';
import TableEditorFeature from './TableEditorFeature';
import { createElement, normalizeRect, safeInstanceOf } from 'roosterjs-editor-dom';
import { CreateElementData, IEditor, Rect } from 'roosterjs-editor-types';
import {
createElement,
getComputedStyle,
normalizeRect,
safeInstanceOf,
} from 'roosterjs-editor-dom';

const TABLE_SELECTOR_LENGTH = 12;
const TABLE_SELECTOR_ID = '_Table_Selector';
Expand Down Expand Up @@ -31,7 +36,8 @@ export default function createTableSelector(
const document = table.ownerDocument;
const createElementData = {
tag: 'div',
style: 'position: fixed; cursor: all-scroll; user-select: none; border: 1px solid #808080',
style:
'position: absolute; cursor: all-scroll; user-select: none; border: 1px solid #808080;',
};

onShowHelperElement?.(createElementData, 'TableSelector');
Expand All @@ -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);
Copy link
Collaborator

@JiuqingSong JiuqingSong Jun 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe it is ok to insert here. But we need to remove it when getContent() is called, by handling ExtractContentWithDomEvent event


const context: TableSelectorContext = {
table,
zoomScale,
rect,
isRTL: getComputedStyle(table, 'direction') == 'rtl',
};

setSelectorDivPosition(context, div);
Expand Down Expand Up @@ -76,6 +83,7 @@ interface TableSelectorContext {
table: HTMLTableElement;
zoomScale: number;
rect: Rect | null;
isRTL: boolean;
}

interface TableSelectorInitValue {
Expand Down Expand Up @@ -109,10 +117,12 @@ class TableSelectorFeature extends DragAndDropHelper<TableSelectorContext, Table
}

function setSelectorDivPosition(context: TableSelectorContext, trigger: HTMLElement) {
const { rect } = context;
const { rect, zoomScale } = context;
if (rect) {
trigger.style.top = `${rect.top - TABLE_SELECTOR_LENGTH}px`;
trigger.style.left = `${rect.left - TABLE_SELECTOR_LENGTH - 2}px`;
trigger.style.marginTop = `${-TABLE_SELECTOR_LENGTH - 1}px`;
context.isRTL
? (trigger.style.marginRight = `${(rect.right - rect.left) / zoomScale}px`)
: (trigger.style.marginLeft = `${-TABLE_SELECTOR_LENGTH - 1}px`);
}
}

Expand Down