Uh oh!
There was an error while loading. Please reload this page.
The certificate wizard's scrollbar was drawn inside out (issue #5636) - #5648
Conversation
A CN1 CSS theme is installed with setThemeProps, which replaces the whole property table -- and this one never declared the UIIDs the look and feel asks for by name. Scroll and ScrollThumb therefore came from the blank-theme defaults: a white track with a black thumb. On a dark page that reads as an inverted bar whose EMPTY part looks like the thumb, which is exactly what the report describes. The same replacement dropped @interactiveScrollBool, which the port injects into the native theme, so the desktop app got the thin mobile overlay bar despite its own stub asking for the grabbable one. Those UIIDs cannot use this app's DarkXxx prefix scheme, because the look and feel looks them up by the exact name. Their dark values go in a prefers-color-scheme block, which the CSS compiler emits as $Dark entries. The new-profile dialog had a matching gap between what it showed and what it knew. It painted the first segment selected while the model held null, so "Create" stayed disabled until you clicked the segment that already looked chosen -- and clicking it again silently cleared it. That is the reported "no combination of buttons enables Create", and the same toggle-on-re-click on the bundle and certificate rows is why a segment took a dozen clicks to land. The rows also joined their two halves with a newline, which a Button does not render, so they came out as "com.example.myappMy App". The dialog now opens on a type it actually holds, single-select means select, certificates are filtered to the ones that type can be signed with, the device list appears only for the types that are device limited (with select all and clear), and a line beside the button names the first thing still missing. The check box glyph is built from the CheckBox style rather than from the UIID the component carries, and only a light rule existed, so in dark mode the box was navy on navy. Declared in both schemes and sized in millimetres, and the dark palette moves off the saturated navy onto a neutral slate with the small type bumped -- the "tiny white fonts on a medium blue background" half of the report. White on the lime primary was near 2:1; it is ink now. CertificateWizardStructureHarness was failing 9 checks on unmodified master. Nothing runs it (it needs a display), so it had rotted: the Android page is gated on a project binding, the confirmation copy moved, and page banners are SpanLabels. Those are repaired, and the checks are split into stages that let the EDT run -- Container mutations are queued while a dialog's show animation holds the AnimationManager, so a rebuild inside a freshly shown dialog read back as if nothing had happened. 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. |
The copyright gate is diff scoped, so it checks MODIFIED files and not only added ones. theme.css and the two test sources have never carried a header; touching them is what makes that this change's problem to fix. Codename One's GPLv2 + Classpath Exception header, not Oracle's -- these are our own files. The CSS keeps its own descriptive comment underneath. 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:7960a4cfb4
ℹ️ 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".
| // Only the certificates this profile type can actually be signed with. Offering all | ||
| // of them let a Mac Installer certificate be picked for an iOS App Store profile, | ||
| // which Apple rejects at creation time -- long after the wizard said it was fine. | ||
| List<SigningState.Certificate> usable = WizardDecisions.compatibleCertificates(state, profileType[0]); |
There was a problem hiding this comment.
When an account has an active matching certificate reconciled from Apple with privateKeyPresent == false, using compatibleCertificates() hides it because that helper was designed for auto-setup/export reuse. Manual profile creation only sends the certificate's appleCertId through CloudSigningService.createProfile, so a local private key is not required; affected users are incorrectly told to generate another certificate and cannot create a profile with their existing valid certificate.
Useful? React with 👍 / 👎.
| for (SigningState.Device dev : state.devices) { | ||
| devs.add(dev.id()); |
There was a problem hiding this comment.
When the account contains disabled devices, Select all adds every device ID to the profile request and enables Create, including IDs that cannot be used for a new profile. The existing deviceIdsFor() path deliberately accepts only ENABLED or ACTIVE devices; the dialog should apply the same predicate when rendering/selecting devices so this shortcut does not produce a request Apple rejects.
Useful? React with 👍 / 👎.
The certificate picker used compatibleCertificates, which also demands a locally stored private key. Creating a profile sends the certificate's Apple ID and nothing else -- the key is needed to export the .p12 afterwards, which is why the auto-setup and reuse path insists on it. Applied here it hid a perfectly valid certificate that came back from a sync with Apple and told its owner to generate a second one they do not need. Profile creation now has its own predicate, keeping only the type match, which is the part Apple really does reject; a certificate with no stored key says so on its row instead. The device list offered every device on the account, disabled ones included, and "select all" made it one click to build a request Apple rejects whole. deviceIdsFor already excluded them for automatic setup, so both now read the same predicate rather than two copies of it. The mock service grows a disabled device and a keyless certificate so the structure harness covers both paths end to end. Reported by the Codex review on the PR. 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:3e8370d6c1
ℹ️ 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".
| // names one, so the picker offers exactly what deviceIdsFor sends. Listing them | ||
| // and letting "select all" sweep them in made one click enough to build a | ||
| // request that could not succeed. | ||
| final List<SigningState.Device> devices = WizardDecisions.usableDevices(state); |
There was a problem hiding this comment.
When an account has enabled iOS devices and the user selects Mac Development, usableDevices(state) returns those iOS devices because it filters only by status. Select all then adds their IDs and enables Create for a MAC_APP_DEVELOPMENT request, even though SigningState.Device exposes the platform and platformForProfile() maps Mac profiles to MAC_OS. Filter the displayed and selected devices by the current profile platform so the shortcut cannot create a cross-platform request.
Useful? React with 👍 / 👎.
| label(c, "No active " + typeLabel(WizardDecisions.requiredCertificateType(profileType[0])) | ||
| + " certificate. Generate one first.", "CWCardMeta"); | ||
| Button makeCert = outline("Generate certificate", "btn.profileNeedsCert"); | ||
| makeCert.addActionListener(e -> { d.dispose(); certificateDialog(); }); |
There was a problem hiding this comment.
When Mac Development is selected and no active MAC_APP_DEVELOPMENT certificate exists, this new empty-state action opens certificateDialog(), but that dialog's typeValues omits MAC_APP_DEVELOPMENT even though requiredCertificateType() requires it. None of the certificate types available after clicking the suggested remedy can satisfy the picker, so the user returns to the same disabled profile form; add the missing certificate type, ideally preselected for this path.
Useful? React with 👍 / 👎.
Two more places where the dialog let you pick a combination that cannot work. The device picker filtered on status alone, so choosing Mac Development offered the account's iPhones and "select all" swept them into a MAC_APP_DEVELOPMENT request. The platform test excludes the KNOWN WRONG platform rather than demanding the known right one: the field is an untyped string in the API, documented only as "Apple BundleIdPlatform", and an unanticipated value must not empty the picker and make a profile type uncreatable. One device too many costs a rejected request; none costs the whole flow. The empty certificate state offered to generate one, and the certificate dialog could not generate MAC_APP_DEVELOPMENT at all -- the only suggested remedy for a Mac Development profile led back to the same disabled form. That type is now offered, the dialog opens on whichever type sent you there, and the type list lives in WizardDecisions so the test that every requiredCertificateType can be generated reads the same array the dialog builds its segments from. The harness gets three stages for the Mac Development path, including that the empty device list says which platform it wanted -- otherwise "no iPhone is offered" would pass just as well if the section were never drawn. Reported by the Codex review on the PR. 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:0ef2ed7b34
ℹ️ 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".
| certificateId[0] = null; | ||
| certs.clear(); |
There was a problem hiding this comment.
When a user selects devices for iOS Development and then switches to Mac Development, this handler clears only the certificate state; devs still contains the iOS device IDs. Although the rebuilt picker is now platform-filtered and hides those devices, canCreateProfile() treats the stale list as satisfying the Mac device requirement and createProfile() submits those invalid IDs. This is fresh evidence beyond the earlier platform-filtering issue: the new filtered rebuild preserves hidden selections from the previous profile type.
Useful? React with 👍 / 👎.
| d.add(actionRow(Component.LEFT, dist, dev, macStore)); | ||
| d.add(actionRow(Component.LEFT, developerId, installer)); | ||
| name.setName("field.certName"); | ||
| name.setText(typeLabel(type[0])); |
There was a problem hiding this comment.
When the normal Generate Certificate dialog opens, this populates the display name from the initial iOS Distribution type, but selecting another type updates only type[0] and the segment styling. If the user chooses iOS Development or a Mac type without manually rewriting the field, createCertificate() submits that new type with the stale DISTRIBUTION display name, leaving the account with a misleadingly labeled certificate.
Useful? React with 👍 / 👎.
Two stale values, both of which the previous rounds made reachable. Filtering the device picker by platform meant a selection made under one profile type could keep devices the next type will not accept -- and now keep them INVISIBLY, since the picker no longer draws them. canCreateProfile read a satisfied device requirement and the request went to Apple naming iPhones in a Mac profile. The selection is pruned in the rebuild, at the one place every type change goes through, rather than in the click handler that happens to be the way the type changes today. A type that names no devices at all drops the lot. The certificate dialog's display name was seeded from the type it opened on and then left there, so choosing another type generated, say, an iOS development certificate labelled "DISTRIBUTION" -- a wrong label on a real credential that outlives the dialog. It follows the type until the user writes their own, the same rule the profile name already uses, and the harness checks all three states: the suggestion, that it follows, and that it stops once overwritten. Reported by the Codex review on the PR. 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:2a1dfbf4c0
ℹ️ 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".
| */ | ||
| Scroll, HorizontalScroll, DesktopScroll, DesktopHorizontalScroll { | ||
| background-color: transparent; | ||
| padding: 0 0.9mm 0 0.9mm; |
There was a problem hiding this comment.
When content overflows horizontally, both HorizontalScroll and DesktopHorizontalScroll receive zero top/bottom padding and zero margins from this shared rule. LookAndFeel.getHorizontalScrollHeight() calculates the scrollbar height solely from those vertical padding and margin values, so the horizontal track and thumb are rendered at zero height and cannot be grabbed; use a separate horizontal rule with nonzero vertical padding.
Useful? React with 👍 / 👎.
The track's padding is what sizes the bar, on the axis ACROSS it: getVerticalScrollWidth sums the left and right padding, and getHorizontalScrollHeight the top and bottom. One rule shared by both axes therefore cannot serve both -- the horizontal track inherited the vertical one's padding and came out zero pixels high, drawn and impossible to grab. Split by axis, in both schemes, which is the shape the android-material and ios-modern native themes already use. The theme test now asserts the padding across each bar is non-zero and the padding along it is zero, so a shared rule cannot come back. Checked against the old CSS: it fails there with "HorizontalScroll needs thickness on the axis across the bar", which is the whole point of adding it. Note scripts/settings/common/src/main/css/theme.css has the same shared rule and the same zero-height horizontal bar; left alone here, as it is a different app. Reported by the Codex review on the PR. 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:917536b12d
ℹ️ 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".
| String required = requiredCertificateType(profileType); | ||
| List<SigningState.Certificate> out = new ArrayList<SigningState.Certificate>(); | ||
| for (SigningState.Certificate c : state.certificates) { | ||
| if ("ACTIVE".equals(c.status()) && required.equals(c.certificateType()) && c.appleCertId() != null) { |
There was a problem hiding this comment.
When reconciliation returns an active DEVELOPMENT or DISTRIBUTION certificate, this exact comparison excludes it from every iOS profile picker because requiredCertificateType() returns only IOS_DEVELOPMENT or IOS_DISTRIBUTION. These generic types are supported by the certificate API, and certificateForProfile() already treats them as development/distribution-compatible; the previous dialog also allowed selecting them. Affected accounts are therefore incorrectly shown the no-certificate remedy and must generate a redundant certificate.
Useful? React with 👍 / 👎.
…ificate The picker matched the certificate type exactly, so an account whose certificates came back from a reconcile as the generic DEVELOPMENT or DISTRIBUTION types was told it had no compatible certificate at all and sent to generate a redundant one. Those two are Apple Development and Apple Distribution, which supersede the platform-specific types and are valid wherever those are -- isDevelopmentCertificate already counts DEVELOPMENT as a development certificate for both platforms, so this is the same reading applied to the distribution half. It stays a type test, not a "kind" test. DEVELOPER_ID_APPLICATION and MAC_INSTALLER_DISTRIBUTION are distribution certificates too and neither signs what the other is for, which is the confusion the type filter exists to prevent; only the two generic types widen anything, and there are tests for both directions. Automatic setup reads the same predicate, so it cannot refuse a certificate the picker just offered. Reported by the Codex review on the PR. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Fixes#5636.
The scrollbar
A CN1 CSS theme is installed with
setThemeProps, which replaces the whole property table -- andscripts/certificatewizard/common/src/main/css/theme.cssnever declared the UIIDs the look and feel asks for by name.ScrollandScrollThumbtherefore came fromUIManager's blank-theme defaults: track background white,ScrollThumb.bgColor = foreground = 0. Sampling the reporter's screenshot and a local repro gives the same thing both times -- track(255,255,255), thumb(0,0,0). On a dark page that reads as an inverted bar whose empty part looks like the thumb.The same replacement dropped
@interactiveScrollBool, whichJavaSEPort.injectDesktopThemeConstantsputs into the native theme.CertificateWizardStubalready setsAPP_DESKTOP_INTERACTIVE_SCROLLBARS = true, so the declared setting was being silently discarded and the desktop tool got the thin, un-grabbable mobile overlay bar.These UIIDs cannot use the app's own
DarkXxxprefix scheme -- the look and feel looks them up by the exact name -- so their dark values live in aprefers-color-schemeblock, which the CSS compiler emits as$Dark<UIID>entries.Why "Create" could never be enabled
The dialog painted the first segment selected while
profileTypeheldnull. Clicking that segment selected it for real; clicking it again silently went back tonull. That is the reported "I was unable to find any combination of buttons that changed this from cancel to generate", and the same toggle-on-re-click on the bundle and certificate rows is why "I had to click about 20 times to get this button to light up" (CWSegmentSelectedalso carried no border whereCWSegmentdid, so selecting one resized it and shifted its neighbours out from under the pointer).The rows joined their two halves with
"\n", which aButtondoes not render, so they came out ascom.example.myappMy AppandApp Store DistributionDISTRIBUTION.The dialog now opens on a type it actually holds; single-select means select; certificates are filtered to the ones that type can be signed with (a Mac Installer certificate could previously be chosen for an iOS App Store profile, which Apple rejects at creation time); the device list appears only for device-limited types, with Select all / Clear; and a line in the footer, beside the button it explains, names the first missing input.
Readability
The check box glyph is built from the
CheckBoxstyle rather than from the UIID the component carries, and only a light rule existed -- in dark mode the box was navy on navy, which is why "until you start checking them, they look like they might all be selected already". Declared in both schemes, sized in millimetres instead of off the label font, unchecked grey vs checked accent. The dark palette moves off the saturated navy onto a neutral slate and every small font size is bumped, for the "tiny white fonts on a medium blue background" half of the report. White on the lime primary button was near 2:1; it is ink now.The harness
CertificateWizardStructureHarnesswas failing 9 checks on unmodified master. Nothing runs it -- it needs a display -- so it had rotted: the Android page is correctly gated on a project binding, the confirmation copy moved, and page banners areSpanLabels thatfindTextcould never see. All repaired, and the checks are split into stages that let the EDT run between them:Containermutations are queued while a dialog's show animation holds theAnimationManager, so a rebuild inside a freshly shown dialog reads back as if nothing happened. The run command is now in the class javadoc.Verification
Built and ran the wizard locally against the mock service, before and after, screenshotting each state; measured the thumb and track colours out of the pixels rather than eyeballing them.
mvn install -Pexecutable-jar: 48 tests pass, including new coverage for the fixed UIIDs, the contrast rules, the segment box, anddescribeMissingProfileInput(exhaustively cross-checked againstcanCreateProfile). The structure harness is at 0 failures, from 9 on master.Still open, and not taken on here: nothing in PR CI builds
scripts/certificatewizard, so none of these tests run before a release tag, and the harness needs a display to run at all.🤖 Generated with Claude Code