Uh oh!
There was an error while loading. Please reload this page.
JS port: apply the text layer's DOM changes with the frame they belong to - #5629
Merged
Conversation
…g to The DOM text layer made every frame two halves: the pixels the canvas draws and the elements the layer positions. Only one of them was issued when the frame was decided. promote(), endComponent() and syncToForm() wrote to the document during the component paint, while the canvas commands that paint went on to record are not replayed and shipped until the frame is drained an animation frame later. The browser composites in between, so it renders one half of the new frame over the other half of the old one. Text released early leaves a hole where the glyphs still are on screen; text added early is drawn twice, once in each place. Both are visible on any screen that rebuilds part of itself in response to a tap: the BuildCloud console blanks every label and every icon on the form for a frame when a navigation row is clicked, and shows the outgoing and incoming label of the row superimposed. Nothing about the layer's bookkeeping was wrong, which is why it survived review -- it was applied at the wrong moment. Every mutation is now recorded as an ExecutableOp into the same buffer as the draws around it, replayed into the same SurfaceCommandRecorder, and shipped in the same flush message, so the host applies it in the task that replays that frame's canvas commands -- and in the draw order the paint gave it. There is no rendering opportunity between the two halves for the compositor to take. OP_BLUR_SELF_REGION already rides the stream for the same reason. syncToForm moves from the drain to flushGraphics for the same reason. By the time the drain runs, this frame's ops have been snapshotted, so a release recorded there would ship with the NEXT frame and the removed component's text would sit over the pixels that replaced it for a frame. In flushGraphics the components have finished painting and the buffer is still open, which is exactly the window a release belongs in. Two things fall out of the change. The retained CSSStyleDeclaration handles are gone -- style is written by opcode now -- and with them the two getStyle() property reads per run the class had to cache to stay off the barrier. And building a run's element pair stays an immediate write, because neither element is in the document yet: nothing can be seen half-built, and it guarantees the pair exists by the time the recorded attach is replayed. Measured on the console, real Chrome, 30fps video over four navigations: the worst single-frame brightness jump goes from +0.0159 (the screen blanking) to +0.0057, which is what the same run measures with the layer disabled entirely -- the flicker is gone, not merely smaller. verify-javascript-web-overlay.mjs reports the same 44 promoted runs and 43 semantic nodes as before. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:385a69d8b5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Uh oh!
There was an error while loading. Please reload this page.
CollaboratorAuthor
Compared 163 screenshots: 163 matched. Benchmark ResultsDetailed Performance Metrics
|
CollaboratorAuthor
Compared 163 screenshots: 163 matched. Benchmark ResultsDetailed Performance Metrics
|
Contributor
✅ Continuous Quality ReportTest & Coverage
Static Analysis
Generated automatically by the PR CI workflow. |
CollaboratorAuthor
Compared 163 screenshots: 163 matched. |
CollaboratorAuthor
Compared 163 screenshots: 163 matched. |
Contributor
Cloudflare Preview
|
The drain detects a buffered transition and hides the layer, but it does so after beginFrame() has snapshotted and cleared the queue. Routing that through the sink -- as the previous commit did with every other mutation -- puts it in the buffer the NEXT frame ships, so the first frame of the transition is composited with the outgoing form's DOM text still over it. That is the very artifact the previous commit set out to remove, reintroduced at the one call site that sits on the wrong side of the snapshot. Worse when there is no next frame. A buffered transition paints only its prebuilt images and never puts a component through the display graphics, so if the last flush went out before this ran, a sink-recorded hide would never ship and the stale text would sit over the whole animation. suspendIntoFrame() writes the mutation straight into the recorder for the frame being assembled, so the hide travels in the same flush as the transition's own pixels. The sink stays the right route for callers inside a component paint, whose frame has not been handed off yet. The other two call sites are fine as they are. updateTextLayerSuspension() runs from beforeComponentPaint at frame start, before the hand-off, so the sink puts it in the correct frame. The readback path records through the sink too, but it sets readbackRepaintPending and requests the repaint it then waits for, so the following flush is guaranteed and the capture deliberately waits for it. Found in review of #5629. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CollaboratorAuthor
Compared 163 screenshots: 163 matched. Benchmark ResultsDetailed Performance Metrics
|
CollaboratorAuthor
Compared 181 screenshots: 181 matched. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:0b889f3594
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Uh oh!
There was an error while loading. Please reload this page.
Putting the text layer's mutations on the render queue changed what an empty frame means. A paint that drew no pixels -- a transparent component that only draws text, or a flush whose sole content is syncToForm releasing the runs of a component that has gone -- used to leave the queue empty, and drainPending- DisplayFrame() returned at the isEmpty() check without touching the canvas. Those frames now carry text ops, so they get past that check and reach the full-frame clearRect, which fires on any crop covering the display. The clear runs, the frame has no draw to put anything back, and retained canvas content is erased. The clear exists to stop stale pixels bleeding through a full repaint, so it belongs to frames that repaint. framePaintsPixels() asks whether the frame holds anything other than a text mutation, and the clear is skipped when it does not. Frames that do paint are unaffected; a mixed frame still clears, because it carries the paint that follows the clear. Deliberately not widened to "no op that produces pixels": a clip-only frame reaching the clear predates these ops and is a separate question. This restores exactly what the queue change took away. Found in review of #5629. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CollaboratorAuthor
Compared 148 screenshots: 148 matched. Benchmark Results
Detailed Performance Metrics
|
CollaboratorAuthor
Compared 149 screenshots: 149 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
Contributor
✅ ByteCodeTranslator Quality ReportTest & Coverage
Benchmark Results
Static Analysis
Generated automatically by the PR CI workflow. |
CollaboratorAuthor
Compared 144 screenshots: 144 matched. |
CollaboratorAuthor
Compared 217 screenshots: 217 matched. |
CollaboratorAuthor
Compared 143 screenshots: 143 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
graphics-draw-image-rect failed CI with the top half of its grid drawn and the bottom half blank -- the same fingerprint the settle code already documents by name from the last time it happened. Its capture was taken mid-draw. The settle decides the UI is ready by watching the canvas stop changing: quietFrames with no render-queue advance plus stableFrames with an unchanged signature. Both are absence of evidence. A screen that composes itself in stages is genuinely still between two of them, so it satisfies both while half of it is missing. The existing answer was to widen the window for these two tests, 48/3/3 to 120/6/6, and the failure above is that window being exceeded again. Widening it further only moves the race. Repainting removes it instead. Form.repaint() marks the whole form dirty, so the paintDirty() that the presentation step already performs redraws all of it in one frame; after that there are no stages left outstanding, and the settle is observing a screen that is complete rather than one that is merely quiet. The repaint is requested before the presentation, because paintDirty() paints what is marked dirty at the moment it runs. Confined to the two tests that compose in stages, the same pair the settle already singles out. The rest of the suite is untouched, and in particular the partial-flush and dirty-clip tests, whose goldens assert what a partial repaint leaves on screen and would be meaningless after a forced full one. Both methods it calls are ones the framework itself calls from Java, so neither can be dropped by the unused-method cull the way a method existing solely for this would be: CodenameOneImplementation.getCurrentForm() on the receiver this file already holds, then Component.repaint() on the result. Both verified present in a translated bundle as cn1_s_getCurrentForm_R_com_codename1_ui_Form and cn1_s_repaint. If neither resolves, the helper logs what it found and does nothing, leaving the capture exactly as it is today -- the worst case is no improvement, never a new failure. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:585cc3f1c2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+3511
to
+3512
| if (!(op instanceof com.codename1.impl.html5.graphics.TextLayerOp)) { | ||
| return true; |
There was a problem hiding this comment.
When a full-screen transparent component repaints only promoted text, PaintSurface.paintDirty() still calls wrapper.setClip(0, 0, dwidth, dheight) before painting (CodenameOne/src/com/codename1/impl/PaintSurface.java:223-230), which queues a ClipRect. This predicate therefore returns true even though no pixels are drawn, so the full-frame clearRect still erases retained canvas content without replacement. Fresh evidence beyond the prior comment is that the newly added predicate classifies every non-TextLayerOp, including clip and transform state commands, as a pixel paint.
Useful? React with 👍 / 👎.
framePaintsPixels() asked whether a frame held anything other than a text mutation, which is the wrong question. PaintSurface.paintDirty() calls setClip(0, 0, width, height) before painting each animatable and BufferedGraphics records that unconditionally, so every painted frame carries a ClipRect whether or not a pixel follows it. A full-screen transparent component that repaints only promoted text therefore produced a frame of [ClipRect, TextLayerOp...], the predicate said "paints", and the full-frame clearRect ran against a frame with nothing to draw back. The predicate now asks what the op does. A clip or a transform records what later draws are subject to and leaves the canvas as it was; a text mutation writes to the document. None of them qualify a frame for the clear. The classification lives next to addOp as isStateOnlyOp() and both callers share it, because they are asking the same question and a second copy would drift. addOp already had this exact set inline -- it is what it keeps when the clip is empty, since a draw may not render but the clip and transform still have to be recorded for a later non-empty clip to restore drawing. Correcting the previous commit's reasoning as well as its code: that message said a frame like this used to be empty and returned early, and the always-recorded ClipRect means it never was. The clear firing on a frame that paints nothing is therefore older than these text ops, not something they introduced -- what they introduced is the detach-only frame, syncToForm releasing a departed component's runs with nothing else queued, which has no ClipRect and was genuinely empty before. Both are covered now. Found in review of #5629. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
The defect
f691c7e4a6(#5552) gave the port a DOM text layer, which makes every frame twohalves: the pixels the canvas draws and the elements the layer positions. Only
one of them is issued when the frame is decided.
promote(),endComponent()andsyncToForm()wrote to the document duringthe component paint. The canvas commands that same paint records are not
replayed and shipped until the frame is drained an animation frame later. The
browser composites in between, so it renders one half of the new frame over the
other half of the old one:
Nothing about the layer's bookkeeping was wrong, which is why it survived
review. It was applied at the wrong moment.
Reproduced on the BuildCloud console (7.0.267, real Chrome, 30fps video): a
click on a navigation row blanks every label and every icon on the form for
a frame, and draws the outgoing and incoming label of that row superimposed.
Icons go through
drawStringtoo, so they are runs and blank with the text.The fix
Every mutation becomes a
TextLayerOp(ExecutableOp) recorded into the samebuffer as the draws around it, replayed into the same
SurfaceCommandRecorder(
OP_TEXT_*, 90-96), and shipped in the same flush message. The host applies itin the task that replays that frame's canvas commands, in the draw order the
paint gave it, so no rendering opportunity exists between the two halves.
OP_BLUR_SELF_REGIONalready rides the stream for the same reason.syncToFormmoves from the drain toflushGraphics. By drain time the frame'sops are already snapshotted, so a release recorded there shipped with the next
frame — which is what put a removed component's text over the pixels that
replaced it. In
flushGraphicsthe components have finished painting and thebuffer is still open.
Two things fall out: the retained
CSSStyleDeclarationhandles are gone (styleis written by opcode now), and with them the two
getStyle()property reads perrun the class had to cache to stay off the barrier.
Measurement
Scripted 1028-frame interaction, real Chrome, 30fps, counting transient
brightness spikes (a frame brighter than the one before that recovers within
three — a navigation is a step that stays, and is not counted):
0 is what the same run measures with the layer disabled entirely
(
?cn1TextLayer=0), so the flicker is gone rather than merely smaller.scripts/verify-javascript-web-overlay.mjsreports the same 44 promoted runsand 43 semantic nodes as before, so DOM text is still real, selectable text.
Not covered
The 181-golden screenshot suite did not run:
build-javascript-port-hellocodenameone.shfails in the plugin's
cssgoal in this environment for an unrelated reason (alocally installed
codenameone-designer:8.0-SNAPSHOTreferencing a class thatexists only on another branch). Worth running in CI before merge.