Repository navigation
Conversation
- Decouple container into JcaTooltipBox and anchor into TooltipIconButton - Add TooltipTriggerEvent and triggerFlow for programmatic control - Add autoDismissDuration with triggerRevision reset support - Expose positioning parameter (TooltipAnchorPosition) - Add comprehensive Robolectric Compose unit tests in TooltipTest
…ng and tests - Add containerColor and contentColor parameters to TooltipIconButton and pass to IconButtonDefaults colors. - Add initialStyle to OverlapAwareStyleProvider to prevent initial frame visual flicker before bounds measurement. - Add topEndContent slot in PreviewLayout aligned to CenterEnd of the top status bar row with OverlapAwareStyleProvider and animated container color. - Wire topEndContent in PreviewScreen with TooltipIconButton positioned below the status bar. - Add unit tests for TooltipIconButton custom container colors and OverlapAwareStyleProvider initialStyle behavior.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a reusable tooltip component (JcaTooltipBox and TooltipIconButton) along with corresponding unit tests, and integrates a placeholder tooltip into the preview screen. It also updates OverlapAwareStyleProvider to support an initial style to prevent visual flicker, and configures unit testing dependencies for the ui:components module. Feedback focuses on improving accessibility by adding a contentDescription to TooltipIconButton and changing the tooltip's live region mode from Assertive to Polite. Additionally, it is recommended to extract the hardcoded placeholder string to strings.xml and group literal dimensions in Tooltip.kt into a Tokens object to avoid magic numbers.
…specs - Use IconButtonDefaults.extraSmallContainerSize() for default TooltipIconButton container size (32.dp). - Use IconButtonDefaults.extraSmallIconSize for placeholder icon size (20.dp). - Add 24.dp end padding to status bar button placement in CaptureLayout to achieve 40.dp total clearance from screen edge. - Add unit test in TooltipTest verifying custom size application.
- Replace upstream RichTooltip with Compose Popup, JcaTooltipPositionProvider, and JcaTooltipBubbleShape for synchronous Frame 0 caret calculation. - Add continuous vector path uniting rounded bubble and triangular caret with corner-radius boundary clamping. - Support CaretAlignment options: AnchorCenter, BubbleCenter, Start, End, and Custom offset. - Move TooltipIconButton into top bar indicatorRow preceding flash mode indicator and remove topEndContent from CaptureLayout. - Add KDoc explaining upstream Material 3 RichTooltip limitations. - Add unit tests for position provider clamping, vertical flipping, caret alignment modes, and shape outline bounds.
…sibility standards - Enforce 48.dp minimum interactive touch target on TooltipIconButton while maintaining 32.dp visual container and 24.dp icon dimensions. - Center 32.dp visual circular container inside 48.dp touch target, moving the anchor center from 32.dp to 40.dp and eliminating caret clamping offset. - Add TalkBack accessibility support to TooltipIconButton: Role.Button, contentDescription, onClickLabel, and ripple indication. - Update JcaTooltipBubble text typography to labelLarge (14.sp, weight 500, proportional line height) and content padding to 16.dp. - Add vertical scrolling to JcaTooltipBubble content column and screen margin clamping to popupY to prevent overflow under large font scaling. - Update JcaTooltipDefaults: CornerRadius to 16.dp and DefaultElevation to 2.dp. - Add unit tests verifying 48.dp touch target size and TalkBack semantics.
- Map CaretSize to TooltipDefaults.caretSize (16.dp x 8.dp) with convenience CaretWidth and CaretHeight delegates. - Map ContainerSize to IconButtonDefaults.extraSmallContainerSize().width (32.dp). - Map IconSize to IconButtonDefaults.mediumIconSize (24.dp). - Map TouchTargetSize to standard Material 3 48.dp minimum interactive size. - Add standardM3Colors() to JcaTooltipDefaults based on TooltipDefaults.richTooltipColors(). - Reference JcaTooltipDefaults.IconSize in PreviewScreen instead of hardcoded dp. - Reformat all long lines to adhere to the spotless 100-character line-length policy. - Add unit test verifying token mappings and ensure all suite tests pass.
- Encapsulate TooltipIconButton and IndicatorPill inside a centered Row in PreviewScreen's indicatorRow with 8dp horizontal spacing. - Pass incoming indicatorRow modifier to the outer Row container. - Guard the entire indicatorRow layout with isErrorDialogShowing. - Remove redundant Text import and add missing Compose UI imports.
Decouple triggers and auto-dismissal timers from the UI composable components (`JcaTooltipBox` and `TooltipIconButton`) into a dedicated `JcaTooltipState` extending Compose Material 3's `TooltipState`. Key changes: - Introduce `JcaTooltipState` interface extending `TooltipState` with `toggle()` and `autoDismissDuration` support. - Implement `JcaTooltipStateImpl` utilizing `MutatorMutex` and `withTimeoutOrNull` to coordinate auto-dismissal and naturally reset timers upon subsequent `show()` triggers without custom revision counters. - Provide `rememberJcaTooltipState()` factory function with configurable `autoDismissDuration`, `initialIsVisible`, `isPersistent`, and optional shared `MutatorMutex`. - Remove `triggerFlow`, `autoDismissDuration`, `triggerRevision`, and `sealed interface TooltipTriggerEvent` from `Tooltip.kt`. - Update `TooltipTest.kt` to cover `JcaTooltipState` methods (`toggle()`, programmatic `show()` and `dismiss()`, auto-dismiss timing, and timer reset on re-trigger).
Update the tooltip bubble semantics liveRegion mode from Assertive to Polite so screen readers do not abruptly cut off ongoing speech when the tooltip is displayed, and add corresponding test coverage.
temcguir
requested changes
Oct 31, 2026
temcguir
left a comment
temcguir
left a comment
Collaborator
There was a problem hiding this comment.
Thanks for digging into this! You're right that M3's tooltip has a couple of real problems right now: the caret drifts off the anchor near screen edges, and it shows up a few frames late. But I think we can work around just those two things and still use TooltipBox + RichTooltip for everything else. I tried that locally, plugging your position provider and bubble shape into M3, and it looked good on device. Details are in the inline comments.
…opBarLeadingContent slot - Replace custom JcaTooltipBox, JcaTooltipState, JcaTooltipBubble, and gesture wrappers with Material 3 TooltipBox, RichTooltip, and rememberTooltipState, retaining only JcaTooltipPositionProvider and JcaTooltipBubbleShape for edge-aware synchronous caret placement. - Clamp localCaretX in JcaTooltipPositionProvider and simplify JcaTooltipBubbleShape using RoundedCornerShape + PathOperation.Union. - Add TooltipScope.() -> Unit overload to TooltipIconButton alongside the String convenience overload, track pointer-down visibility to fix anchor tap-to-close on device, and clear duplicate TalkBack onClickLabel speech. - Move top-bar leading button support into an optional cutout-aware topBarLeadingContent slot on PreviewLayout with a Compose preview, reverting placeholder PreviewScreen and OverlapAwareStyleProvider changes. - Streamline TooltipTest, add TooltipInstrumentedTest for real window-level tap toggling, and add CaptureLayoutInsetsTest coverage for topBarLeadingContent.
# Conflicts: # ui/components/capture/src/main/java/com/google/jetpackcamera/ui/components/capture/CaptureLayout.kt
temcguir
requested changes
Nov 6, 2026
temcguir
left a comment
temcguir
left a comment
Collaborator
There was a problem hiding this comment.
Thanks, this looks much better! Just a couple of small things left inline.
Reconcile topBarLeadingContent with CaptureScaffoldLayout, CaptureScaffold, and OverlapAwareStyleProvider (#637), remove @SuppressLint on TopBarLeadingAndIndicatorsRow, and drop unbounded wrapContentWidth.
- Add @stable JcaTooltipCaret and rememberJcaTooltipCaret(positioning) encapsulating CaretPositionState privately and keying state on positioning. - Defer horizontal caret X evaluation via localCaretXProvider in JcaTooltipBubbleShape.createOutline so Frame 0 renders at the exact anchor offset without an extra recomposition. - Require explicit contentDescription on the TooltipScope.() -> Unit TooltipIconButton overload and use contentColor for the outlined button border. - Guard onDismissRequest invocations with !state.transition.targetState so tapping the anchor while open fires onDismissRequest exactly once. - Remove unused unit test dependencies from :ui:components and expand unit/instrumented test coverage.
- Restrict JcaTooltipBubbleShape and JcaTooltipPositionProvider to internal classes with private constructor properties. - Add behavioral descriptions and @PARAM KDocs to both TooltipIconButton overloads and JcaTooltipPositionProvider. - Update TOOLTIP_ANCHOR_BUTTON_TAG string value to btn_tooltip_anchor per the element_purpose_value test tag schema. - Extract shared clampCaretX helper accounting for bodyHeight / 2f consistently and simplify TooltipBox onDismissRequest wiring. - Import Rect directly in TooltipTest and add class-level KDocs to TooltipTest and TooltipInstrumentedTest.
temcguir
requested changes
Nov 10, 2026
…prefix - Slot TooltipIconButton overload now takes caret: TooltipCaret instead of a PopupPositionProvider, so the bubble shape, caret padding, and popup placement always come from the same instance. - Rename JcaTooltipCaret/rememberJcaTooltipCaret to TooltipCaret/ rememberTooltipCaret, JcaTooltipDefaults to TooltipIconButtonDefaults, and internal JcaTooltipBubbleShape/JcaTooltipPositionProvider to TooltipBubbleShape/TooltipPositionProvider. - Align tooltip test tag values to PascalCase (TooltipAnchorButton, TooltipContent) to match the capture module. - Center the instrumented test anchor within safeDrawingPadding so injected taps never land on System UI bars.
temcguir
approved these changes
Nov 14, 2026
| contentDescription: String?, | ||
| modifier: Modifier = Modifier, | ||
| state: TooltipState = rememberTooltipState(isPersistent = true), | ||
| caret: TooltipCaret = rememberTooltipCaret(), |
Collaborator
There was a problem hiding this comment.
Thanks for this! One more thing, and it's on my earlier suggestion: since caret still has a default, it's possible to hoist val caret = rememberTooltipCaret(), use its shape and padding, and then forget caret = caret. That gives the same ~100dp-off caret as before. Dropping the default so caret is required would close that gap. It's a small change, so feel free to fold it in before merging or do it as a quick follow-up. Approving so this isn't blocked.
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.
Summary
Adds a Material 3-based
TooltipIconButtoncomponent in:ui:componentsand an optionaltopBarLeadingContentslot inCaptureScaffold/CaptureScaffoldLayoutadjacent to the top status indicator pill.Key Changes
Tooltip.kt(:ui:components):TooltipIconButton(StringandTooltipScope.() -> Unitoverloads) built on M3TooltipBoxandRichTooltip, tracking pointer-down visibility so tapping the anchor cleanly closes an open tooltip and firesonDismissRequestonce.JcaTooltipCaret/rememberJcaTooltipCaret(positioning)pairing16.dpedge-clamped popup positioning and vertical flipping with a draw-phase (createOutline) caret bubble shape.CaptureLayout.kt,CaptureScaffold.kt(:ui:components:capture):topBarLeadingContentbeforeindicatorRowinsideCutoutAwareRowandOverlapAwareStyleProvider.Testing
TooltipTest,CaptureLayoutInsetsTest,CaptureScaffoldTest) and window-level touch toggle test (TooltipInstrumentedTest).