Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[NumberBox] fix for text overlapping over spinbuttons #5985

Conversation

eugenegff
Copy link
Contributor

Fixes #5983

@ghost ghost added the needs-triage Issue needs to be triaged by the area owners label Sep 27, 2021
@StephenLPeters
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@StephenLPeters StephenLPeters added area-NumberBox NumberBox Control team-Controls Issue for the Controls team and removed needs-triage Issue needs to be triaged by the area owners labels Sep 27, 2021
@eugenegff
Copy link
Contributor Author

some problems with Azure Pipelines, need to rerun

@beervoley
Copy link
Contributor

some problems with Azure Pipelines, need to rerun

The pipeline run failed due to failing tests:

image

Please, could you have a look and fix them if possible?:)

@eugenegff
Copy link
Contributor Author

eugenegff commented Sep 28, 2021

Please, could you have a look and fix them if possible?:)

You right, I did not realize that this project was used for automated tests. Will force push only actual fix for the problem, removing modifications to test project

@eugenegff eugenegff force-pushed the NumberBox-text-spinbuttons-overlapping-fix branch from c22a6b1 to fd18422 Compare September 28, 2021 18:55
@beervoley
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@eugenegff
Copy link
Contributor Author

Summary of TAEF Warnings:
Warning: TAEF: Forcibly terminating child process (PID: 6340).

Summary of Errors Outside of Tests:
Error: TAEF: [HRESULT 0x800705B4] A test timeout expired while running a test operation: 'ClassInitialize'. (The process hosting the test code was terminated by TAEF while invoking a test operation.)

Summary of Non-passing Tests:
Windows.UI.Xaml.Tests.MUXControls.InteractionTests.WebView2Tests.WebMessageReceivedTest [Blocked]
Windows.UI.Xaml.Tests.MUXControls.InteractionTests.WebView2Tests.ReloadTest [Blocked]
Windows.UI.Xaml.Tests.MUXControls.InteractionTests.WebView2Tests.NavigateToStringTest [Blocked]
Windows.UI.Xaml.Tests.MUXControls.InteractionTests.WebView2Tests.SourceBindingTest [Blocked]
Windows.UI.Xaml.Tests.MUXControls.InteractionTests.WebView2Tests.GoBackAndForwardTest [Blocked]
Windows.UI.Xaml.Tests.MUXControls.InteractionTests.WebView2Tests.ResizeTest [Blocked]
Windows.UI.Xaml.Tests.MUXControls.InteractionTests.WebView2Tests.BasicPanTouchTest [Blocked]
Windows.UI.Xaml.Tests.MUXControls.InteractionTests.WebView2Tests.ScaledTouchTest [Blocked]
Windows.UI.Xaml.Tests.MUXControls.InteractionTests.WebView2Tests.MoveTest [Blocked]
Windows.UI.Xaml.Tests.MUXControls.InteractionTests.WebView2Tests.ReparentElementTest [Blocked]
Windows.UI.Xaml.Tests.MUXControls.InteractionTests.WebView2Tests.SourceBeforeLoadTest [Blocked]
Windows.UI.Xaml.Tests.MUXControls.InteractionTests.WebView2Tests.VisibilityHiddenTest [Blocked]
Windows.UI.Xaml.Tests.MUXControls.InteractionTests.WebView2Tests.VisibilityTurnedOnTest [Blocked]
Windows.UI.Xaml.Tests.MUXControls.InteractionTests.WebView2Tests.ParentVisibilityHiddenTest [Blocked]

…dth is unrestricted

(cherry picked from commit c22a6b1)
@eugenegff eugenegff force-pushed the NumberBox-text-spinbuttons-overlapping-fix branch from fd18422 to d0a1cf0 Compare September 30, 2021 15:20
@eugenegff
Copy link
Contributor Author

@eugenegff after #5994 and #5993 are checked in, please merge the updated main to fix the test issues we're running into.

done

@eugenegff
Copy link
Contributor Author

eugenegff commented Sep 30, 2021

Issue, that is fixed by this PR

image

@beervoley
Copy link
Contributor

Issue, that is fixed by this PR

image

Could you please also attach the screenshot with the fix applied?:)

@beervoley
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@eugenegff
Copy link
Contributor Author

With fix, width unlimited, NumberBox grows as needed
image

With limited width=140, text is clipped but does not overlap buttons
image

@beervoley
Copy link
Contributor

With fix, width unlimited, NumberBox grows as needed image

With limited width=140, text is clipped but does not overlap buttons image

@ranjeshj @StephenLPeters is it expected behavior?

