Repository navigation
DEV-3554: Fix the hyperformula-ui issues found in the Handsontable formula builder review - #1839
Open
adrianspdev wants to merge 34 commits into
Open
adrianspdev wants to merge 34 commits into
adrianspdev wants to merge 34 commits into
Conversation
…e editor The idle formula bar tokenized without resolving named expressions, so a named expression did not take a reference color slot there while it did in the cell editor, and every later reference was shown in a different color. The idle view now runs the same named-expression annotation as the editor and re-renders when a named expression is added or removed.
Unsubscribing from a destroyed HyperFormula instance throws, and the first throw in FormulaBuilder.destroy() skipped the rest of the teardown, leaving the popup portal, the live region and the injected stylesheet in the page. The named-expression unsubscribe now tolerates a destroyed engine, and every teardown step runs even when one throws; the first error is rethrown at the end.
The quoted sheet-name pattern was retried at every position of a run of apostrophes that no '! closes, so a long run took quadratic time on every tokenize (about 600 ms for 40,000 characters). The closing quote of every possible quoted sheet name is now found in one right-to-left pass per tokenize, with the same matches as the pattern it replaces.
The error value in a cell used the monospace formula font while the error popup title used the UI font, so the same #NAME? appeared in two fonts side by side. Both now read the new --hfe-error-font-family custom property, which defaults to --hfe-font-family, the host's font.
The stylesheet set color-scheme: light dark on every package element through a class-attribute substring selector, so light-dark() followed the operating system instead of the host theme: a dark grid on a light system showed light popups, a host could undo it only element by element, and the selector was tested against every element of the page. It is now set once per root (the editor, the formula bar, the popup portal and a hosted popup), with no specificity, so a host theme class on a root overrides it and the rest inherits.
A cell's error message was associated with the cell only while the error popup was open, through aria-errormessage, which some screen readers ignore, so a screen-reader user landing on the cell heard nothing. markCell() now also sets the message as the cell's aria-description, and removes it again when the error clears.
The address box checked only the grid bounds and reported success even when the host could not select the cell, for example a filtered-out row or a hidden column, so it showed the typed address while the selection stayed put. IGridAdapter.setActiveCell() and setSelection() may now return false; the address box then reverts, announces the invalid address and does not scroll. Adapters that return nothing keep working as before.
A popups.*.container or formulaBarContainer selector that matched nothing made the FormulaBuilder constructor throw. A host that rebuilds the builder from an event handler may pass a container its framework has just unmounted, and the throw left it without a builder. The constructor now logs a warning once per selector and uses the default placement: a floating popup, or no formula bar. The attach methods still throw on a missing container.
Ctrl/Cmd+Enter was bound to inserting a newline, and the adapter had no way to commit to the whole selection, so a spreadsheet host could not offer the usual fill-selection shortcut. Ctrl/Cmd+Enter now calls the new optional IGridAdapter.commitFill(value), or the onCommitFill option, when the host provides one, and inserts a newline otherwise. Alt+Enter always inserts a newline.
Most default reference colors fell below 4.5:1 against the editor background in light or dark mode (orange and green were near 2:1 on white), so reference text and highlight borders failed WCAG 1.4.3 and 1.4.11 unless the host restated the whole palette. The colors that failed are darkened in light mode or lightened in dark mode, keeping their hue, and the README documents --hfe-ref-1 to --hfe-ref-8 as the way to override them.
The popup portal, the live region and the stylesheet always went into the global document's body and head, and focus checks read the global document.activeElement. Inside a shadow root the editor's ARIA references pointed across the boundary and resolved to nothing; in an iframe the popups landed in the parent page without its styles; and focus checks were wrong in both. FormulaBuilder now takes rootDocument and portalParent options, creates its elements and resolves selectors in that document, appends the portal and live region to the portal parent, injects the styles into a shadow root when the portal parent is in one, and reads focus through open shadow roots.
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
hyperformula-docs | 96be8a3 | Commit Preview URL Branch Preview URL |
Oct 23 2026, 12:22 PM |
Performance comparison of head (96be8a3) vs base (d143456) |
Haxikowy
reviewed
Oct 22, 2026
Haxikowy
left a comment
Haxikowy
left a comment
Contributor
There was a problem hiding this comment.
Findings with no line in the diff:
adapter/domRangePicker.ts:159-162,386bindsmousemove/mouseup/scrollto the globaldocumentand callsdocument.elementFromPoint, so drag-to-extend a range ref does not work in a shadow root (the hit is the retargeted host) or an iframe. Either use the root's document there or list it next to the window listeners under out of scope.- DEFINITION-OF-DONE.md:78 asks for one change per pull request, and :84 for no new public option on its own; this one carries 11 fixes and adds
rootDocument,portalParent,onCommitFill,commitFilland--hfe-error-font-family. Please split it along the existing commits, or record the maintainers' agreement in the description. - DEFINITION-OF-DONE.md:105-111 counts a changed default and removed documented behavior as breaking: the ref colors, the error cell font, the
color-schemescope and the constructor no longer throwing. Tick the box or say why not, and give CHANGELOG.md:17 the "what to do instead" line. - CHANGELOG has no Added entry for
FormulaBarCell.refresh(),tokenPostProcess,StyleRoot,inject(root)/release(root)orLiveRegiontaking aShadowRoot. SPEC.md's types list lacksStyleRoot, and KEYBOARD-SHORTCUTS.md's behavior table has no Alt+Enter row. - The three out-of-scope items need follow-up task IDs (DEFINITION-OF-DONE.md:86), and the red FOSSA check needs a line in the description (:39); it is unrelated, and the same failure is on #1835-#1838.
sequba
approved these changes
Oct 22, 2026
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 79f8031. Configure here.
Author
|
On the findings in the review body:
|
Haxikowy
reviewed
Oct 22, 2026
Haxikowy
approved these changes
Oct 23, 2026
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #1839 +/- ##
========================================
Coverage 97.28% 97.28%
========================================
Files 207 207
Lines 16218 16218
Branches 3556 3588 +32
========================================
Hits 15777 15777
Misses 433 433
Partials 8 8 🚀 New features to boost your workflow:
|
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
The PR fixes the
hyperformula-uiissues found while reviewing the Handsontable formula builder (handsontable/handsontable#13172).hyperformula-ui/.undo(),redo(),changeNamedExpression(),removeSheet()valuesUpdated,sheetRemovedand everysetValuedestroy()throws after the host destroyed the engine, leaking DOM and stylesrootDocumentandportalParentoptions; listeners (scroll in a shadow root included), hit tests and focus checks follow the element's own documentsetActiveCell/setSelectionmay returnfalse; announced as "Cannot select B7", not as invalid'tokenizes in quadratic timesheetIdmakes every keystroke throw--hfe-error-font-family, falling back to--hfe-font-familywhere the error rendersadapter.commitFilloronCommitFill; Alt+Enter inserts a newlinecolor-schemeset on every element through[class*=hfe-]phrasesoption, English defaults inDEFAULT_PHRASESHow did you test your changes?
Every fix has a test that fails without it.
npm run build:uinpm run typecheck(afternpm run bundle-all)npm run lintnpm run test:uinpm run test:e2eformula-builderspecs, axe scanChecked in Chromium on the Handsontable demo against 0.3.5: error-title font,
color-schemewith the OS in the other scheme, Ctrl/Cmd+Enter fill, undo of a named expression, a reference drag in a shadow root, the error popup in an iframe.Compatibility with Excel and Google Sheets
Not applicable. The change is in the formula editor UI and cannot affect calculation results.
Public API changes
All in
hyperformula-ui:FormulaBuilderOptions:rootDocument,portalParent.FormulaEditorOptions(andFormulaBuilderOptions):onCommitFill(value),phrases;FormulaEditor.phrases.IGridAdapter: optionalcommitFill(value);setActiveCellandsetSelectionreturnboolean | void.HyperFormulaLike:on/offforsheetRemoved.Phrases(includescellUnavailable),DEFAULT_PHRASES,StyleRoot,AddressBoxNavigation.FormulaBarCellOptions.tokenPostProcessandphrases,FormulaBarCell.refresh(),FormulaBarAddressBoxOptions.phrasesandonUnavailableTarget; its navigation callbacks may return"unavailable".StyleInjector.inject(root?)/release(root?);LiveRegionaccepts aShadowRoot.--hfe-error-font-family; new--hfe-ref-1..8defaults.Not breaking, though some defaults change (reference colors, the error cell's font, the
color-schemescope, no constructor throw). The package is 0.x and no released consumer depends on these defaults. The changelog says how to keep each old default.Out of scope and follow-ups
None. Keeping all fixes in one PR was agreed by the PR author. The popups staying over frozen panes is recorded as the intended behavior in
dev-docs/SPEC.md.The License Compliance (FOSSA) check also fails on #1838 and #1841; it is not related to this change.
Types of changes
Related issues
Checklist
dev-docs/pages this change affects – or none needed it.Note
Medium Risk
Touches the public
IGridAdaptercontract, destroy/teardown paths, and event/focus behavior in shadow roots and iframes—important for host integrations though calculation logic is unchanged.Overview
hyperformula-uigains host-friendly embedding, i18n, and several Handsontable review fixes in one release-sized change set.Embedding & DOM:
FormulaBuilderaddsrootDocumentandportalParentso popups, live regions, and styles can live in an iframe or shadow root; listeners, hit-testing, and focus use the element’s document instead of the globaldocument. Style injection is ref-counted perStyleRoot(document or shadow root). Invalid or missing constructor container selectors warn once and fall back;attach*methods still throw.Grid adapter & keyboard: Optional
commitFill/onCommitFillmakes Ctrl/Cmd+Enter fill all selected cells; Alt+Enter inserts a newline when fill isn’t supported.setActiveCell/setSelectionmay returnfalse; the address box surfaces"unavailable"with translated announcements via newphrases/DEFAULT_PHRASES.Named expressions & teardown: Named-expression coloring stays in sync between editor and idle formula bar (
tokenPostProcess,refresh()); caches invalidate onvaluesUpdated,sheetRemoved, andsetValue.destroy()runs all teardown steps viarunAlleven if the engine was destroyed first.Other fixes: Lexer uses a linear pass for quoted sheet prefixes (long
'runs). Theming updates reference contrast defaults,--hfe-error-font-family, and removes package-levelcolor-schemeso the host controls light/dark.Reviewed by Cursor Bugbot for commit 96be8a3. Bugbot is set up for automated code reviews on this repo. Configure here.