Add background to maintain selection highlight #2636
Conversation
FrancisMengx
force-pushed
the
u/xiameng/add-background-for-copilot-treatment
branch
from
ef023d1 to
b42f20a
Compare
|
Please fix test |
|
Please add description |
|
Question: how will this new feature be used? It seems the new option is not exposed to any public API? |
FrancisMengx
force-pushed
the
u/xiameng/add-background-for-copilot-treatment
branch
from
cc77651 to
4ed3e67
Compare
| } | ||
| } else { | ||
| core.api.setEditorStyle(core, SelectionClassName, null /*rule*/); | ||
| CSS.highlights.delete(SelectionClassName); |
There was a problem hiding this comment.
do we need to check "highlights in CSS" here?
|
|
||
| if (option?.shouldMaintainSelection) { | ||
| if ('highlights' in CSS && Highlight) { | ||
| const selectionEl = document.querySelector(SelectionSelector); |
There was a problem hiding this comment.
get document from core.physicalRoot to make it work for projection popout
| @@ -15,6 +18,28 @@ import type { SetContentModel } from 'roosterjs-content-model-types'; | |||
| */ | |||
| export const setContentModel: SetContentModel = (core, model, option, onNodeCreated) => { | |||
| const editorContext = core.api.createEditorContext(core, true /*saveIndex*/); | |||
|
|
|||
| if (option?.shouldMaintainSelection) { | |||
There was a problem hiding this comment.
This need to move down after line 57, and use the selection range from the return value, but not rely on CSS. otherwise what if there are multiple selected elements
| @@ -64,7 +64,12 @@ export const formatContentModel: FormatContentModel = ( | |||
| core.api.setContentModel( | |||
| core, | |||
| model, | |||
| hasFocus ? undefined : { ignoreSelection: true }, // If editor did not have focus before format, do not set focus after format | |||
| hasFocus | |||
There was a problem hiding this comment.
This should be:
hasFocus || options?.shouldMaintainSelection ? {
ignoreSelection: hasfocus, // ..... (move the comment here)
shouldMaintainSelection: options?.shouldMaintainSelection
} : undefined
|
please resolve the conflict and fix build |
FrancisMengx
force-pushed
the
u/xiameng/add-background-for-copilot-treatment
branch
from
c1ceed2 to
b49b845
Compare
FrancisMengx
force-pushed
the
u/xiameng/add-background-for-copilot-treatment
branch
from
46998fb to
3e9397d
Compare
…s://github.com/microsoft/roosterjs into u/xiameng/add-background-for-copilot-treatment
| @@ -15,6 +18,18 @@ import type { SetContentModel } from 'roosterjs-content-model-types'; | |||
| */ | |||
| export const setContentModel: SetContentModel = (core, model, option, onNodeCreated) => { | |||
| const editorContext = core.api.createEditorContext(core, true /*saveIndex*/); | |||
| const currentWindow = core.logicalRoot.ownerDocument.defaultView; | |||
| if (currentWindow && isWindowWithHighlight(currentWindow)) { | |||
There was a problem hiding this comment.
I feel we should check "!" for isWindowWithHighlight(), we only need this CSS based solution when browser does not support highlight, right?
| /** | ||
| * When pass to true, selection is maintained even when focus is moved out of editor. | ||
| */ | ||
| shouldMaintainSelection?: boolean; |
There was a problem hiding this comment.
Let's use a simpler name, for example: highlightSelection
| } | ||
|
|
||
| /** | ||
| * @internal |
There was a problem hiding this comment.
nit: if it is not exported, no need to add "@internal"
There was a problem hiding this comment.
same for other places
| @@ -31,6 +32,7 @@ export const formatContentModel: FormatContentModel = ( | |||
| rawEvent, | |||
| selectionOverride, | |||
| scrollCaretIntoView: scroll, | |||
| shouldMaintainSelection, | |||
There was a problem hiding this comment.
same here, use a simpler name
Changes
Add a new option in the setContentModel function to indicate maintaing selection UI even when the focus has moved away.