Disable TabView scroll buttons based on scroll position #2162

0x7c13 · 2020-03-25T11:21:30Z

Disable TabView scroll buttons based on scroll position for a better accessibility UX.

Description

If TabView cannot scroll to the left (horizontal offset = 0), disable DecreaseScroll button.
If TabView cannot scroll to the right (horizontal offset = scrollable width), disable IncreaseScroll button.

Motivation and Context

Fixes #2143

How Has This Been Tested?

Tested using MUXControlsTestApp with below tested scenarios:

When buttons are visible. Scroll all the way from left to right and right to left to verified their visual state.
When buttons are visible. Using increase/decrease button to scroll the strip and verified their visual state.
When buttons are hidden, decrease size of the app window to make buttons appear and verified their visual state
When buttons are visible, increase size of the app window to make buttons disappear and verified their visual state.

Screenshots (if appropriate):

Disable TabView scroll buttons based on scroll position

dev/TabView/TabView.cpp

Store the return value of scrollViewer.ViewChanged in a revoker

marcelwgn · 2020-03-25T12:39:30Z

dev/TabView/TabView.cpp

+        auto decreaseButton = SharedHelpers::FindInVisualTreeByName(scrollViewer, L"ScrollDecreaseButton").as<winrt::RepeatButton>();
+        auto increaseButton = SharedHelpers::FindInVisualTreeByName(scrollViewer, L"ScrollIncreaseButton").as<winrt::RepeatButton>();


It seem that those button are always the same and do not get unloaded and loaded, so it might be better to get a reference in OnApplyTemplate instead of searching for those buttons every time we have changed the scrolling position.

We can save the buttons as private members. I was wondering why we are doing FindInVisualTree instead of GetTemplateChild, but that might be because the button is in a re-templated scrollviewer inside the control template ?

I don't see a problem of getting it in OnApplyTemplate. I will make the changes to see if it works as indented and push out changes if it does.

@ranjeshj you are right, it is not possible to get from GetTemplateChild since it is in a re-templated scroll-viewer. And this is true for both ScrollViewer and it's repeated buttons.

performance wise, I don't think this proposal is viable, FindInVisualTreeByName is expensive and we can't call it every time the view changes, I hope we are able to find a way to store a pointer to the buttons in a tracker_ref

I will save them after ListView loaded so we only do it once.

marcelwgn · 2020-03-25T12:45:57Z

Can you please add an interaction test for this? :)

You can take a look at the following test, to see how you can test scrolling behavior:

microsoft-ui-xaml/dev/NumberBox/InteractionTests/NumberBoxTests.cs

