Skip to content

Commit eca48e2

Browse files
CarlaCosteaCarlaCostea
authored
Merge pull request #2275 from pie-framework/develop
fix: PIE-695, PIE-697, PIE-699, PIE-680, PIE-714, PIE-665, PIE-656
2 parents 053acd3 + 35de54d commit eca48e2

9 files changed

Lines changed: 351 additions & 53 deletions

File tree

‎packages/controller-utils/src/__tests__/persistence.test.js‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -96,9 +96,9 @@ describe('lockChoices', () => {
9696
${false} | ${undefined} | ${undefined} | ${false}
9797
${undefined} | ${session()} | ${env(false)} | ${false}
9898
${undefined} | ${session()} | ${env(undefined)} | ${false}
99-
${false} | ${session()} | ${env(false, 'instructor')} | ${true}
100-
${false} | ${session([0, 1])} | ${env(false, 'instructor')} | ${true}
101-
${false} | ${undefined} | ${env(false, 'instructor')} | ${true}
99+
${false} | ${session()} | ${env(false, 'instructor')} | ${false}
100+
${false} | ${session([0, 1])} | ${env(false, 'instructor')} | ${false}
101+
${false} | ${undefined} | ${env(false, 'instructor')} | ${false}
102102
`('1. model.lockChoiceOrder: $modelLock, $session, $env => $expected', ({ modelLock, session, env, expected }) => {
103103
const model = { lockChoiceOrder: modelLock };
104104
const result = lockChoices(model, session, env);
@@ -120,7 +120,7 @@ describe('lockChoices mod', () => {
120120
${false} | ${session()} | ${env(false)} | ${false}
121121
${undefined} | ${session()} | ${env(true)} | ${true}
122122
${undefined} | ${session()} | ${env(undefined)} | ${false}
123-
${undefined} | ${session()} | ${env(undefined, 'instructor')} | ${true}
123+
${undefined} | ${session()} | ${env(undefined, 'instructor')} | ${false}
124124
`('2. model.lockChoiceOrder: $modelLock, $session, $env => $expected', ({ modelLock, session, env, expected }) => {
125125
const model = { lockChoiceOrder: modelLock };
126126
const result = lockChoices(model, session, env);
@@ -142,7 +142,7 @@ describe('lockChoices', () => {
142142
${false} | ${session()} | ${env(false)} | ${false}
143143
${undefined} | ${session()} | ${env(true)} | ${true}
144144
${undefined} | ${session()} | ${env(undefined)} | ${false}
145-
${undefined} | ${session()} | ${env(undefined, 'instructor')} | ${true}
145+
${undefined} | ${session()} | ${env(undefined, 'instructor')} | ${false}
146146
`('3. model.lockChoiceOrder: $modelLock, $env => $expected', ({ modelLock, session, env, expected }) => {
147147
const model = { lockChoiceOrder: modelLock };
148148
const result = lockChoices(model, session, env);

‎packages/controller-utils/src/persistence.js‎

Lines changed: 4 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,10 @@ export const getShuffledChoices = (choices, session, updateSession, choiceKey) =
6969
* - true - that means that the order of the choices will be ordinal (as is created in the configure item)
7070
* - false - that means the getShuffledChoices above will be called and that in turn means that we either
7171
* return the shuffled values on the session (if any exists) or we shuffle the choices
72+
*
73+
* Note: the role (student/instructor) is intentionally not considered here — instructor mode
74+
* will respect the same `lockChoiceOrder` value as students, instead of forcing the order to be locked.
75+
*
7276
* @param model - model to check if we should lock order
7377
* @param session - session to check if we should lock order
7478
* @param env - env to check if we should lock order
@@ -85,20 +89,5 @@ export const lockChoices = (model, session, env) => {
8589
return true;
8690
}
8791

88-
const role = get(env, 'role', 'student');
89-
90-
if (role === 'instructor') {
91-
// TODO: .. in the future the instructor can toggle between ordinal and shuffled here, so keeping this code until then
92-
/*const alreadyShuffled = hasShuffledValues(session);
93-
94-
if (alreadyShuffled) {
95-
return false;
96-
}
97-
98-
return true;*/
99-
return true;
100-
}
101-
102-
// here it's a student, so don't lock and it will shuffle if needs be
10392
return false;
10493
};

‎packages/editable-html-tip-tap/src/components/EditableHtml.jsx‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -334,7 +334,7 @@ export const EditableHtml = (props) => {
334334
const nextMarkup = normalizeInitialMarkup(props.markup);
335335

336336
if (nextMarkup !== editor.getHTML()) {
337-
editor.commands.setContent(nextMarkup, false);
337+
editor.commands.setContent(nextMarkup, { emitUpdate: false });
338338
}
339339
}, [props.markup, editor]);
340340

‎packages/editable-html-tip-tap/src/extensions/__tests__/math.test.js‎

Lines changed: 146 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import React from 'react';
2-
import { render, waitFor, fireEvent } from '@testing-library/react';
2+
import { render, waitFor, fireEvent, act } from '@testing-library/react';
33
import { EnsureTextAfterMathPlugin, MathNode, MathNodeView, ZeroWidthSpaceHandlingPlugin } from '../math';
44
import * as toolbarUtils from '../../utils/toolbar';
55

@@ -390,6 +390,7 @@ describe('MathNodeView', () => {
390390
selection: {
391391
from: 0,
392392
to: 1,
393+
node: { type: { name: 'math' } },
393394
},
394395
tr: {
395396
setSelection: jest.fn().mockReturnThis(),
@@ -467,30 +468,31 @@ describe('MathNodeView', () => {
467468
});
468469

469470
describe('toolbar positioning', () => {
470-
it('positions relative to the editor element using coordsAtPos', async () => {
471+
it('positions relative to portal container using coordsAtPos', async () => {
471472
const { container } = render(<MathNodeView {...defaultProps} selected={true} />);
472473
await waitFor(() => {
473474
const toolbar = container.querySelector('[data-toolbar-for]');
474475
expect(toolbar).toBeInTheDocument();
475-
expect(toolbar.style.top).toBe('140px');
476+
expect(toolbar.style.top).toBe('100px');
476477
expect(toolbar.style.left).toBe('50px');
477478
});
478479
});
479480

480-
it('accounts for editor scroll offset when calculating toolbar position', async () => {
481-
const editorElement = createEditorElement({ top: -200, left: 0, width: 600, height: 400 });
481+
it('offsets position by portal container getBoundingClientRect', async () => {
482+
const containerEl = document.createElement('div');
483+
containerEl.getBoundingClientRect = jest.fn(() => ({ top: 100, left: 50, width: 600, height: 400 }));
482484

483485
const editor = {
484486
...defaultProps.editor,
485-
options: { element: editorElement },
487+
_tiptapContainerEl: containerEl,
486488
};
487489

488490
const { container } = render(<MathNodeView {...defaultProps} editor={editor} selected={true} />);
489491
await waitFor(() => {
490492
const toolbar = container.querySelector('[data-toolbar-for]');
491493
expect(toolbar).toBeInTheDocument();
492-
expect(toolbar.style.top).toBe('340px');
493-
expect(toolbar.style.left).toBe('50px');
494+
expect(toolbar.style.top).toBe('0px');
495+
expect(toolbar.style.left).toBe('0px');
494496
});
495497
});
496498

@@ -503,6 +505,15 @@ describe('MathNodeView', () => {
503505
});
504506
});
505507

508+
it('renders above other editor overlays with a high z-index', async () => {
509+
const { container } = render(<MathNodeView {...defaultProps} selected={true} />);
510+
await waitFor(() => {
511+
const toolbar = container.querySelector('[data-toolbar-for]');
512+
expect(toolbar).toBeInTheDocument();
513+
expect(toolbar.style.zIndex).toBe('1000');
514+
});
515+
});
516+
506517
it('updates position from coordsAtPos when selection changes', async () => {
507518
const editor = {
508519
...defaultProps.editor,
@@ -517,11 +528,64 @@ describe('MathNodeView', () => {
517528
await waitFor(() => {
518529
const toolbar = container.querySelector('[data-toolbar-for]');
519530
expect(toolbar).toBeInTheDocument();
520-
expect(toolbar.style.top).toBe('240px');
531+
expect(toolbar.style.top).toBe('200px');
521532
expect(toolbar.style.left).toBe('150px');
522533
});
523534
});
524535

536+
it('clamps toolbar position to viewport margins', async () => {
537+
const originalInnerHeight = window.innerHeight;
538+
const originalInnerWidth = window.innerWidth;
539+
540+
Object.defineProperty(window, 'innerHeight', { configurable: true, writable: true, value: 200 });
541+
Object.defineProperty(window, 'innerWidth', { configurable: true, writable: true, value: 300 });
542+
543+
const editor = {
544+
...defaultProps.editor,
545+
view: {
546+
...defaultProps.editor.view,
547+
coordsAtPos: jest.fn(() => ({ top: 190, left: 280, bottom: 195 })),
548+
dispatch: jest.fn(),
549+
},
550+
};
551+
552+
const { container } = render(<MathNodeView {...defaultProps} editor={editor} selected={true} />);
553+
554+
let toolbar;
555+
await waitFor(() => {
556+
toolbar = container.querySelector('[data-toolbar-for]');
557+
expect(toolbar).toBeInTheDocument();
558+
});
559+
560+
Object.defineProperty(toolbar, 'offsetHeight', { configurable: true, value: 100 });
561+
Object.defineProperty(toolbar, 'offsetWidth', { configurable: true, value: 150 });
562+
563+
await act(async () => {
564+
window.dispatchEvent(new Event('resize'));
565+
await new Promise((resolve) => requestAnimationFrame(resolve));
566+
});
567+
568+
await waitFor(() => {
569+
expect(parseInt(toolbar.style.top, 10)).toBeLessThanOrEqual(200 - 100 - 8);
570+
expect(parseInt(toolbar.style.left, 10)).toBeLessThanOrEqual(300 - 150 - 8);
571+
expect(parseInt(toolbar.style.top, 10)).toBeGreaterThanOrEqual(8);
572+
expect(parseInt(toolbar.style.left, 10)).toBeGreaterThanOrEqual(8);
573+
});
574+
575+
Object.defineProperty(window, 'innerHeight', { configurable: true, writable: true, value: originalInnerHeight });
576+
Object.defineProperty(window, 'innerWidth', { configurable: true, writable: true, value: originalInnerWidth });
577+
});
578+
579+
it('attaches scroll and resize listeners while toolbar is open', async () => {
580+
const addSpy = jest.spyOn(window, 'addEventListener');
581+
render(<MathNodeView {...defaultProps} selected={true} />);
582+
await waitFor(() => {
583+
expect(addSpy).toHaveBeenCalledWith('scroll', expect.any(Function), true);
584+
expect(addSpy).toHaveBeenCalledWith('resize', expect.any(Function));
585+
});
586+
addSpy.mockRestore();
587+
});
588+
525589
it('portals toolbar into _tiptapContainerEl when available', async () => {
526590
const containerEl = document.createElement('div');
527591
containerEl.getBoundingClientRect = jest.fn(() => ({ top: 0, left: 0, width: 600, height: 400 }));
@@ -648,6 +712,31 @@ describe('MathNodeView', () => {
648712
});
649713
});
650714

715+
it('re-registers click listener when node changes', async () => {
716+
const addEventListenerSpy = jest.spyOn(document, 'addEventListener');
717+
const removeEventListenerSpy = jest.spyOn(document, 'removeEventListener');
718+
const nodeA = { attrs: { latex: 'x^2' } };
719+
const nodeB = { attrs: { latex: 'y^2' } };
720+
721+
const { rerender } = render(<MathNodeView {...defaultProps} node={nodeA} selected={true} />);
722+
723+
await waitFor(() => {
724+
expect(addEventListenerSpy).toHaveBeenCalledWith('click', expect.any(Function));
725+
});
726+
727+
const initialCallCount = addEventListenerSpy.mock.calls.length;
728+
729+
rerender(<MathNodeView {...defaultProps} node={nodeB} selected={true} />);
730+
731+
await waitFor(() => {
732+
expect(removeEventListenerSpy).toHaveBeenCalled();
733+
expect(addEventListenerSpy.mock.calls.length).toBeGreaterThan(initialCallCount);
734+
});
735+
736+
addEventListenerSpy.mockRestore();
737+
removeEventListenerSpy.mockRestore();
738+
});
739+
651740
it('does not close toolbar when clicking the math node preview', async () => {
652741
const { getByTestId, queryByTestId } = render(<MathNodeView {...defaultProps} selected={true} />);
653742

@@ -724,6 +813,54 @@ describe('MathNodeView', () => {
724813
});
725814
});
726815

816+
describe('selection-based toolbar guard', () => {
817+
it('opens toolbar when selected transitions to true and the editor has a NodeSelection on math', async () => {
818+
const { queryByTestId, rerender } = render(<MathNodeView {...defaultProps} selected={false} />);
819+
expect(queryByTestId('math-toolbar')).not.toBeInTheDocument();
820+
821+
rerender(<MathNodeView {...defaultProps} selected={true} />);
822+
await waitFor(() => {
823+
expect(queryByTestId('math-toolbar')).toBeInTheDocument();
824+
});
825+
});
826+
827+
it('does not open toolbar when selected briefly becomes true but editor selection has no node (Cmd+A / drag case)', async () => {
828+
const editor = {
829+
...defaultProps.editor,
830+
state: {
831+
...defaultProps.editor.state,
832+
selection: { from: 0, to: 100 }, // no .node — TextSelection / AllSelection shape
833+
},
834+
};
835+
836+
const { queryByTestId, rerender } = render(
837+
<MathNodeView {...defaultProps} editor={editor} selected={false} />,
838+
);
839+
rerender(<MathNodeView {...defaultProps} editor={editor} selected={true} />);
840+
841+
await act(async () => {});
842+
expect(queryByTestId('math-toolbar')).not.toBeInTheDocument();
843+
});
844+
845+
it('does not open toolbar when selected briefly becomes true but NodeSelection targets a non-math node', async () => {
846+
const editor = {
847+
...defaultProps.editor,
848+
state: {
849+
...defaultProps.editor.state,
850+
selection: { from: 0, to: 1, node: { type: { name: 'image' } } },
851+
},
852+
};
853+
854+
const { queryByTestId, rerender } = render(
855+
<MathNodeView {...defaultProps} editor={editor} selected={false} />,
856+
);
857+
rerender(<MathNodeView {...defaultProps} editor={editor} selected={true} />);
858+
859+
await act(async () => {});
860+
expect(queryByTestId('math-toolbar')).not.toBeInTheDocument();
861+
});
862+
});
863+
727864
it('does not close toolbar when clicking equation editor dropdown', async () => {
728865
const { queryByTestId } = render(<MathNodeView {...defaultProps} selected={true} />);
729866

‎packages/editable-html-tip-tap/src/extensions/css.js‎

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -169,14 +169,27 @@ export const CSSMark = Mark.create({
169169
},
170170

171171
parseHTML() {
172-
// Any span with a class that matches one of allowed classes
172+
// Any span with a class that matches one of allowed classes any span that carries a class attribute
173+
// so that pre-existing spans are preserved when loading content
173174
return [
174175
{
175176
tag: 'span[class]',
176177
getAttrs: (el) => {
177178
const cls = el.getAttribute('class') || '';
178-
const match = this.options.classes.find((name) => cls.includes(name));
179-
return match ? { class: match } : false;
179+
180+
if (!cls) {
181+
return false;
182+
}
183+
184+
const allowedClasses = (this.options && this.options.classes) || [];
185+
186+
if (allowedClasses.length > 0) {
187+
const match = this.options.classes.find((name) => cls.includes(name));
188+
189+
return match ? { class: match } : false;
190+
}
191+
192+
return { class: cls };
180193
},
181194
},
182195
];

0 commit comments

Comments
 (0)