@beervoley beervoley requested a review from teaP October 1, 2021 16:21
@StephenLPeters
Copy link
Contributor

Probably, is there a proposed "better" behavior?

@StephenLPeters
Copy link
Contributor

I'm surprised it is clipping from the left, although I think this is the better behavior. I think this should likely be scrollable though.

@eugenegff
Copy link
Contributor Author

eugenegff commented Oct 4, 2021

I'm surprised it is clipping from the left, although I think this is the better behavior. I think this should likely be scrollable though.

There are scroller inside, it remains scrolled to the position where it was scrolled to on focus lost event. On screenshot I just entered "123456789" text and caret was at the end. Usually for not just entered values it would be clipped from the right. Anyway, it is untweaked TextBox behavior, the same happens with NumberBox before this fix, as it clips text too, just at different point, somewhere between spin buttons.

@StephenLPeters
Copy link
Contributor

ah, so you can scroll this content to see the 12 that is being clipped in the screenshot?

@eugenegff
Copy link
Contributor Author

eugenegff commented Oct 4, 2021

ah, so you can scroll this content to see the 12 that is being clipped in the screenshot?

You need to focus on NumberBox to do this. But then, yes, by moving caret.

OTOH, I have no HorizontalWheel on my mouse, but I still doubt that events from HWheel would be routed to unfocused control under the mouse pointer.

Scrolled by focusing and moving caret:
image

Default state, when value (12345678) is set from Model rather then being entered:
image

But all of this is "bad path", where control is artificially constrained to inevitably clip content. The fixed bug was in "happy path" - when control is not constrained at all, but anyway overlapped text and buttons.

@eugenegff
Copy link
Contributor Author

"Happy path" (unconstrained NumberBox width) with fix
image

"Happy path" (unconstrained NumberBox width) without fix
image

@eugenegff
Copy link
Contributor Author

eugenegff commented Oct 4, 2021

Note also, that SpinButtonPlacementMode="Compact" has no such bug, only SpinButtonPlacementMode="Inline"
image

@eugenegff
Copy link
Contributor Author

eugenegff commented Oct 4, 2021

Probably, is there a proposed "better" behavior?

In our app space is premium, so we slightly restyled NumberBox - among other we set NumberBox.InputBox.TextWrapping = TextWrapping::Wrap to remove "Clear all" button, but this had nice side effect for too large texts

image

That idea still requires non-overlapping TextBox and buttons, i.e. fix from current PR

@StephenLPeters
Copy link
Contributor

Probably, is there a proposed "better" behavior?

In our app space is premium, so we slightly restyled NumberBox - among other we set NumberBox.InputBox.TextWrapping = TextWrapping::Wrap to remove "Clear all" button, but this had nice side effect for too large texts

image

That idea still requires non-overlapping TextBox and buttons, i.e. fix from current PR

I do think this makes more sense to me at first glance, @SavoySchuler What do you think? @eugenegff could you open another issue detailing this as to not block this awesome change?

@eugenegff
Copy link
Contributor Author

@StephenLPeters , @beervoley can we speed up things a little bit?

@eugenegff eugenegff changed the title Number box text spinbuttons overlapping fix [NumberBox] fix for text overlapping over spinbuttons Oct 7, 2021
@StephenLPeters
Copy link
Contributor

@StephenLPeters , @beervoley can we speed up things a little bit?

Sorry about that! looks good.

@StephenLPeters StephenLPeters merged commit 60cfdb6 into microsoft:main Oct 7, 2021
@eugenegff eugenegff deleted the NumberBox-text-spinbuttons-overlapping-fix branch October 7, 2021 18:07
@ghost
Copy link

ghost commented Apr 14, 2022

🎉Microsoft.UI.Xaml v2.8.0-prerelease.220413001 has been released which incorporates this pull request.:tada:

Handy links:

@ghost
Copy link

ghost commented Jul 13, 2022

🎉Microsoft.UI.Xaml v2.8.0-prerelease.220712001 has been released which incorporates this pull request.:tada:

Handy links:

@ghost
Copy link

ghost commented Jul 18, 2022

🎉Microsoft.UI.Xaml v2.8.0 has been released which incorporates this pull request.:tada:

Handy links:

@ghost
Copy link

ghost commented Jan 3, 2023

🎉Microsoft.UI.Xaml v2.8.2 has been released which incorporates this pull request.:tada:

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-NumberBox NumberBox Control team-Controls Issue for the Controls team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Regression, fix proposed] NumberBox content overlaps with increment button in win11 styles
3 participants