Add BorderStyle property to Entry#33310
Conversation
jfversluis
left a comment
jfversluis
left a comment
There was a problem hiding this comment.
This would have to target the .NET 11 branch and also needs a good set of tests to make sure that none of the existing functionality is broken with this.
|
Sure @jfversluis I'll write the tests and reopen another PR that targets .NET 11 branch and put everything in it |
|
What happens when you set hasBorder and, for example, a red background to entry? |
|
Editor also has this underline :) |
kubaflo
left a comment
kubaflo
left a comment
There was a problem hiding this comment.
I think that UITests would be valuable here
|
We should decide which approach is better. This or #33309. However, in my opinion, either of these would be a great addition to net11! |
Good idea, I'll add the tests |
I agree this would be valuable. But I'd prefer to keep this PR focused on |
pictos
left a comment
pictos
left a comment
There was a problem hiding this comment.
I did test and found some inconsistency between platforms. Android it adds an underline and iOS it adds a border around the entry. Since the property name is Border I would expect it to add a border around the entry, for Android, as well.
|
Thanks for your feedback @pictos |
|
@mhrastegari I think the name is the main problem. As a mobile dev when I see Also, if you go on google and type something like "edit text compat with border" or "UiTextView with border" will can see images of the control wrapped inside a border and not just the underline |
|
How about public enum BorderStyle
{
/// <summary>
/// Shows the platform's default border/decoration style.
/// </summary>
Default,
/// <summary>
/// Hides the border/decoration.
/// </summary>
None
} |
|
@mhrastegari I like it |
|
If this is Accepted, I'll close it and open a new PR targeting .NET 11 branch |
|
@mhrastegari please don't close, just change a base :) |
|
Done @kubaflo |
mhrastegari
changed the title
HasBorder property to EntryBorderStyle property to Entry
|
/rebase |
|
|
|
@jfversluis what do you think? |
|
/review -b feature/refactor-copilot-yml |
|
|
AI code review for net11.0 targetVerdict: Needs changes (predicted build break from an incomplete rename) This is a non-approval, automated review comment. It is advisory only — human approval is required. What this PR doesAdds a new Findings (blocking)
Both look like remnants of a Findings (non-blocking)
CI noteOnly Confidence: High on the netstandard |
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 33310Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 33310" |
AI code review refresh for net11.0 targetHead reviewed: 7341703 Verdict: Needs changes — a compile-breaking leftover from the Blocking finding (high confidence)
Other observations (lower signal)
CI noteOnly GitHub-app checks have reported so far ( Automated non-approving review refresh. This is not an approval or a request-changes; a human maintainer should make the final merge decision. |
AI code review refresh for net11.0 targetHead reviewed: 56a0df6 Verdict: Needs changes (completeness/correctness gaps; not a CI blocker for this PR)What changed since the prior round
What looks good
High-signal findings
CI note
ConfidenceMedium-high on findings #1 and #2 (code-evident from the diff and current sources); medium on #3 (behavioral edge case, not verified on-device). Non-approval disclaimer: This is an automated, advisory AI review refresh. It is not an approval and does not request changes via GitHub review state. A human maintainer must make the final merge decision. |
kubaflo
left a comment
kubaflo
left a comment
There was a problem hiding this comment.
Synthesized review — PR #33310 (Entry BorderStyle API)
Verdict: NEEDS_CHANGES (confidence: high)
Head reviewed: 56a0df6 · Target: net11.0
The BorderStyle enum (Default/None), IEntry.BorderStyle, the Entry bindable property, and the iOS/Android/Windows handlers + device tests are coherent and well-formed. However, Tizen support is incomplete in a way that breaks the -tizen build, and the Android handler has two correctness issues. The Android cursor-reset finding below is net-new versus the prior automated review rounds (which already flagged the Tizen and Default-tint gaps).
Key findings (all code-confirmed at head)
- Tizen build break + incomplete coverage (error) —
EntryHandler.cs:32addsMapBorderStyleto the shared mapper, butEntryHandler.Tizen.csnever defines it → CS0103 on the-tizenTFM (Core builds Tizen viaCore.csproj). Same root cause: Corenet-tizen/PublicAPI.Unshipped.txtwasn't updated (→ RS0016), andEntryHandler2.Android.cs(Material) has its own mapper missingBorderStyle(silent no-op). - Android cursor reset (warning) —
EntryHandler.Android.cs:95MapBorderStylecallshandler.UpdateValue(nameof(IEntry.Text)); AndroidUpdateTextdoesSetSelection(Text.Length), moving the caret to the end on a border-only change. iOS/Windows don't do this; the call is unnecessary. - Android
Defaulttint fallback (suggestion) —EditTextExtensions.cs:78only restores the accent tint when API ≥ 23 and accent resolution succeeds; otherwise a priorNone(transparent) tint persists and the underline stays invisible. Add an unconditionalBackgroundTintList = null;fallback.
Dropped / not surfaced
- IEntry.cs:16 "source-breaking interface addition" (gpt-5.5, error) — dropped as by-design: adding the member to
IEntry+PublicAPI.Unshipped.txtis the sanctioned MAUI pattern for this feature (every other Entry property lives onIEntry). - Gemini's separate Controls
net-tizenPublicAPI item was folded into finding #1 (that file was correctly updated; the real gap is Corenet-tizen).
CI
GitHub Actions only: add-dogfood-comment pass, license/cla pass, Bump global.json skipping, Build Analysis pending. No failing required checks here, but gh pr checks does not reflect the AzDO maui-pr build/device-test legs — and default PR CI does not compile Tizen, so the CS0103/RS0016 Tizen gap will not turn this PR red despite being a real build break for Tizen consumers.
| [nameof(IEntry.ClearButtonVisibility)] = MapClearButtonVisibility, | ||
| [nameof(IEntry.Font)] = MapFont, | ||
| [nameof(IEntry.IsPassword)] = MapIsPassword, | ||
| [nameof(IEntry.BorderStyle)] = MapBorderStyle, |
There was a problem hiding this comment.
Tizen build break + incomplete platform coverage. Adding [nameof(IEntry.BorderStyle)] = MapBorderStyle to the shared mapper makes every TFM resolve MapBorderStyle, but EntryHandler.Tizen.cs never defines it (Tizen compiles its own partial, not EntryHandler.Standard.cs), so the -tizen TFM built by Core.csproj fails with CS0103. Two related gaps share the same root cause: (1) src/Core/src/PublicAPI/net-tizen/PublicAPI.Unshipped.txt was not updated with Microsoft.Maui.BorderStyle / Microsoft.Maui.IEntry.BorderStyle.get (RS0016 on the Tizen Core build, even though every other TFM's PublicAPI was updated); (2) EntryHandler2.Android.cs (Material) declares its own Mapper with no BorderStyle entry, so setting BorderStyle is a silent no-op on the Material Android path. Add a Tizen MapBorderStyle/UpdateBorderStyle, the net-tizen Core API entries, and the EntryHandler2 mapper entry. (Default PR CI does not compile Tizen, so this won't show red here, but it breaks Tizen consumers.)
|
|
||
| public static void MapBorderStyle(IEntryHandler handler, IEntry entry) | ||
| { | ||
| handler.UpdateValue(nameof(IEntry.Text)); |
There was a problem hiding this comment.
MapBorderStyle calls handler.UpdateValue(nameof(IEntry.Text)) before applying the border-only change. On Android MapText -> UpdateText reassigns EditText.Text and calls SetSelection(Text.Length) (EditTextExtensions.cs:29), so toggling BorderStyle while the user is editing moves the caret to the end and re-runs text-side effects even though no text changed. iOS and Windows MapBorderStyle don't do this -- UpdateBorderStyle only touches BackgroundTintList, so this line is unnecessary (it appears copied from the MapIsPassword/InputType pattern, where it is justified). Remove it, and consider a regression test asserting CursorPosition/SelectionLength are preserved across a BorderStyle change.
|
|
||
| public static void UpdateBorderStyle(this EditText editText, IEntry entry) | ||
| { | ||
| if (entry.BorderStyle is BorderStyle.Default) |
There was a problem hiding this comment.
The Default branch only restores BackgroundTintList when OperatingSystem.IsAndroidVersionAtLeast(23) AND the accent attribute resolves AND the color state list is non-null. If any check fails (e.g. API < 23, or accent/attribute resolution fails) the method returns without touching the tint, so after toggling None -> Default the previously-set transparent tint persists and the underline stays invisible. Add an unconditional fallback at the end of the Default branch (e.g. editText.BackgroundTintList = null;) so the platform default tint is restored even when accent resolution fails.
6 participants
Note
Are you waiting for the changes in this PR to be merged?
It would be very helpful if you could test the resulting artifacts from this PR and let us know in a comment if this change resolves your issue. Thank you!
Description of Change
This change adds
BorderStyleproperty to theEntryso that developers can easily create borderless inputs without custom handlers/renderers or platform-specific hacks.Platform
Issues Fixed
Fixes #33300
Fixes #7906
Screenshots