scrollToWidget called continually

[edherbert]

I discovered this issue when trying to create a scrollable window which contained only non keyboard focusable widgets (labels). In this case, ColibriManager.cpp:1531 resets m_keyboardFocusedPair.widget to 0 each frame, and then the block below beginning at 1540 notices that the widget is missing so tries to query the default widget from the window and set it. If the widget returned happens to be non keyboard navigable, then it gets setup, scrolled to, but the next frame it is reset again and the cycle continues, meaning scrollToWidget is constantly called each frame. For me this effectively disabled scrolling on this window as any time you scroll, it scrolled back to the top automatically to the default widget.

My solution to this was to add a check for keyboard focusability

diff --git a/src/ColibriGui/ColibriManager.cpp b/src/ColibriGui/ColibriManager.cpp
index 64a5e69..7d44887 100644
--- a/src/ColibriGui/ColibriManager.cpp
+++ b/src/ColibriGui/ColibriManager.cpp
@@ -1538,7 +1538,7 @@ namespace Colibri
 		if( m_keyboardFocusedPair.window && !m_keyboardFocusedPair.widget )
 		{
 			m_keyboardFocusedPair.widget = m_keyboardFocusedPair.window->getDefaultWidget();
-			if( m_keyboardFocusedPair.widget )
+			if( m_keyboardFocusedPair.widget && m_keyboardFocusedPair.widget->isKeyboardNavigable() )
 			{
 				m_keyboardFocusedPair.widget->setState( States::HighlightedButton );
 				callActionListeners( m_keyboardFocusedPair.widget, Action::Highlighted );

However on closer inspection I see that this doesn't break the cycle and the getDefaultWidget function would be called each frame, which is just a pointless for loop.

[darksylinc]

I can see the logic you speak of.

I'm still surprised & confused on why I never came into this problem (AFAIK I do have scrollable windows with all non-keyboard-naviagable widgets). It seems like the conditions to trigger this bug are even more restrictive than what you mention?

[darksylinc]

Also: I don't see a way to prevent the pointless loop every frame you speak of when using your solution. It seems like a corner case we just have to ignore every frame, for which taking care of it would consume a lot of code (which means lots of potential bugs), time and resources (it doesn't seem to be free to track which windows are entirely composed of non-keyboard-navigable widgets)

[edherbert]

The scenario for me was that I had a completely blank screen, where I then created a window and then a window child to contain the scrolling. This was for an inventory screen, so you can see the title inventory above in the parent window, with the scrollable content below. I've reproduced this in the demo project and it does show the issue.

diff --git a/src/ColibriGuiGameState.cpp b/src/ColibriGuiGameState.cpp
index 895a05c..bc6e104 100644
--- a/src/ColibriGuiGameState.cpp
+++ b/src/ColibriGuiGameState.cpp
@@ -116,7 +116,11 @@ namespace Demo
 
 		// colibriManager->setTouchOnlyMode( true );
 
-		fullWindow = colibriManager->createWindow( 0 );
+        Colibri::Window* parentWindow = colibriManager->createWindow( 0 );
+        parentWindow->setSize(Ogre::Vector2(500, 500));
+        parentWindow->setTopLeft(Ogre::Vector2(500, 400));
+
+ 		fullWindow = colibriManager->createWindow( 0 );
 		mainWindow = colibriManager->createWindow( 0 );
 		//mainWindow->setVisualsEnabled( false );
 		vertWindow = colibriManager->createWindow( 0 );
@@ -443,6 +447,28 @@ namespace Demo
 #endif
 		mainWindow->sizeScrollToFit();
 
+
+        Colibri::Window* testWindow = colibriManager->createWindow( parentWindow );
+        testWindow->setSize(Ogre::Vector2(400, 400));
+
+        Colibri::LayoutLine* testLayoutLine = new Colibri::LayoutLine( colibriManager );
+        for(int i = 0; i < 100; i++){
+            Colibri::Label* label = colibriManager->createWidget<Colibri::Label>( testWindow );
+            label->setText("Item " + std::to_string(i));
+            label->sizeToFit();
+            //label->setKeyboardFocus();
+
+            testLayoutLine->addCell(label);
+        }
+        testLayoutLine->layout();
+        testWindow->sizeScrollToFit();
+
+        parentWindow->setZOrder(255);
+
+        //layout->addCell(testWindow);
+
+
+
 		TutorialGameState::createScene01();
 	}
 	//-----------------------------------------------------------------------------------

Create the parent window first, so it's registered as the keyboardFocused window in ColibriManager.cpp:812 and then create the child within that. It sees that the parent is the focused window and re-assigns the new window to be focused ColibriManager.cpp:812. When it's then focused the issue will occur. If you change the labels to be buttons it scrolls fine.

I agree that this is a corner case, but I do still wonder why objects that are not keyboard navigable can be set as keyboardFocusable, for instance in this patch

diff --git a/src/ColibriGuiGameState.cpp b/src/ColibriGuiGameState.cpp
index 895a05c..25ab364 100644
--- a/src/ColibriGuiGameState.cpp
+++ b/src/ColibriGuiGameState.cpp
@@ -121,6 +121,21 @@ namespace Demo
 		//mainWindow->setVisualsEnabled( false );
 		vertWindow = colibriManager->createWindow( 0 );
 
+        Colibri::Window* testWindow = colibriManager->createWindow( 0 );
+        testWindow->setSize(Ogre::Vector2(500, 500));
+        testWindow->setTopLeft(Ogre::Vector2(200, 300));
+        Colibri::LayoutLine* testLayoutLine = new Colibri::LayoutLine( colibriManager );
+        for(int i = 0; i < 100; i++){
+            Colibri::Label* label = colibriManager->createWidget<Colibri::Label>( testWindow );
+            label->setText("Item " + std::to_string(i));
+            label->sizeToFit();
+            label->setKeyboardFocus();
+
+            testLayoutLine->addCell(label);
+        }
+        testLayoutLine->layout();
+        testWindow->sizeScrollToFit();
+
 		//Ogre::Hlms *hlms = mGraphicsSystem->getRoot()->getHlmsManager()->getHlms( Ogre::HLMS_UNLIT );
 		//mainWindow->setDatablock( hlms->getDefaultDatablock() );
 

I just call setKeyboardFocus on a widget that has m_keyboardNavigable set as false and it's just allowed to steal the focus (and cause the same issue to occur).

I agree with what you're saying about tracking the keyboard focusability per window. It will be difficult, and this does look like an edge case to me.