Skip to content

feat(ui): establish component boundary with accessible tabs - #540

Open
FenjuFu wants to merge 3 commits into
apache:mainfrom
FenjuFu:feat/ui-tab-boundary
Open

FenjuFu wants to merge 3 commits into
apache:mainfrom
FenjuFu:feat/ui-tab-boundary

Conversation

@FenjuFu

@FenjuFu FenjuFu commented Sep 10, 2026

Copy link
Copy Markdown

What and why

Shared custom-field tabs now use an app-owned component and test contract. Arrow keys move focus without fetching a table; Enter/Space activates the selected table. Native buttons and CDK handle keyboard navigation, disabled items and RTL, while the feature receives a table-name string and owns loading the panel.

This also establishes the first migration increment from #530: ADR 0005 specifies the complete component/CVA value, keyboard/ARIA, browser-helper, theming and rollout contracts; the Ionic import ratchet records existing use while rejecting new unsuppressed dependencies outside UI implementations. Existing Material/i18n restrictions and other-rule allowances are preserved. AGENTS.md and STYLE.md direct new feature/shared code through app/ui.

Refs #530. Remaining tab strips, popup controls, form CVAs, cosmetic primitives, deployment-token expansion and legacy browser helpers remain under the overall migration issue. This PR delivers the boundary and one independently verifiable behavioural primitive; it does not close that issue.

Verification

  • Final head: e992e9b88315455bcd310dc6941946e3804f7e49.

  • Local: 17 rendered Angular unit/integration tests and 3 ESLint boundary tests passed. App/E2E TypeScript checks, changed-source ESLint, formatting and git diff checks passed. Other lint-rule allowances were compared with the base and preserved.

  • Full fork CI passed on this head, including full unit suites, full lint/suppression pruning, production/container builds, API drift, translations, security, GA and license/RAT checks.

  • Full fork E2E passed on the same head: all mocked and real-Fineract shards, mobile and two-factor authentication. The new custom-field cases exercise the app-owned keyboard/panel contract at 1280px and 390px; they use mocked group data and attach screenshots after loading completes.

  • All three PR commits have matching FenjuFu noreply sign-offs and valid GitHub signatures. No generated API files were hand-edited. The upstream signed-commit check passed.

  • Upstream CI, E2E and CodeQL still require maintainer approval. The fork results above are independent evidence.

Screenshots

Unmodified image attachments from the passing new CI browser cases, showing the custom-field region after the panel finishes loading. Screenshots are stored on a separate review branch.

Desktop, 1280px viewport

App-owned custom-field tabs on desktop

Narrow, 390px viewport

App-owned custom-field tabs on a narrow viewport

AI assistance (optional)

  • Tool: Codex.

  • Harness / workflow: assisted implementation and review, targeted local checks, and full fork GitHub Actions validation.

Checklist

  • I did not hand-edit generated files under src/app/api/.

  • Feature translation uses the core adapter; DOM/focus behaviour is confined to the app/ui primitive.

  • User-facing static strings use translation keys; registered table names remain user data.

  • Added rendered unit, import-boundary and browser contract tests.

  • UI changes have browser coverage; full mocked and real-backend suites passed.

  • Commits are signed and contain matching sign-offs.

  • Followed the AI-assisted contributions guidance.

Define the component, CVA, theme and browser-test migration contracts. Ratchet existing Ionic imports and migrate shared entity datatable tabs to an app-owned CDK-backed primitive with keyboard and ARIA contract tests.

Refs apache#530

Signed-off-by: FenjuFu <92919259+FenjuFu@users.noreply.github.com>
Make the initial UI import instructions agree with the new boundary: feature/shared code uses app-owned primitives, existing imports follow the migration baseline, and direct vendor imports belong to UI implementations.

Signed-off-by: FenjuFu <92919259+FenjuFu@users.noreply.github.com>
Complete the mocked business-date endpoint and attach desktop/narrow screenshots after the tab panel stops loading. Product source remains unchanged.

Signed-off-by: FenjuFu <92919259+FenjuFu@users.noreply.github.com>
border: 0;
border-bottom: 2px solid transparent;
background: transparent;
color: var(--text-secondary);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--text-secondary is not defined anywhere in the repository — this is its only occurrence. The declaration is therefore invalid at computed-value time, so color falls back to inherit and unselected tabs render at full --text-color rather than the muted tone intended here. The de-emphasis never appears in either theme.

The repo's token for this is --text-muted (src/styles/_common.scss:33, redefined for dark at :92).

Suggested change
color: var(--text-secondary);
color: var(--text-muted);

