diff --git a/.changeset/diagnostics-summary-callback.md b/.changeset/diagnostics-summary-callback.md new file mode 100644 index 000000000..732474b02 --- /dev/null +++ b/.changeset/diagnostics-summary-callback.md @@ -0,0 +1,9 @@ +--- +"@doenet/doenetml": patch +"@doenet/standalone": patch +"@doenet/doenetml-iframe": patch +"@doenet/vscode-extension": patch +"doenet-vscode-extension": patch +--- + +Replace `isAccessibleCallback` with `diagnosticsSummaryCallback` in `DoenetEditor`. The new callback receives an object with counts for `warningsCount`, `errorsCount`, `infosCount`, `accessibilityLevel1Count`, and `accessibilityLevel2Count` instead of a single boolean. The callback is only invoked after diagnostics have been received from the viewer. diff --git a/packages/doenetml/src/EditorViewer/EditorViewer.tsx b/packages/doenetml/src/EditorViewer/EditorViewer.tsx index 3d172b6f6..2d0b2e698 100644 --- a/packages/doenetml/src/EditorViewer/EditorViewer.tsx +++ b/packages/doenetml/src/EditorViewer/EditorViewer.tsx @@ -24,6 +24,7 @@ import { ViewerControlsBar } from "./ViewerControlsBar"; import "./editor-viewer.css"; import { useTabStore } from "@ariakit/react"; import { setVariantsFromCallback } from "../utils/variants"; +import type { DiagnosticsSummary } from "./diagnostics"; import { mergeDiagnosticsByType, toAdditionalDiagnosticsForLsp, @@ -47,7 +48,7 @@ export function EditorViewer({ doenetmlChangeCallback, immediateDoenetmlChangeCallback, documentStructureCallback, - isAccessibleCallback, + diagnosticsSummaryCallback, id: specifiedId, readOnly = false, showFormatter = true, @@ -71,7 +72,9 @@ export function EditorViewer({ doenetmlChangeCallback?: Function; immediateDoenetmlChangeCallback?: Function; documentStructureCallback?: Function; - isAccessibleCallback?: (isAccessible: boolean) => void; + diagnosticsSummaryCallback?: ( + diagnosticsSummary: DiagnosticsSummary, + ) => void; id?: string; readOnly?: boolean; showFormatter?: boolean; @@ -131,11 +134,18 @@ export function EditorViewer({ const [infoPanelIsOpen, setInfoPanelIsOpen] = useState(false); const [diagnostics, setDiagnostics] = useState([]); + // Track whether we've received diagnostics from the viewer at least once. + // This is used to gate when we call the diagnosticsSummaryCallback to avoid sending + // just the initial diagnostics on load, which would be misleading since that + // does not include most of the diagnostics, which are generated by the viewer. + const [receivedDiagnosticsFromViewer, setReceivedDiagnosticsFromViewer] = + useState(false); const [showInfoAnnotations, setShowInfoAnnotations] = useState(false); /** Receives diagnostics from DocViewer and stores them for panel/LSP sync. */ function setDiagnosticsCallback(newDiagnostics: DiagnosticRecord[]) { setDiagnostics(newDiagnostics); + setReceivedDiagnosticsFromViewer(true); } useEffect(() => { @@ -155,6 +165,9 @@ export function EditorViewer({ errors: errorsObjs, infos: infoObjs, accessibility: accessibilityObjs, + warningsCount, + errorsCount, + infosCount, accessibilityLevel1Count, accessibilityLevel2Count, } = useMemo( @@ -166,9 +179,44 @@ export function EditorViewer({ [initialDiagnostics, diagnostics], ); + /** + * Memoize the diagnosticsSummary object so its reference is stable when + * diagnostic counts don't change. This prevents unnecessary re-invocations + * of the effect callback and avoids potential render loops if a consumer + * stores this object in React state and passes an inline callback. + */ + const diagnosticsSummary = useMemo( + () => ({ + warningsCount, + errorsCount, + infosCount, + accessibilityLevel1Count, + accessibilityLevel2Count, + }), + [ + warningsCount, + errorsCount, + infosCount, + accessibilityLevel1Count, + accessibilityLevel2Count, + ], + ); + useEffect(() => { - isAccessibleCallback?.(accessibilityLevel1Count === 0); - }, [accessibilityLevel1Count, isAccessibleCallback]); + // On initial load of the editor, don't call `diagnosticsSummaryCallback` + // until the viewer has sent diagnostics so avoid sending just the initial diagnostics. + // We do not need to reset `receivedDiagnosticsFromViewer`, as this guard is just + // needed to avoid calling `diagnosticsSummaryCallback` on the initial load of the editor. + if (!receivedDiagnosticsFromViewer) { + return; + } + + diagnosticsSummaryCallback?.(diagnosticsSummary); + }, [ + diagnosticsSummary, + receivedDiagnosticsFromViewer, + diagnosticsSummaryCallback, + ]); const [responses, setResponses] = useState< { diff --git a/packages/doenetml/src/EditorViewer/diagnostics.ts b/packages/doenetml/src/EditorViewer/diagnostics.ts index f37d43e72..d039d7d69 100644 --- a/packages/doenetml/src/EditorViewer/diagnostics.ts +++ b/packages/doenetml/src/EditorViewer/diagnostics.ts @@ -1,10 +1,11 @@ -import { +import type { + AccessibilityRecord, DiagnosticRecord, ErrorRecord, InfoRecord, WarningRecord, - isAccessibilityRecord, } from "@doenet/utils"; +import { isAccessibilityRecord } from "@doenet/utils"; import { Diagnostic, DiagnosticSeverity, @@ -13,6 +14,15 @@ import { /** CodeMirror/LSP diagnostic with optional editor mark class metadata. */ export type EditorLspDiagnostic = Diagnostic & { markClass?: string }; +/** Aggregated counts emitted for editor diagnostics callbacks. */ +export type DiagnosticsSummary = { + warningsCount: number; + errorsCount: number; + infosCount: number; + accessibilityLevel1Count: number; + accessibilityLevel2Count: number; +}; + /** * Converts a single editor diagnostic into an LSP-compatible diagnostic. * Accessibility diagnostics are surfaced as warnings with custom source/markClass. @@ -101,7 +111,12 @@ export function mergeDiagnosticsByType({ }: { initialDiagnostics: DiagnosticRecord[]; diagnostics: DiagnosticRecord[]; -}) { +}): { + warnings: WarningRecord[]; + errors: ErrorRecord[]; + infos: InfoRecord[]; + accessibility: AccessibilityRecord[]; +} & DiagnosticsSummary { const warnings = [ ...initialDiagnostics.filter( (diagnostic): diagnostic is WarningRecord => @@ -145,6 +160,9 @@ export function mergeDiagnosticsByType({ errors, infos, accessibility, + warningsCount: warnings.length, + errorsCount: errors.length, + infosCount: infos.length, accessibilityLevel1Count: accessibility.filter( (diagnostic) => diagnostic.level === 1, ).length, diff --git a/packages/doenetml/src/doenetml.tsx b/packages/doenetml/src/doenetml.tsx index 331c18f01..d2584136e 100644 --- a/packages/doenetml/src/doenetml.tsx +++ b/packages/doenetml/src/doenetml.tsx @@ -4,6 +4,7 @@ import React, { useCallback, useEffect, useRef, useState } from "react"; import { DocViewer } from "./Viewer/DocViewer"; import { MathJaxContext } from "better-react-mathjax"; import { mathjaxConfig, isErrorRecord, isWarningRecord } from "@doenet/utils"; +import type { DiagnosticsSummary } from "./EditorViewer/diagnostics"; import type { DiagnosticRecord, ErrorRecord, @@ -323,7 +324,7 @@ export function DoenetEditor({ doenetmlChangeCallback, immediateDoenetmlChangeCallback, documentStructureCallback, - isAccessibleCallback, + diagnosticsSummaryCallback, id, readOnly = false, showFormatter = true, @@ -353,7 +354,9 @@ export function DoenetEditor({ doenetmlChangeCallback?: Function; immediateDoenetmlChangeCallback?: Function; documentStructureCallback?: Function; - isAccessibleCallback?: (isAccessible: boolean) => void; + diagnosticsSummaryCallback?: ( + diagnosticsSummary: DiagnosticsSummary, + ) => void; id?: string; readOnly?: boolean; showFormatter?: boolean; @@ -425,7 +428,7 @@ export function DoenetEditor({ doenetmlChangeCallback={doenetmlChangeCallback} immediateDoenetmlChangeCallback={immediateDoenetmlChangeCallback} documentStructureCallback={documentStructureCallback} - isAccessibleCallback={isAccessibleCallback} + diagnosticsSummaryCallback={diagnosticsSummaryCallback} id={id} readOnly={readOnly} showFormatter={showFormatter} diff --git a/packages/test-cypress/cypress/e2e/EditorViewer/editorViewerAccessibilityReport.cy.js b/packages/test-cypress/cypress/e2e/EditorViewer/editorViewerAccessibilityReport.cy.js index 93d4017be..3c3406222 100644 --- a/packages/test-cypress/cypress/e2e/EditorViewer/editorViewerAccessibilityReport.cy.js +++ b/packages/test-cypress/cypress/e2e/EditorViewer/editorViewerAccessibilityReport.cy.js @@ -89,7 +89,7 @@ describe( ); }); - it("isAccessibleCallback reports false when level 1 accessibility issues exist", () => { + it("diagnosticsSummaryCallback reports level 1 accessibility issue", () => { postDoenetML(` Low contrast text @@ -98,11 +98,14 @@ describe( cy.get("#p112").should("contain.text", "Low contrast text"); cy.window().should((win) => { - expect(win.returnIsAccessibleCallbackValue()).eq(false); + expect( + win.returnDiagnosticsSummaryCallbackValue() + ?.accessibilityLevel1Count, + ).eq(1); }); }); - it("isAccessibleCallback reports true when no level 1 accessibility issues exist", () => { + it("diagnosticsSummaryCallback reports no level 1 accessibility issue", () => { postDoenetML(` Good contrast text @@ -111,7 +114,44 @@ describe( cy.get("#p113").should("contain.text", "Good contrast text"); cy.window().should((win) => { - expect(win.returnIsAccessibleCallbackValue()).eq(true); + expect( + win.returnDiagnosticsSummaryCallbackValue() + ?.accessibilityLevel1Count, + ).eq(0); + }); + }); + + it("diagnosticsSummaryCallback reports warning", () => { + postDoenetML(` + hello + $abc +`); + + cy.get("#t").should("contain.text", "hello"); + + cy.window().should((win) => { + const diagnosticsSummary = + win.returnDiagnosticsSummaryCallbackValue(); + expect(diagnosticsSummary?.warningsCount).eq(1); + expect(diagnosticsSummary?.errorsCount).eq(0); + expect(diagnosticsSummary?.infosCount).eq(0); + }); + }); + + it("diagnosticsSummaryCallback reports error", () => { + postDoenetML(` + hello + +`); + + cy.get("#t").should("contain.text", "hello"); + + cy.window().should((win) => { + const diagnosticsSummary = + win.returnDiagnosticsSummaryCallbackValue(); + expect(diagnosticsSummary?.warningsCount).eq(0); + expect(diagnosticsSummary?.errorsCount).eq(1); + expect(diagnosticsSummary?.infosCount).eq(0); }); }); diff --git a/packages/test-cypress/src/CypressTest.tsx b/packages/test-cypress/src/CypressTest.tsx index 8cdd32c4b..f649b4af6 100644 --- a/packages/test-cypress/src/CypressTest.tsx +++ b/packages/test-cypress/src/CypressTest.tsx @@ -110,21 +110,25 @@ export function CypressTest() { const [includeVariantSelector, setIncludeVariantSelector] = useState( testSettings.includeVariantSelector, ); - const [isAccessible, setIsAccessible] = useState(null); - const isAccessibleRef = useRef(null); - isAccessibleRef.current = isAccessible; + const diagnosticsSummaryRef = useRef | null>(null); useEffect(() => { - (window as any).returnIsAccessibleCallbackValue = (): - | boolean - | null => { - return isAccessibleRef.current; + (window as any).returnDiagnosticsSummaryCallbackValue = (): Record< + string, + number + > | null => { + return diagnosticsSummaryRef.current; }; return () => { - delete (window as any).returnIsAccessibleCallbackValue; + delete (window as any).returnDiagnosticsSummaryCallbackValue; }; }, []); + // Reset diagnosticsSummaryRef when new DoenetML is posted to avoid stale values + useEffect(() => { + diagnosticsSummaryRef.current = null; + }, [doenetMLstring]); + const solutionDisplayMode = "button"; // requestedVariantIndex is undefined by default so that viewer @@ -590,8 +594,10 @@ export function CypressTest() { showAnswerResponseButton={answerResponseCounts !== undefined} answerResponseCounts={answerResponseCounts} readOnly={readOnly} - isAccessibleCallback={(nextIsAccessible: boolean) => { - setIsAccessible(nextIsAccessible); + diagnosticsSummaryCallback={( + nextDiagnosticsSummary: Record, + ) => { + diagnosticsSummaryRef.current = nextDiagnosticsSummary; }} /> );