Stop padding a Sheet by a CSS corner radius (issue #5488) - #5489
Conversation
A stylesheet rule such as
cntSheet { border-radius: 4mm 4mm 0mm 0mm; padding: 0mm; margin: 0mm; }
renders a band of empty space under the sheet title in 7.0.262 that was
not there in 7.0.233.
Cause: the same border class switch behind #5454. PR #5054 made a simple
border-radius compile to RoundRectBorder rather than CSSBorder, and
Sheet.show has always inset the content pane by the corner radius for
every RoundRectBorder it sees. That inset exists because a hand written
RoundRectBorder reserves twice its radius, so content would otherwise be
drawn under the rounded corners. A border out of a stylesheet reserves
nothing and the sheet is padded by whatever the CSS asked for, which here
is nothing, so the inset is 4mm of padding on all four sides that the
author never wrote. The reported app lays the sheet out in a Y box and
adds to it directly, so the empty content pane sits between the title bar
and the labels and those 8mm are the reported gap.
#5469 already stopped the radius from inflating the box, but the
cssBoxModel flag it added never reached this padding line. Skip the inset
for a CSS sized border and keep it for a hand written one. The default
themes are unaffected: neither native theme defines a Sheet UIID, so the
branch only runs for a sheet a developer styled.
Tests: SheetCssBorderRadiusTest covers the content pane picking up no
padding, the reported layout leaving no gap under the title, and a hand
written border still being inset. The first two fail on master with 4 and
8 pixels of padding respectively.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9708464110
ℹ️ 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".
There was a problem hiding this comment.
Pull request overview
This PR fixes a regression where a Sheet styled via CSS with border-radius (compiled to RoundRectBorder) would incorrectly inset/pad its content pane by the corner radius, creating a visible gap under the title (issue #5488). The change makes Sheet.show() respect the RoundRectBorder.cssBoxModel flag so CSS-sized borders don’t trigger the legacy inset behavior, and adds a targeted UI test to prevent regressions.
Changes:
- Update
Sheet.show()to skip corner-radius-based content-pane inset when the border is a CSS box-modelRoundRectBorder. - Add
SheetCssBorderRadiusTestto verify CSS-sized borders don’t add padding/gaps, while handwritten borders still inset.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
CodenameOne/src/com/codename1/ui/Sheet.java |
Avoids applying corner-radius padding to the content pane for CSS box-model RoundRectBorder to prevent the title-gap regression. |
maven/core-unittests/src/test/java/com/codename1/ui/SheetCssBorderRadiusTest.java |
Adds regression tests covering CSS border-radius behavior on Sheet and preserving legacy inset for handwritten borders. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
✅ Continuous Quality ReportTest & Coverage
Static Analysis
Generated automatically by the PR CI workflow. |
Review of the previous commit: skipping the inset for a CSS sized border is not enough when the same sheet was already shown with a hand written one. The inset is written into the style of the content pane, so it outlives the restyle and the gap comes back. Remember the padding of the content pane before the first inset, units included, and put it back when the sheet is next shown with a border that asks for no inset, a CSS sized RoundRectBorder or no RoundRectBorder at all. Restoring rather than zeroing matters because the content pane is public API: a developer who padded it keeps that padding instead of having it silently cleared. Nothing is touched when no inset was ever applied. Also drop the unused assertTrue import from the test. Tests: two more cases in SheetCssBorderRadiusTest, one restyling from a hand written border to a CSS sized one and one checking the padding a developer set in millimetres comes back in millimetres. Both fail on the previous commit with the stale 4px inset. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Cloudflare Preview
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d9ba37b92a
ℹ️ 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".
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
CodenameOne/src/com/codename1/ui/Sheet.java:968
restoreContentPanePadding()usescontentPane.getAllStyles(), which is a proxy style that writes to unselected/selected/pressed/disabled styles. The inset is applied via$(contentPane).setPaddingMillimeters(...), which (with the default selector style type) only mutatescontentPane.getStyle()(unselected). Restoring viagetAllStyles()can therefore unintentionally overwrite state-specific paddings/units that were never changed by the inset.
Style cps = contentPane.getAllStyles();
cps.setPaddingUnit(contentPaneInsetUnits);
cps.setPadding(contentPaneInset[0], contentPaneInset[1], contentPaneInset[2], contentPaneInset[3]);
Review of the previous commit: the restore went through getAllStyles while the inset goes through the component selector, which pads the current style of the content pane and leaves the selected, pressed and disabled styles alone. So restoring wrote the padding of the current style over three styles that were never insetted. Read and write the same style instead. The other styles are never part of the inset, so they have nothing to restore and are now left untouched. Tests: restoringLeavesTheOtherStylesOfTheContentPaneAlone pads the selected and pressed styles of the content pane before the first show and checks they survive the restyle. It fails on the previous commit with the selected padding replaced by 0. 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: f12aba4856
ℹ️ 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".
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
CodenameOne/src/com/codename1/ui/Sheet.java:949
- The doc comment uses the nonstandard word "insetted"; this is likely a typo and can be confusing in API documentation/search.
/// are never insetted and so have nothing to restore.
maven/core-unittests/src/test/java/com/codename1/ui/SheetCssBorderRadiusTest.java:143
- These assertion messages say the restored padding comes back "in millimetres", but the test explicitly sets the padding unit to DIPs and later asserts
UNIT_TYPE_DIPS. Updating the message avoids misleading failures/debugging output.
assertEquals(Display.getInstance().convertToPixels(1f), contentStyle.getPaddingTop(),
"the padding of the developer comes back, in millimetres");
assertEquals(Display.getInstance().convertToPixels(2f), contentStyle.getPaddingLeftNoRTL(),
"the padding of the developer comes back, in millimetres");
Review of the previous commit: the snapshot was put back unconditionally on the next show, but the style it describes can be gone by then. A theme refresh replaces the style of the content pane, and application code is free to pad it between two shows. In both cases restoring wrote a stale snapshot over padding that was deliberately set. Keep the style that was padded and the padding the inset wrote alongside the padding it replaced, and restore only into a style that is still that same object and still holds exactly what the inset left. Anything else drops the snapshot and leaves the style alone, which is the safe reading: the inset is gone in that case anyway, so there is nothing to take off. The three fields become one ContentPaneInset holding them together. Also two review nits: "insetted" is not a word, and a test message said millimetres of a padding the test writes in DIPs. Tests: aThemeRefreshBetweenShowsDropsTheSnapshot swaps the style of the content pane between the two shows, paddingChangedBetweenShowsIsNot Overwritten pads it between them. Both fail on the previous commit, which replaces the new padding with the snapshot. 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: c315719f37
ℹ️ 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".
Review of the previous commit: the style a component presents follows its state, so getStyle returns the disabled style once the content pane is disabled. A sheet shown with a hand written border while the pane was enabled therefore insetted the unselected style, and restyling it after the pane was disabled compared that inset against the disabled style, rejected it and dropped the snapshot anyway. The inset stayed in the unselected style and the gap came back with the pane. The snapshot already holds the style it was taken from, so restore into that style rather than looking up the current one, and keep one snapshot per style that was insetted rather than a single one. Which style the inset lands in is decided by the state of the pane at the time and can differ between two shows, so more than one may be outstanding. The list is capped at the four styles a component presents: an entry older than that belongs to a style that has since been replaced and is no longer attached to the content pane. Tests: disablingTheContentPaneBetweenShowsStillTakesTheInsetOff disables the pane between the two shows and checks the unselected style is clean afterwards. It fails on the previous commit with the stranded 4px inset. 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: 3644c00698
ℹ️ 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".
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
CodenameOne/src/com/codename1/ui/Sheet.java:972
- Spelling nit in JavaDoc: “insetted” should be “inset”.
/// The inset recorded for the given style of the content pane, null when that style was never
/// insetted.
CodenameOne/src/com/codename1/ui/Sheet.java:1530
- Spelling nit in JavaDoc: “insetted” should be “inset”.
/// here, and the sheet keeps one of these per style it insetted.
CodenameOne/src/com/codename1/ui/Sheet.java:813
- Spelling/grammar nit: “insetted” should be “inset”, and the sentence should end with a period to keep comments consistent and readable.
This issue also appears in the following locations of the same file:
- line 971
- line 1530
// The inset pads the current style of the content pane, which is not always the same
// style: it follows the state of the pane, so a sheet shown while the pane is disabled
// pads the disabled style. Each style that gets insetted is recorded separately
Review of the previous commit: capping the recorded insets at four and dropping the oldest assumed age says whether a style is still attached to the content pane, and it does not. A style insetted first and still in use is evicted by four later entries, stranding its inset. Drop the cap. The other way to bound the list, asking the content pane for its four styles and pruning anything not among them, would create the selected, pressed and disabled styles on a pane that never had them, and creating those registers elevation and surface state, so it is not free. Instead an entry for a style that has been replaced is carried until the next restore, where putting padding back into a detached style costs nothing. Entries there is nothing left to restore for are dropped when the next inset is recorded, which is what keeps the list short in practice. Tests: everyStyleThatWasInsetIsRestoredHoweverManyThereAre insets one style, pushes four more through the content pane, and checks the first one is still cleaned up. It fails on the previous commit with the first inset stranded. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review nit: the past participle of inset is inset, not insetted. Three comments in Sheet used the nonstandard form. 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: a863424e3e
ℹ️ 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".
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
CodenameOne/src/com/codename1/ui/Sheet.java:970
- Spelling: “insetted” is nonstandard; use “inset” in this doc comment.
/// The inset recorded for the given style of the content pane, null when that style was never
/// inset.
private ContentPaneInset contentPaneInsetFor(Style style) {
CodenameOne/src/com/codename1/ui/Sheet.java:1530
- Spelling: “insetted” is nonstandard; use “inset” in this doc comment.
/// The inset is applied through the component selector, which pads the style the content pane
/// presents at the time rather than all of its styles. Which style that is follows the state of
/// the pane, so an inset applied while it was enabled has to be taken off the unselected style
/// even if the pane is disabled by the time the sheet is restyled. Hence the style is held
/// here, and the sheet keeps one of these per style it inset.
CodenameOne/src/com/codename1/ui/Sheet.java:810
- Spelling: “insetted” is nonstandard; use “inset” (or “gets inset”) in this comment for clarity.
This issue also appears in the following locations of the same file:
- line 968
- line 1526
// The inset pads the current style of the content pane, which is not always the same
// style: it follows the state of the pane, so a sheet shown while the pane is disabled
// pads the disabled style. Each style that gets inset is recorded separately
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
CodenameOne/src/com/codename1/ui/Sheet.java:991
contentPaneInsetscan grow without bound if the content pane’sStyleinstance is replaced repeatedly (e.g., via theme refreshes /setUnselectedStyle(new Style(...))) while the sheet continues to use a non-CSSRoundRectBorder. Because intact entries for detached styles are intentionally never evicted, theSheetinstance will retain strong references to oldStyleobjects until a later restyle callsrestoreContentPanePadding(), which may never happen in typical usage.
Consider switching the tracking structure to avoid retaining detached styles indefinitely (e.g., store Style references weakly and prune cleared entries, or use a map keyed by Style with weak keys and no strong back-reference in the value).
/// Entries are not otherwise evicted. Dropping the oldest once a few have accumulated would be
/// wrong, because age does not say whether a style is still attached to the content pane, and
/// the alternative of asking the pane for its four styles would create the selected, pressed
/// and disabled ones on a pane that never had them, which registers elevation and surface
/// state. So an entry for a style that has been replaced is simply carried until the next
/// restore, where putting padding back into a detached style costs nothing.
Review of the previous commit: padding one side of the content pane after the inset went on made the whole snapshot count as changed, so restyling the sheet left the inset stranded on the other three sides. Each side is now compared and restored on its own. A side that still holds what the inset wrote is put back, a side padded since keeps what it was given. Insetting again refreshes the remembered padding of the sides that were changed, so the value preserved for a side is always the last one asked for rather than the one from before the first inset. Tests: changingOneSideAfterTheInsetLeavesTheOtherThreeRestorable pads only the top after the inset and checks the top survives while the other three are cleaned up. It fails on the previous commit with the three sides left holding the inset. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review asked whether the list can grow without bound. Record the analysis where the decision is: one entry per style the content pane presents while a hand written border is in effect, so the count follows how often those styles are replaced, a theme refresh in practice, between one show and the show that takes the inset off. Weak references would let it shrink on its own, but the portable weak reference of the platform is allowed to report that it holds nothing, and reading that as a style that went away would silently skip a restore that is still owed. No behavior change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
On the unbounded-growth note in the last Copilot pass (suppressed, low confidence): keeping it as is, with the reasoning now written into the code in 41fb3f5. One entry is added per style the content pane presents while a hand written border is in effect, so the count is bounded by how often those styles are replaced, a theme refresh in practice, between one show of the sheet and the show that takes the inset off. In an app that is zero or a handful of entries of about a hundred bytes each, all released at the next restore. Weak references are the obvious way to let the list shrink on its own, but the portable API for them, Display.createSoftWeakRef, is documented as possibly returning null when the platform does not support caching. Reading that as a style that went away would silently skip a restore that is still owed, which trades a theoretical leak for the stranded inset this PR exists to fix. The other bound, pruning against the four styles of the content pane, forces the selected, pressed and disabled ones into existence on a pane that never had them, and creating those registers elevation and surface state. Unrelated: build-test (8) failed on a863424 in MCPLoopbackTransportOpenTest, refused on port 47899 by a transport left registered by another test, a port this test never uses. It passes locally and on the other JDKs, master is green, and nothing here touches MCP. Watching whether it recurs on the current head. |
The PMD gate forbids ForLoopCanBeForeach and the index loops added for the inset bookkeeping tripped it, failing build-test (8). None of them used the index for anything but element access, so they all convert. The one that stays indexed walks backwards while removing, which foreach cannot do, and PMD does not flag it. Verified by running the gate the way CI does, mvn verify on core-unittests followed by generate-quality-report.py, which now exits clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Compared 151 screenshots: 151 matched. Native Android coverage
✅ Native Android screenshot tests passed. Native Android coverage
Benchmark ResultsDetailed Performance Metrics
|
|
Compared 181 screenshots: 181 matched. |
iOS Metal screenshot updatesCompared 149 screenshots: 148 matched, 1 updated.
Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
|
Compared 217 screenshots: 217 matched. |
|
Compared 144 screenshots: 144 matched. |
|
Compared 143 screenshots: 143 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
|
Compared 148 screenshots: 148 matched. Benchmark Results
Detailed Performance Metrics
|
Fixes #5488.
A stylesheet rule such as
renders a band of empty space under the sheet title in 7.0.262 that was not there in 7.0.233.
Cause
The same border class switch behind #5454. PR #5054 made a simple
border-radiuscompile toRoundRectBorderrather thanCSSBorder, andSheet.showhas always inset the content pane by the corner radius for everyRoundRectBorderit sees:That inset exists because a hand written
RoundRectBorderreserves twice its radius, so content would otherwise be drawn under the rounded corners. A border out of a stylesheet reserves nothing and the sheet is padded by whatever the CSS asked for, which here is nothing, so the inset is 4mm of padding on all four sides the author never wrote. The reported app lays the sheet out in a Y box and adds to it directly, so the empty content pane sits between the title bar and the labels, and those 8mm are the reported gap.#5469 already stopped the radius from inflating the box, but the
cssBoxModelflag it added never reached this padding line.Fix
Skip the inset for a CSS sized border, keep it for a hand written one. The default themes are unaffected: neither native theme defines a
SheetUIID, so the branch only runs for a sheet a developer styled, and a designer authored border is not flagged.Tests
New
SheetCssBorderRadiusTest:cssSizedBorderDoesNotPadTheContentPane— the content pane picks up no padding on any side. Fails on master with 4px per side.cssSizedBorderLeavesNoGapUnderTheTitle— reproduces the reported layout, the empty content pane takes no height. Fails on master with 8px.handWrittenBorderStillInsetsTheContentPane— a legacy border is still inset by its radius, passes either way.Full
core-unittestssuite: 4280 tests, 0 failures.🤖 Generated with Claude Code