Overlay: SpeedDialAction has no right offset when a vertical scrollbar appears after initial render (T1334663) - #35062
Open
EugeniyKiyashko wants to merge 13 commits into
Open
Conversation
…s not reported yet Chrome reports window.outerWidth as 0 until the first frame is painted, which is when widgets are created on document ready. innerWidth > outerWidth then holds by mistake and the window is measured with the classic scrollbar included, so an overlay anchored to the right or bottom edge lands under the scrollbar.
The window resize event is not raised when only a scrollbar appears or disappears, so an overlay created while the page still fits the screen kept its old coordinate once content arrived and the scrollbar took its 15px. Overlays anchored to the window now follow documentElement.clientWidth/clientHeight through a shared ResizeObserver.
documentElement.clientWidth/clientHeight exclude the classic scrollbar, window.innerWidth/innerHeight include it. Reading the window through innerWidth whenever it exceeded outerWidth put every overlay anchored to the right or bottom edge under the scrollbar: on a zoomed out page, where innerWidth is the wider of the two, and before the first frame, where Chrome still reports outerWidth as 0. The phone branch above keeps using the visual viewport, and Safari keeps innerHeight.
…t when subscribing The subscription was only re-evaluated on show and hide, so changing visualContainer or position on a visible overlay left it stale: an overlay that became window-anchored never started following the viewport, and one that stopped being window-anchored kept re-rendering its geometry. The handler now checks the container itself, which also keeps the dispose path from reaching into the position controller.
The module holds its subscribers in module scope, so a case that failed before its own remove() leaked the handler into the next one and turned one defect into a cascade of failures.
documentElement.clientHeight is what desktop Safari should be measured by: it matches innerHeight in every configuration except a horizontal scrollbar, where innerHeight is 15px too large and puts a bottom-anchored overlay on the scrollbar. Measured in Safari 26.4 - bottom edge 364 against an expected 349. The fallback stays for iOS, where the client height does not match the visible area while the address bar slides; iPadOS in desktop mode is detected through maxTouchPoints, so it keeps the fallback too. browser.safari is now read where it is used rather than captured at module load, which also makes the branch reachable from tests.
Only the width comparison was guarded, so the height one was safe purely because && short-circuits. Reordering the operands or splitting the condition would have dereferenced a null previousSize.
The module kept its own Set with a hand-rolled copy for re-entrancy, which is the shape m_resize_callbacks already gets from Callbacks(). The list is created with unique: true, since Callbacks otherwise stores duplicates and removes only the first of them, which would leave a handler behind. The local HandlerList type stands in until the shared declaration describes has() without arguments.
The names carry what they said, and the reasoning belongs in the commits.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The newly added QUnit tests in overlay.tests.js create visible Overlay instances without disposing them, which can leak global subscriptions/DOM and cause test flakiness.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR addresses an Overlay positioning edge case where a vertical scrollbar appears after the initial render (notably affecting SpeedDialAction right offset), by re-measuring visible document/window dimensions and re-rendering Overlay geometry when needed.
Changes:
- Added a
documentSizeCallbacksutility (ResizeObserver-based) to notify whendocumentElement.clientWidth/clientHeightchanges. - Updated
Overlayto subscribe/unsubscribe to document size changes while visible and re-render geometry when the visual container is the window. - Updated window sizing logic in positioning and expanded QUnit coverage for window measurement scenarios (including Safari/iOS Safari specifics).
File summaries
| File | Description |
|---|---|
| packages/devextreme/testing/tests/DevExpress.ui.widgets/overlay.tests.js | Adds QUnit coverage for Overlay document-size subscription and geometry re-rendering behavior. |
| packages/devextreme/testing/tests/DevExpress.animation/position.tests.js | Updates/extends position tests to validate window measurement behavior across browsers (incl. Safari variants). |
| packages/devextreme/js/__internal/ui/overlay/overlay.ts | Subscribes Overlay to document size changes while visible and conditionally re-renders geometry. |
| packages/devextreme/js/__internal/core/utils/document_size_callbacks.ts | Introduces ResizeObserver-backed callback registry for document client size changes. |
| packages/devextreme/js/__internal/core/utils/tests/document_size_callbacks.test.ts | Adds Jest unit tests validating observe/unobserve behavior and change detection. |
| packages/devextreme/js/__internal/common/core/animation/m_position.ts | Adjusts window size measurement to use client width/height, with special-casing for iOS Safari height. |
Review details
Suppressed comments (3)
packages/devextreme/testing/tests/DevExpress.ui.widgets/overlay.tests.js:4523
- This test leaves a visible Overlay instance undisposed, so it stays subscribed to documentSizeCallbacks and can impact subsequent tests. Dispose the overlay at the end of the test.
QUnit.test('geometry should not be re-rendered for an overlay placed against an element', function(assert) {
const overlay = new Overlay(this.$element, {
visible: true,
visualContainer: $('#container')
});
packages/devextreme/testing/tests/DevExpress.ui.widgets/overlay.tests.js:4536
- This test doesn't dispose the visible Overlay it creates, which can keep global documentSizeCallbacks subscriptions alive and affect other tests. Dispose the overlay at the end of the test.
QUnit.test('geometry should be re-rendered after visualContainer becomes the window at runtime', function(assert) {
const overlay = new Overlay(this.$element, {
visible: true,
visualContainer: $('#container')
});
packages/devextreme/testing/tests/DevExpress.ui.widgets/overlay.tests.js:4552
- This test leaves the created Overlay instance alive and still subscribed to documentSizeCallbacks. Dispose the overlay at the end to avoid leaking global subscriptions/DOM between tests.
QUnit.test('geometry should not be re-rendered after visualContainer stops being the window at runtime', function(assert) {
const overlay = new Overlay(this.$element, { visible: true });
const handler = this.subscribedHandler();
overlay.option('visualContainer', $('#container'));
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+4509
to
+4517
| QUnit.test('geometry should be re-rendered when the visible area changes', function(assert) { | ||
| const overlay = new Overlay(this.$element, { visible: true }); | ||
| const handler = this.subscribedHandler(); | ||
| const renderGeometrySpy = sinon.spy(overlay, '_renderGeometry'); | ||
|
|
||
| handler(); | ||
|
|
||
| assert.strictEqual(renderGeometrySpy.callCount, 1, 'geometry is re-rendered'); | ||
| }); |
The old innerHeight > outerHeight check was true on iOS whenever the browser toolbar was collapsed: WebKit reports the unobscured area as innerHeight and the main frame view as outerHeight, in Safari, Chrome, Firefox and WKWebViews alike. Keying the fallback on browser.safari dropped it for iPad Chrome, Firefox and in-app web views, whose user agents carry no Version/ token, so the platform alone decides now. The desktop Safari skip in the client height test is gone too: desktop Safari measures the window by its client height like every other desktop browser, and the neighbouring test covers that case.
… element A ResizeObserver on documentElement sits at depth 0, so whenever another observer callback in the same frame changed the html size (a popup content handler moving the popup inside the viewport and taking the scrollbar with it) the browser reported "ResizeObserver loop completed with undelivered notifications" - the DropDownButton right-bottom popup test failed 3 of 3 attempts on it. The window.visualViewport width and height exclude classic scrollbars and its resize event fires when one appears or disappears, in Chrome, Firefox and WebKit, without a window resize. The subscription now listens to that event and reads the document client size as before. A window resize changes both the window and the client size and is already reported through resizeCallbacks, so such events are skipped: subscribers would otherwise re-render twice per resize. Only the client size is read from domAdapter.getDocumentElement() now, which also frees the dialog and AIDialog tests that fake it with a plain object - the native observe() rejected that object and the suites hung until the runner timeout.
Only window-anchored overlays followed the document size. An overlay placed against an element moves as well when a scrollbar appears and the page reflows, and an overlay with container: body keeps position.of on the window while its visual container is the body. The handler now goes through _dimensionChanged, the same path a window resize takes, so every visible overlay re-renders and subclasses such as ContextMenu keep their own handling. The test module disposes its overlays so their handlers do not outlive the test in the module-scoped callback list.
The unit tests call the subscription handler by hand; this one lets the browser do it. The FAB is rendered on an empty page, then a 3000px element is appended and both scrollbars appear, and the button is expected to keep its 16px distance from the right and bottom edges of the client area. Linux Chrome on CI draws classic scrollbars, so the client area shrinks there.
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The functional changes are well-covered by targeted unit/integration tests, with only a minor performance nit suggested in positioning hot-path code.
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Lite
Comment on lines
259
to
+266
| if (devices.real().deviceType === 'phone' && of[0].visualViewport) { | ||
| h.atLocation = Math.max(h.atLocation, of[0].visualViewport.offsetLeft); | ||
| v.atLocation = Math.max(v.atLocation, of[0].visualViewport.offsetTop); | ||
| h.atSize = of[0].visualViewport.width; | ||
| v.atSize = of[0].visualViewport.height; | ||
| } else { | ||
| h.atSize = of[0].innerWidth > of[0].outerWidth ? of[0].innerWidth : getWidth(of); | ||
| v.atSize = of[0].innerHeight > of[0].outerHeight || IS_SAFARI ? of[0].innerHeight : getHeight(of); | ||
| const isIos = devices.real().platform === 'ios'; | ||
|
|
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.
No description provided.