Comment thread eslint.config.js
Comment on lines +246 to +255
// UI implementations may name their vendor, but retain the Material and i18n boundaries.
files: ['src/app/ui/**/*.ts', 'src/app/testing/ionic-testing.ts'],
rules: {
'no-restricted-imports': [
'error',
{
patterns: restrictedImportPatterns.filter(
(pattern) => !pattern.group.includes('@ionic/angular'),
),
},

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This filter drops the entire @ionic/angular pattern for src/app/ui/** and src/app/testing/ionic-testing.ts, which also removes ADR 0003's ban on Ionic's imperative controllers in those paths. Before this PR the controller restriction applied to every file with no exemption.

Confirmed by running ESLint against the new config:

src/app/ui/probe.ts       <- ModalController   from @ionic/angular/standalone: ALLOWED
src/app/ui/probe.ts       <- AlertController   from @ionic/angular/standalone: ALLOWED
src/app/ui/probe.ts       <- LoadingController from @ionic/angular:            ALLOWED
src/app/features/probe.ts <- ModalController   from @ionic/angular/standalone: BLOCKED

ADR 0005 states "Imperative controllers still use OVERLAY" and "Existing OVERLAY, I18N, STORAGE and DOWNLOAD adapters remain in force", so the config is now weaker than the decision it implements. A UI primitive that needs a modal should still go through the OVERLAY adapter rather than reaching for ModalController.

Suggest narrowing the exemption to components only, rather than removing the Ionic entry — keep the controller-scoped importNames restriction that the old config had:

patterns: [
  ...restrictedImportPatterns.filter((pattern) => !pattern.group.includes('@ionic/angular')),
  {
    group: ['@ionic/angular', '@ionic/angular/*'],
    importNames: [
      'ModalController',
      'ToastController',
      'AlertController',
      'LoadingController',
      'ActionSheetController',
      'PopoverController',
    ],
    message:
      "Use the OVERLAY adapter from 'app/core/adapters' instead of Ionic's controllers, inside src/app/ui as well. See DOCS/adr/0003-adapter-boundary.md.",
  },
],

Comment on lines +40 to +53
test('UI implementations may use Ionic but cannot bypass other adapter boundaries', async () => {
assert.equal(
(await importErrors('src/app/ui/probe.ts', '@ionic/angular/standalone', 'IonButton')).length,
0,
);
assert.equal(
(await importErrors('src/app/ui/probe.ts', '@angular/material/button', 'MatButton')).length,
1,
);
assert.equal(
(await importErrors('src/app/ui/probe.ts', '@ngx-translate/core', 'TranslateService')).length,
1,
);
});

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test name promises UI implementations "cannot bypass other adapter boundaries", but it only asserts Material and ngx-translate — which is why the controller gap in eslint.config.js goes unnoticed. Worth closing the loop here so the assertion matches the name:

  assert.equal(
    (await importErrors('src/app/ui/probe.ts', '@ionic/angular/standalone', 'ModalController'))
      .length,
    1,
  );

This currently returns 0. With the importNames entry restored in the src/app/ui/** override it returns 1, and the OVERLAY boundary stays enforced across the whole tree as the migration adds primitives that need dialogs.

Comment on lines +86 to +89
button[aria-selected='true'] {
color: var(--primary-color);
border-bottom-color: var(--primary-color);
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The selected tab's label is body-sized text in --primary-color. On --card-bg that is #3498db on #ffffff ≈ 3.2:1, short of the 4.5:1 WCAG AA minimum for body text. Dark theme is fine (#3498db on #1e1e1e ≈ 5.2:1). The border-bottom-color on the next line is a non-text indicator at 3:1 and is fine as-is.

The token comment in _common.scss does say "Keep --primary-color for accents, borders and text on light backgrounds", but that clause sits directly beneath its own measurement of the same ratio at 3.15:1 — it holds for accents and borders, not for text.

Worth flagging that --primary-strong is not a drop-in fix: it is not redefined in the [data-theme='dark'] block, and #2471a3 on #1e1e1e is ≈3.2:1, so a blanket swap trades the light-theme failure for a dark-theme one. This needs a themed token, the way --guidance-highlight-color is already themed in _common.scss for exactly this reason ("one colour cannot serve both").

Given ADR 0005 makes "dark mode, focus visibility, touch target size and overflow ... part of each primitive's acceptance", the first primitive seems like the right place to establish that token.

role="tab"
data-testid="ui-tab"
[id]="tabId(tab.value)"
[attr.aria-controls]="panelId()"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor, on edge states in the ARIA contract:

aria-controls is emitted unconditionally, but the consumer renders the role="tabpanel" element only under @if (activeTable(); as dt). Before a selection resolves — and in EntityDatatablesComponent the tabs render as soon as datatables() is non-empty — every tab points at an ID that is not in the document.

Two related cases, both reachable through the same path: when value() matches no tab, no button carries aria-selected="true"; and when every tab is disabled, focusIndex is -1 so the strip has no tab stop at all (asserted as intended at the end of the unit test). APG expects a tablist to keep exactly one tab stop and one selected tab, so a screen-reader user can land on the strip and find out where they are.

EntityDatatablesComponent also sets activeTable to data[0] unconditionally while tableTabs filters on registeredTableName — so a first table without a name yields a rendered panel whose aria-labelledby resolves to nothing, with no tab selected. Narrow, but it is the same class of gap and the non-null assertions in the template already acknowledge the field is optional.

Tests consume roles, accessible names, selected/expanded/disabled state and stable `data-testid`
scopes. They never inspect vendor shadow DOM, CSS classes or `CustomEvent.detail`. Helpers in
`e2e/utils/ui-locators.ts` are the new seam. Existing Ionic helpers stay for unmigrated controls;
do not extend them for app-owned primitives. Stable hooks also apply to guided tours.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On "Stable hooks also apply to guided tours" — worth recording the concrete debt here, since it is invisible from this diff.

src/app/core/services/guidance.service.ts:63 sets TAB_GROUP_SELECTOR = 'ion-segment', used by five tours. Nothing breaks in this PR: the record-view outer strips are still ion-segment and match ahead of the nested custom-fields strip in DOM order. But the doc comment above it now reads falsely —

All sixteen components with a tab strip render an ion-segment, so that is what the step points at.

That comment exists precisely because the previous selector (.tab-group) silently matched nothing after the Material port and four tours pointed at empty space for some time. The same failure mode returns the moment rollout step 1 migrates a record-view strip — silently, since a tour step that matches nothing does not fail CI.

Either update the comment now to say the selector is mid-migration, or promote the tours to a data-testid hook as part of step 1 rather than after it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants