Browser paused state updates #289247
Browser paused state updates #289247
Conversation
There was a problem hiding this comment.
Pull request overview
This PR enhances the browser view feature by adding visual feedback when browser interactions are paused due to overlapping UI overlays. The implementation categorizes different overlay types (menu, quick input, hover, dialog, notification) and displays contextual pause messages to users.
Changes:
- Refactored overlay detection to categorize overlay types and return detailed information about overlapping overlays
- Added batching mechanism using microtasks to optimize multiple mutation observer callbacks
- Introduced visual pause overlay with optional message display for notifications
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| overlayManager.ts | Added BrowserOverlayType enum and IBrowserOverlayInfo interface; refactored overlay tracking to include type information; added microtask-based batching for overlay state changes; changed API from boolean check to returning array of overlapping overlays |
| browser.css | Removed old blur-based placeholder styling; added new pause overlay with semi-transparent background, message container with heading and detail sections, fade-in/out transitions, and conditional message display |
| browserEditor.ts | Added DOM elements for pause overlay UI; updated visibility logic to differentiate between paused and viewing states; implemented overlay-specific message display (currently only for notifications); integrated with refactored overlay manager API |
| this._overlayPauseContainer.classList.toggle('show-message', hasNotificationOverlay); | ||
|
|
||
| if (hasNotificationOverlay) { | ||
| this._overlayPauseHeading.textContent = localize('browser.overlayPauseHeading.notification', "Paused due to notification"); |
There was a problem hiding this comment.
The UI label "Paused due to notification" does not follow title-style capitalization. According to the coding guidelines, UI labels for buttons, menu items, and command labels should use title-style capitalization where each word is capitalized (except prepositions of four or fewer letters unless it's the first or last word). This should be "Paused Due to Notification".
| // Placeholder screenshot: shown when there is a page loaded (even when the view is not hidden, so hiding is smooth) | ||
| this._placeholderScreenshot.style.display = isViewingPage ? '' : 'none'; | ||
|
|
||
| // Pause overlay: fades in when an overlay is detected |
There was a problem hiding this comment.
nit: It might be slightly confusing if we overload the term "overlay" to mean the both the overlays themselves as well as the message we show when there are overlays
| } | ||
|
|
||
| private updateOverlayPauseMessage(overlappingOverlays: readonly IBrowserOverlayInfo[]): void { | ||
| // Only show the pause message for notification overlays |
There was a problem hiding this comment.
I'm glad we're showing that, since I agree that it can be confusing when it's paused because of a toast or notification and the user might not know otherwise how to unpause it. Especially for users who just leave the notifications undismissed for long periods
| return; | ||
| } | ||
| this._overlayStateChangePending = true; | ||
| queueMicrotask(() => { |
There was a problem hiding this comment.
From what I understand, mutationObserver callbacks are also microtasks, so I'm guessing the idea is that with this approach, the idea is to batch all the mutationObserver callbacks that are currently in the microtask queue? I think there's no guarantee that there won't be more than one call before a given rendering, in case more mutationObserver microtasks are added to the microtask queue after this is added there
Part of #286579