Skip to content

Add ScrollContainer widget to the Toga web backend#3362

Merged
freakboy3742 merged 7 commits intobeeware:mainfrom
kavi2du:scrollcontainerweb
May 2, 2025
Merged

Add ScrollContainer widget to the Toga web backend#3362
freakboy3742 merged 7 commits intobeeware:mainfrom
kavi2du:scrollcontainerweb

Conversation

@kavi2du
Copy link
Contributor

@kavi2du kavi2du commented Apr 27, 2025

Adds support for the ScrollContainer widget on Toga web platform. Changes have also been made to the documentation to reflect this.

The scroll container, which was previously unsupported on web platforms, is now functional on the web.

Refs #3334

PR Checklist:

  • [ x ] All new features have been tested
  • [ x ] All new features have been documented
  • [ x ] I have read the CONTRIBUTING.md file
  • [ x ] I will abide by the code of conduct

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The broad strokes of this look about what I'd expect; however, I'm intrigued to know how you've tested this. There obviously aren't any automated tests, and the PR doesn't describe any specific testing regimen. My usual testing approach is to open the example app that matches the widget being implemented - in this case, examples/scrollcontainer - and.... it definitely doesn't work right.

Some of that might be because of the changes that #3345 is introducing, because there's lots of console errors talking about dangling proxies - but as a result, the example app doesn't behave as it does on a desktop platform.

Part of the complication is also caused by the fact that a web page is inherently scrollable - so the "flexible" height of a scroll container used as main content is effectively infinite - so vertical scrolling isn't needed.

So - how did you verify that this actually works?

@mEp3ii2 mEp3ii2 mentioned this pull request Apr 28, 2025
15 tasks
@kavi2du
Copy link
Contributor Author

kavi2du commented Apr 28, 2025

hi @freakboy3742, thanks for the feedback. I also followed a similar manual testing approach to what you described. The app.py example that was in the examples/scrollcontainer directory opened in the web browser when i ran it and yes, the content wasn’t displayed correctly.

So, I modified the example app.py file to check horizontal and vertical scrolling of the scroll container. After testing, I reverted those changes and restored app.py back to its original state. Should I just leave the edited app.py as it is, or is there a different approach you'd like me to follow?

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for that detail about the testing regimen - it looks like the scrollcontainer example is getting caught on #3345 and related issues; if I manually set the height and the horizontal scrollability of the scroll container, the scroll part of the demo seems to work as expected; and the general shape of the handlers looks like it would be right, if only handlers worked :-)

There's definitely a bigger issue to resolve over how to handle height allocations on web widgets - whereas a normal app window has a finite height, a web page has an effectively infinite height. That will cause problems if anyone writes a web page where the widgets are intended to be floated to the bottom of the page. However, that's not an issue we need to resolve here. I'll open a separate ticket to track this one.

I've flagged a minor tweak to the release notes; I'll hold of merging this until #3345 is resolved and we can verify that the event handlers are working as expected.

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With #3345 resolved, I've been able to test this; there were 2 lingering problem:

  • The need to use create_proxy() when registering the event listener, and
  • The way the on_scroll event was being handled.

I've corrected those issues, and the scrollcontainer demo now works as expected.

I've also taken this opportunity to clean up the other remaining event registrations to ensure they're all using proxies correctly.

Thanks for the PR; hopefully we're on the right track now with adding new widgets.

@freakboy3742 freakboy3742 merged commit 8085d7a into beeware:main May 2, 2025
52 checks passed
@kavi2du kavi2du deleted the scrollcontainerweb branch May 2, 2025 05:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants