Skip to content

refactor(data): use ancestor-aware visibility in Binder default isApplied#24478

Open
Artur- wants to merge 1 commit into
mainfrom
binder-visible-util
Open

refactor(data): use ancestor-aware visibility in Binder default isApplied#24478
Artur- wants to merge 1 commit into
mainfrom
binder-visible-util

Conversation

@Artur-

@Artur- Artur- commented May 29, 2026

Copy link
Copy Markdown
Member

Binder's default isAppliedPredicate skips bindings whose field is a non-visible Component during validation and bean writing. The previous check only considered the field's own visibility flag, so a field whose own flag is true but whose parent layout is hidden was still validated — even though the user could neither see the field nor correct any error reported on it.

Switch to ComponentUtil.isEffectivelyVisible and update the javadoc on both the default predicate and the related
setApplyBindingsToHiddenFields setter to spell out the new definition. Behaviour change at the edge: fields with ancestor-hidden state are now skipped just like fields with their own flag cleared.

…lied

Binder's default isAppliedPredicate skips bindings whose field is a
non-visible Component during validation and bean writing. The previous
check only considered the field's own visibility flag, so a field
whose own flag is true but whose parent layout is hidden was still
validated — even though the user could neither see the field nor
correct any error reported on it.

Switch to ComponentUtil.isEffectivelyVisible and update the javadoc on
both the default predicate and the related
setApplyBindingsToHiddenFields setter to spell out the new definition.
Behaviour change at the edge: fields with ancestor-hidden state are
now skipped just like fields with their own flag cleared.

Follow-up from PR #24326.
@sonarqubecloud

Copy link
Copy Markdown

@github-actions

Copy link
Copy Markdown

Test Results

 1 430 files  ±0   1 430 suites  ±0   1h 26m 24s ⏱️ + 1m 40s
10 052 tests ±0   9 984 ✅ ±0  68 💤 ±0  0 ❌ ±0 
10 524 runs  ±0  10 455 ✅ ±0  69 💤 ±0  0 ❌ ±0 

Results for commit 477beda. ± Comparison against base commit b238ec1.

@tltv tltv left a comment

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.

Technically change is correct, for new API it would be fine, but with Binder I would reconsider if changing just generate more trouble than help anyone. Note that visibility check can be already explicitly disabled (added just to help migrations since visibility check was added recently).

Needs some tests too.

Concern: Taking ancestor visibility in account in Binder is a bit dangerous now since devs may create Binder, and have a layout that's hidden by default, and bind fields that are inside the layout, and then bind bean/record => resulting fields in the hidden layout not being applied.

This could break existing apps, e.g. when using Binder to do validation for collapsed (hidden) section in the form.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants