feat: PromiseAction + CopyTextToClipboardAction + RequestFullscreenAction#24394
Open
Artur- wants to merge 6 commits into
Open
feat: PromiseAction + CopyTextToClipboardAction + RequestFullscreenAction#24394Artur- wants to merge 6 commits into
Artur- wants to merge 6 commits into
Conversation
…tion
Adds a `PromiseAction` base class in the trigger framework and two
concrete subclasses — `CopyTextToClipboardAction` and
`RequestFullscreenAction` — on top of the executeJs-based trigger
framework introduced in step 1.
`PromiseAction` is the generic primitive: many gesture-bound browser
APIs are asynchronous (clipboard, fullscreen, file picker, share,
payments, …) and follow the same shape — call the API, then handle the
resolved value or the rejection. Subclasses override
`appendPromiseExpression(JsBuilder, StringBuilder)` to emit the
promise-yielding JS; the base wires the outcome reporting.
Two construction modes, mirroring the Geolocation API:
- No-arg `super()` — fire-and-forget; the rendered JS is just the
promise expression and the server never sees the outcome.
- `super(onSuccess, onError)` — `onSuccess` runs after the promise
resolves; `onError` receives the browser's error message after the
promise rejects. Both run on the UI thread; both are required (pass
`() -> {}` or `err -> {}` to opt out of one, matching the
Geolocation pattern).
The with-outcome mode lazily registers one
`ReturnChannelRegistration` per trigger host node and appends
`.then(()=>$N(true,null)).catch(e=>$N(false, msg))` to the promise
expression, so the client invokes the channel after the promise
resolves or rejects. `JsBuilder.capture(Object)` is the new
package-private hook that lets actions allocate a `$N` placeholder for
non-Element captures (the return channel here, which materialises
client-side as a callable JS function).
Concrete subclasses:
- `CopyTextToClipboardAction` — supplies
`navigator.clipboard.writeText(textExpr)` as the promise expression.
Takes an `Action.Input<String>` for the text. Named verb-first so
parallels like `CopyImageToClipboardAction` and
`PasteFromClipboardAction` read naturally.
- `RequestFullscreenAction` — supplies `<target>.requestFullscreen()`
as the promise expression. Takes a target `Component` whose root
element will be fullscreened.
Both ITs (`TriggerCopyTextToClipboardView`/`IT`,
`TriggerRequestFullscreenView`/`IT`) shim the underlying browser API
to resolving/rejecting promises and assert the server-side status div
updates accordingly — avoiding dependence on browser clipboard /
fullscreen permissions in CI.
Artur-
force-pushed
the
feature/trigger-step2
branch
2 times, most recently
from
1dc9334 to
db61e7a
Compare
`PromiseAction.appendStatement` used to assemble the `.then`/`.catch`
glue by chained `StringBuilder.append` calls, with the channel
receiving positional `(boolean, message)` args decoded by index in
`dispatch`. Two smells in one place: structural JS hidden in string
concatenation, and an implicit positional wire contract.
Replace both:
- A static `JsFunction OBSERVE_PROMISE` carries the wrapper body once,
with named runtime arguments `(promise, channel)`. Subclasses are
unchanged; `appendStatement` now allocates two captures (observer +
channel), then composes one call: `$observe($promiseExpr, $channel)`.
No more `.append(".then(()=>")…` chains; the handler body is one
line, the JS literal lives in one place.
- Introduce a private `record Outcome(boolean ok, @nullable String
error)` as the wire shape. The OBSERVE_PROMISE body pushes
`{ok: true}` / `{ok: false, error: …}`; `dispatch` reads it via
`JacksonUtils.readValue` rather than positional `args.get(0)` /
`args.get(1)` decoding. Renaming or adding a field can no longer
silently desynchronize the two sides.
Test assertions follow: the handler body is now `$0(<promise>, $1);`
across `PromiseActionTest`, `CopyTextToClipboardActionTest`, and
`RequestFullscreenActionTest`; the channel-invocation tests pass an
`Outcome`-shaped `ObjectNode` instead of positional booleans/strings.
`OBSERVE_PROMISE` is a `static final` shared instance — `JsFunction`
is `Serializable` and immutable, and each `addJsInitializer`
registration still gets its own capture entry, so there's no
cross-UI state and no leak. 19/19 trigger-internal tests pass.
`LiteralInput` was package-private and accepted `@Nullable T`. Both
were carry-overs from its only original user, `SetPropertyAction`'s
null-clearing convenience constructor, but neither survives as
LiteralInput becomes a building block for other actions:
- Public so callers can wire a fixed value through any action that
takes an `Action.Input<? extends T>` — e.g. copying a constant
string with `new CopyTextToClipboardAction(new LiteralInput<>("…"))`.
- Non-null because `null` as a literal payload almost never matches
a sensible browser API. `writeText(null)` writes the string
`"null"` to the clipboard; `target.requestFullscreen.call(null)`
is meaningless. Forbidding it at the `LiteralInput` constructor
catches the foot-gun at compile time (via NullAway) wherever the
type permits, and at runtime via `Objects.requireNonNull`
otherwise.
`SetPropertyAction`'s null-accepting convenience constructor stays —
clearing a property by assigning `null` is a real use case. It now
routes the null branch through a private `NULL_LITERAL` singleton
that emits the JS literal `null`, instead of leaking nullability
into `LiteralInput`.
Adds a `fireAndForget_literalInput_encodesValueAsJsonLiteral` test
in `CopyTextToClipboardActionTest` that copies `hello "world"\n` and
asserts the quotes and newline are JSON-escaped — proves callers
can't break out of the JS string by passing values with quotes or
newlines.
The view now has two buttons — `#copy` ("Copy input value") wired to a
PropertyInput on the input field, and `#copy-static` ("Copy static
text") wired to a LiteralInput carrying `hello "world"\n`. The literal
deliberately includes a quote and a newline so the IT verifies the
value the browser ends up with is exactly what Java passed in, i.e.
the JSON encoding done at build time round-trips without losing or
mangling characters.
Button labels updated to describe what each button copies.
|
mshabarov
reviewed
May 21, 2026
Contributor
mshabarov
left a comment
mshabarov
left a comment
There was a problem hiding this comment.
API and implementation looks good to me overall - clear purposes for added classes, nice classes hierarchy. I left several comments with some of them not being 100% changes requests, so ready to merge even as is with a meaningful elaborations of why it's implemented this way.
Comment on lines
+46
to
+47
| * Two construction modes, mirroring the | ||
| * {@link com.vaadin.flow.component.geolocation.Geolocation Geolocation} API: |
Contributor
There was a problem hiding this comment.
Referring to geolocation API here is probably redundant. I assume it added by Claude because it had a geolocation knowledge in a context.
| .withArguments("promise", "channel"); | ||
|
|
||
| private final @Nullable SerializableRunnable onSuccess; | ||
| private final @Nullable SerializableConsumer<String> onError; |
Contributor
There was a problem hiding this comment.
The error handler can give also an error name as an argument, so it can be possible to discriminate various types of errors. Also a value, but not 100% if anyone would need it.
| error: e && e.message ? e.message : String(e)}));""") | ||
| .withArguments("promise", "channel"); | ||
|
|
||
| private final @Nullable SerializableRunnable onSuccess; |
Contributor
There was a problem hiding this comment.
Just an out loud thinking: is it ever a case when a copied value is needed in the onSuccess (or even in orError callback)? I think it's usually not, nothing wants to show a copied value, but the fact that it's copied.
|
|
||
| @Override | ||
| protected final void appendStatement(JsBuilder builder, StringBuilder out) { | ||
| if (onSuccess == null) { |
Contributor
There was a problem hiding this comment.
Should this also check onError callback availability? I guess this is for fire-and-forget case, so I'd expect both callbacks to absent. Or just only the success?
Comment on lines
+35
to
+39
| * Note that fullscreening an arbitrary component element does not interact well | ||
| * with Vaadin theming or overlay components — those expect the fullscreen | ||
| * element to be {@code document.documentElement}. This action is intentionally | ||
| * low-level; the higher-level {@code Component.requestFullscreen()} facade | ||
| * handles the wrapping needed for full Vaadin compatibility. |
Contributor
There was a problem hiding this comment.
What does this mean, are we just repeating by this that this class is purely internal and Component will provide a proper API?
| StringBuilder out); | ||
|
|
||
| @Override | ||
| protected final void appendStatement(JsBuilder builder, StringBuilder out) { |
Contributor
There was a problem hiding this comment.
Not sure, maybe it's fine for internal class, but are we intentionally want to keep this away from overrides?
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
2 participants
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.
Adds a
PromiseActionbase class in the trigger framework and two concrete subclasses —CopyTextToClipboardActionandRequestFullscreenAction— on top of the executeJs-based trigger framework introduced in step 1.PromiseActionis the generic primitive: many gesture-bound browser APIs are asynchronous (clipboard, fullscreen, file picker, share, payments, …) and follow the same shape — call the API, then handle the resolved value or the rejection. Subclasses overrideappendPromiseExpression(JsBuilder, StringBuilder)to emit the promise-yielding JS; the base wires the outcome reporting.Two construction modes, mirroring the Geolocation API:
super()— fire-and-forget; the rendered JS is just the promise expression and the server never sees the outcome.super(onSuccess, onError)—onSuccessruns after the promise resolves;onErrorreceives the browser's error message after the promise rejects. Both run on the UI thread; both are required (pass() -> {}orerr -> {}to opt out of one, matching the Geolocation pattern).The with-outcome mode lazily registers one
ReturnChannelRegistrationper trigger host node and appends.then(()=>$N(true,null)).catch(e=>$N(false, msg))to the promise expression, so the client invokes the channel after the promise resolves or rejects.JsBuilder.capture(Object)is the new package-private hook that lets actions allocate a$Nplaceholder for non-Element captures (the return channel here, which materialises client-side as a callable JS function).Concrete subclasses:
CopyTextToClipboardAction— suppliesnavigator.clipboard.writeText(textExpr)as the promise expression. Takes anAction.Input<String>for the text. Named verb-first so parallels likeCopyImageToClipboardActionandPasteFromClipboardActionread naturally.RequestFullscreenAction— supplies<target>.requestFullscreen()as the promise expression. Takes a targetComponentwhose root element will be fullscreened.Both ITs (
TriggerCopyTextToClipboardView/IT,TriggerRequestFullscreenView/IT) shim the underlying browser API to resolving/rejecting promises and assert the server-side status div updates accordingly — avoiding dependence on browser clipboard / fullscreen permissions in CI.