Lines 302 to 328 in f083cde

    
           public void ScrollTest() 
        
           { 
        
               using (var setup = new TestSetupHelper("NumberBox Tests")) 
        
               { 
        
                   RangeValueSpinner numBox = FindElement.ByName<RangeValueSpinner>("TestNumberBox"); 
        
                   numBox.SetValue(0); 
        
                   Wait.ForIdle(); 
        
                   Log.Comment("Verify that scroll doesn't work when the control doesn't have focus."); 
        
                   FindElement.ByName("MaxCheckBox").SetFocus(); 
        
                   Wait.ForIdle(); 
        
                   InputHelper.RotateWheel(numBox, 1); 
        
                   Wait.ForIdle(); 
        
                   Verify.AreEqual(0, numBox.Value); 
        
                   FindTextBox(numBox).SetFocus(); 
        
                   InputHelper.RotateWheel(numBox, 1); 
        
                   InputHelper.RotateWheel(numBox, 1); 
        
                   Wait.ForIdle(); 
        
                   Verify.AreEqual(2, numBox.Value); 
        
                   InputHelper.RotateWheel(numBox, -1); 
        
                   InputHelper.RotateWheel(numBox, -1); 
        
                   InputHelper.RotateWheel(numBox, -1); 
        
                   Wait.ForIdle(); 
        
                   Verify.AreEqual(-1, numBox.Value);

Setup UI for testing scroll viewer's increase/decrease button

0x7c13 · 2020-03-25T13:54:36Z

Can you please add an interaction test for this? :)

You can take a look at the following test, to see how you can test scrolling behavior:

microsoft-ui-xaml/dev/NumberBox/InteractionTests/NumberBoxTests.cs

Lines 302 to 328 in f083cde

public void ScrollTest()

{

using (var setup = new TestSetupHelper("NumberBox Tests"))

{

RangeValueSpinner numBox = FindElement.ByName<RangeValueSpinner>("TestNumberBox");

numBox.SetValue(0);

Wait.ForIdle();

Log.Comment("Verify that scroll doesn't work when the control doesn't have focus.");

FindElement.ByName("MaxCheckBox").SetFocus();

Wait.ForIdle();

InputHelper.RotateWheel(numBox, 1);

Wait.ForIdle();

Verify.AreEqual(0, numBox.Value);

FindTextBox(numBox).SetFocus();

InputHelper.RotateWheel(numBox, 1);

InputHelper.RotateWheel(numBox, 1);

Wait.ForIdle();

Verify.AreEqual(2, numBox.Value);

InputHelper.RotateWheel(numBox, -1);

InputHelper.RotateWheel(numBox, -1);

InputHelper.RotateWheel(numBox, -1);

Wait.ForIdle();

Verify.AreEqual(-1, numBox.Value);

Any pointer on how to run UI/Integration tests? I don't see any documentation in the README.

marcelwgn · 2020-03-25T13:58:31Z

You can run them through the Visual Studio Test explorer.

0x7c13 · 2020-03-25T14:08:54Z

You can run them through the Visual Studio Test explorer.

Ok, I was able to run the tests. Previously the "run" button was grayed out because I did not build it after few changes.

Btw, the NumberBox scroller trick does not apply here since the scroll viewer in NumberBox UI Test is external and owned by the test page. Here we are testing the scroll viewer inside TabView itself. Unless we expose some APIs to scroll it, otherwise it is hard to manipulate the scrolling here. Clicking the increase/decrease button might be a way out but it is hard to know if we are reaching to the end.

marcelwgn · 2020-03-25T14:18:50Z

Btw, the NumberBox scroller trick does not apply here since the scroll viewer in NumberBox UI Test is external and owned by the test page. Here we are testing the scroll viewer inside TabView itself. Unless we expose some APIs to scroll it, otherwise it is hard to manipulate the scrolling here. Clicking the increase/decrease button might be a way out but it is hard to know if we are reaching to the end.

Oh I see. Maybe you can try to find the Scrollviewer in the VisualTree using the VisualTreeHelper.

StephenLPeters · 2020-03-25T20:55:51Z

dev/TabView/TabView.cpp

+        auto decreaseButton = SharedHelpers::FindInVisualTreeByName(scrollViewer, L"ScrollDecreaseButton").as<winrt::RepeatButton>();
+        auto increaseButton = SharedHelpers::FindInVisualTreeByName(scrollViewer, L"ScrollIncreaseButton").as<winrt::RepeatButton>();
+
+        auto minThreshold = 0.1;


auto minThreshold = 0.1; [](start = 8, length = 24)

constexpr

Would it make sense to inline the variable into the if statements if its constant, that is, just write 0.1 instead of minThreshold?

dev/TabView/TabView.cpp

StephenLPeters · 2020-03-25T20:58:24Z

dev/TabView/TabView.cpp

+        if (abs(scrollViewer.HorizontalOffset() - scrollViewer.ScrollableWidth()) < minThreshold)
+        {
+            decreaseButton.IsEnabled(true);
+            increaseButton.IsEnabled(false);


I think better than setting the properties from code behind would be to add visual states to the template and calling go to state

I am not sure if using VisualState is the way to go as I am not seeing much benefits here. We still need to do some calculation here to disable or enable it on the fly based on min threshold since it is a floating value. (Or we will have this magic number lives inside xaml?) On the other hand we need to make sure it works in all cases like Window resizing. Maybe we should rewrite this whole scrolling buttons thingy in TabView vNext? @chingucoding

I am not sure if with TabView vNext, a rewrite is a) a good idea and b) a rewrite would fix this at all. After all, we need to provide some sort of scrolling, and disabling those buttons is something we have to do. @stmoy FYI

StephenLPeters · 2020-03-25T20:59:36Z

dev/TabView/TabView.cpp

+
+        auto minThreshold = 0.1;
+
+        if (abs(scrollViewer.HorizontalOffset() - scrollViewer.ScrollableWidth()) < minThreshold)


if (abs(scrollViewer.HorizontalOffset() - scrollViewer.ScrollableWidth()) < minThreshold) [](start = 8, length = 89)

Is there a situation where both scroll buttons should be disabled, when the content is smaller than the viewport for instance.

If the content is smaller than the viewport, the ScrollViewer does not render the buttons at all, making disabling them unnecessary.

dev/TabView/InteractionTests/TabViewTests.cs

…ents Save decrease/increase button ref for future use and address few comments

Added UI tests

marcelwgn

Edit: This looks good to go.

marcelwgn · 2020-03-26T10:30:01Z

dev/TabView/InteractionTests/TabViewTests.cs

+                // Scroll all the way to the right and verify decrease/increase button visual state
+                FindElement.ByName<Button>("ScrollTabViewToTheRight").InvokeAndWait();
+                Verify.IsTrue(IsScrollDecreaseButtonEnabled(), "Scroll decrease button should be enabled");
+                Verify.IsFalse(IsScrollIncreaseButtonEnabled(), "Scroll increase button should be disabled");


This assert fails on my machine 100% of the time, however adding a small delay after line 154 (e.g. 300ms) seems to fix that issue. I am not sure if it's just my machine or if this is generally an issue. Did the test run perfectly on your machine @Jasonstein ?

Yes, it runs fine on my machine but I have only tested once. Let me check again.

You are right, it does not work if there is no delay (not sure why it works on my machine previously). Because the view state change triggered after scroll viewer view changed event so it is not going to happen right away. I added a delay after each invoke. Please check latest iteration.

Test looks fine now. Thanks for looking into this.

Can we do Wait.ForIdle instead ? Thread.Sleep /Wait for milliseconds should be avoided in general, they tend to make the test slower on fast machines and fail on really slow machines causing the test to be flaky. Wait.ForIdle is designed for this exact purpose - once the system is done with everything it will come out of the wait and that time will be less on faster machines.

There is some event in the WaitHelper.cs file which technically should work, though the fact that this exact class is letting us down here is not to promising. The easies/"safest" way would be to wait for a certain amount of time and retry. This is also something the WaitHelper does in some cases (I think).

Even with wait on ViewChanged, I am still smelling race condition here since "IsEnabled" is set after ViewChanged not as part of ViewChanged event. It is unlikely but in theory, it could happen. @ranjeshj Should we keep 300ms waiting here for now? Or I would suggest to move this test to a new method and name it flaky test or give it an attribute. At the end of the day, this is UI test (integration test), "flaky" is hard to avoid.

There are some unreliable tests, however we try to keep avoiding unreliable tests mostly. If waiting 300ms or 500ms or 1000ms makes it stable, that would be a possible solution, though it may create problems in the future.

@Jasonstein The tests are PR gates, so if tests are flaky it will start blocking future PRs. We should try as much as possible to avoid that. Please file a test issue to investigate this and to unblock this PR for now. If the test starts failing, then we can disable it and link it to the issue. Please also add a comment next to the delay pointing to the issue.

I tried to implement ViewChanged and even with that, it is not going to work. Then I realized that I thought I disabled scrolling animation, in fact, I did not. Now I have disabled all scrolling animation and things started to work and test is now passing with Wait.ForIdle.

Adding a sleep after each scrolling to make sure view state get updated

dev/TabView/InteractionTests/TabViewTests.cs

Use Wait.ForMilliseconds instead of Thread.Sleep

Use Wait.ForIdle() instead

ranjeshj · 2020-03-26T18:42:43Z

/azp run

azure-pipelines · 2020-03-26T18:42:53Z

Azure Pipelines successfully started running 1 pipeline(s).

Disable scrolling animation

ranjeshj · 2020-03-27T12:18:18Z

/azp run

azure-pipelines · 2020-03-27T12:18:30Z

Azure Pipelines successfully started running 1 pipeline(s).

dev/TabView/InteractionTests/TabViewTests.cs

Remove unused namespace

ranjeshj · 2020-03-27T12:42:24Z

/azp run

azure-pipelines · 2020-03-27T12:42:52Z

Azure Pipelines successfully started running 1 pipeline(s).

marcelwgn

Looks good to go!

dev/TabView/TabView.cpp

StephenLPeters

Resolve comments

ranjeshj · 2020-04-01T12:17:23Z

/azp run

azure-pipelines · 2020-04-01T12:17:34Z

Azure Pipelines successfully started running 1 pipeline(s).

ranjeshj · 2020-04-03T14:08:13Z

@Jasonstein can you please merge with master which includes some stability fixes for the PR pipeline ?

0x7c13 · 2020-04-03T14:51:28Z

@Jasonstein can you please merge with master which includes some stability fixes for the PR pipeline ?

Done, can you help trigger another CI build?

ranjeshj · 2020-04-03T15:08:49Z

/azp run

azure-pipelines · 2020-04-03T15:09:17Z

Azure Pipelines successfully started running 1 pipeline(s).

StephenLPeters

Disable TabView scroll buttons based on scroll position

7120fa9

Disable TabView scroll buttons based on scroll position

msft-github-bot added the needs-triage Issue needs to be triaged by the area owners label Mar 25, 2020

0x7c13 mentioned this pull request Mar 25, 2020

Tab strip scroll bumpers don't disable when scrolling to the edge #2143

Closed

ranjeshj requested a review from teaP March 25, 2020 12:22

ranjeshj added area-TabView team-Controls Issue for the Controls team labels Mar 25, 2020

ranjeshj reviewed Mar 25, 2020

View reviewed changes

dev/TabView/TabView.cpp Outdated Show resolved Hide resolved

Store the return value of scrollViewer.ViewChanged in a revoker

b52f6bb

Store the return value of scrollViewer.ViewChanged in a revoker

marcelwgn reviewed Mar 25, 2020

View reviewed changes

Setup UI for testing scroll viewer's increase/decrease button

1f0c63d

Setup UI for testing scroll viewer's increase/decrease button

StephenLPeters removed the needs-triage Issue needs to be triaged by the area owners label Mar 25, 2020

StephenLPeters reviewed Mar 25, 2020

View reviewed changes

dev/TabView/TabView.cpp Outdated Show resolved Hide resolved

StephenLPeters reviewed Mar 25, 2020

View reviewed changes

dev/TabView/InteractionTests/TabViewTests.cs Show resolved Hide resolved

0x7c13 changed the title ~~Disable TabView scroll buttons based on scroll position~~ [WIP] Disable TabView scroll buttons based on scroll position Mar 25, 2020

0x7c13 added 2 commits March 25, 2020 16:12

Save decrease/increase button ref for future use and address few comm…

696f0c5

…ents Save decrease/increase button ref for future use and address few comments

Added UI tests

ac59d56

Added UI tests

0x7c13 changed the title ~~[WIP] Disable TabView scroll buttons based on scroll position~~ Disable TabView scroll buttons based on scroll position Mar 26, 2020

marcelwgn approved these changes Mar 26, 2020

View reviewed changes

Adding a sleep after each scrolling to make sure view state get updated

e08c75c

Adding a sleep after each scrolling to make sure view state get updated

marcelwgn reviewed Mar 26, 2020

View reviewed changes

dev/TabView/InteractionTests/TabViewTests.cs Outdated Show resolved Hide resolved

0x7c13 added 2 commits March 26, 2020 05:09

Use Wait.ForMilliseconds instead of Thread.Sleep

647eaf7

Use Wait.ForMilliseconds instead of Thread.Sleep

Use Wait.ForIdle() instead

bb448b5

Use Wait.ForIdle() instead

Disable scrolling animation

bac6872

Disable scrolling animation

ranjeshj reviewed Mar 27, 2020

View reviewed changes

dev/TabView/InteractionTests/TabViewTests.cs Outdated Show resolved Hide resolved

Remove unused namespace

0eea6e9

Remove unused namespace

marcelwgn approved these changes Mar 27, 2020

View reviewed changes

marcelwgn mentioned this pull request Mar 29, 2020

ProcessHelixFiles job is unreliable (PR pipeline) #2187

Closed

StephenLPeters reviewed Mar 31, 2020

View reviewed changes

dev/TabView/TabView.cpp Outdated Show resolved Hide resolved

StephenLPeters approved these changes Mar 31, 2020

View reviewed changes

Resolve comments

1968401

Resolve comments

Merge remote-tracking branch 'upstream/master' into dev/jackil/tabview

2bfc114

StephenLPeters approved these changes Apr 3, 2020

View reviewed changes

StephenLPeters merged commit 68abde4 into microsoft:master Apr 3, 2020

		auto decreaseButton = SharedHelpers::FindInVisualTreeByName(scrollViewer, L"ScrollDecreaseButton").as<winrt::RepeatButton>();
		auto increaseButton = SharedHelpers::FindInVisualTreeByName(scrollViewer, L"ScrollIncreaseButton").as<winrt::RepeatButton>();


		auto minThreshold = 0.1;

		if (abs(scrollViewer.HorizontalOffset() - scrollViewer.ScrollableWidth()) < minThreshold)

Disable TabView scroll buttons based on scroll position #2162

Disable TabView scroll buttons based on scroll position #2162

Conversation

0x7c13 commented Mar 25, 2020 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0x7c13 Mar 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcelwgn commented Mar 25, 2020

0x7c13 commented Mar 25, 2020 • edited Loading

marcelwgn commented Mar 25, 2020

0x7c13 commented Mar 25, 2020

marcelwgn commented Mar 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcelwgn left a comment • edited Loading

Choose a reason for hiding this comment

marcelwgn Mar 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcelwgn Mar 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0x7c13 Mar 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ranjeshj commented Mar 26, 2020

azure-pipelines bot commented Mar 26, 2020

ranjeshj commented Mar 27, 2020

azure-pipelines bot commented Mar 27, 2020

ranjeshj commented Mar 27, 2020

azure-pipelines bot commented Mar 27, 2020

marcelwgn left a comment

Choose a reason for hiding this comment

StephenLPeters left a comment

Choose a reason for hiding this comment

ranjeshj commented Apr 1, 2020

azure-pipelines bot commented Apr 1, 2020

ranjeshj commented Apr 3, 2020

0x7c13 commented Apr 3, 2020

ranjeshj commented Apr 3, 2020

azure-pipelines bot commented Apr 3, 2020

StephenLPeters left a comment

Choose a reason for hiding this comment

0x7c13 commented Mar 25, 2020 •

edited

Loading

0x7c13 Mar 25, 2020 •

edited

Loading

0x7c13 commented Mar 25, 2020 •

edited

Loading

marcelwgn left a comment •

edited

Loading

marcelwgn Mar 26, 2020 •

edited

Loading

marcelwgn Mar 26, 2020 •

edited

Loading

0x7c13 Mar 26, 2020 •

edited

Loading