Skip to content

Support Overflow::Scroll#446

Merged
alice-i-cecile merged 17 commits intoDioxusLabs:mainfrom
nicoburns:feat/overflow-scroll
Apr 23, 2023
Merged

Support Overflow::Scroll#446
alice-i-cecile merged 17 commits intoDioxusLabs:mainfrom
nicoburns:feat/overflow-scroll

Conversation

@nicoburns
Copy link
Copy Markdown
Collaborator

@nicoburns nicoburns commented Apr 16, 2023 •
edited
Loading

Objective

  • To make it easier to use Taffy for UI trees containing scrollviews on systems that have scrollbars that take up space in the layout.
  • To support layouts containing overflow: scroll for those wanting to faithfully render web layouts.

Tasks

  • Add style types
  • Implement for leaf algorithm
  • Implement for flexbox algorithm
  • Implement for grid algorithm

Context

Feedback wanted

  • scrollbar_width deviates from the spec. The spec only allow auto, thin or none and the user agent must decide what actual widths these correspond to. Whereas I've allowed a pixel value to be specified. I think this difference is justified because Taffy is aimed at "user agents" rather than end users.
  • I've made scrollbar_width a u8 to save on space. Does this seem reasonable, or do you think I should make it an f32 like everything else? scrollbar_width has been converted to f32 in response to feedback.
  • I've used a single scrollbar_width for both axis (the overflow property still applies separately in each axis). So IF there is a scrollbar in both axis then they must both be the same size, does this seem reasonable.

@nicoburns nicoburns added enhancement New feature or request breaking-change A change that breaks our public interface labels Apr 16, 2023
@alice-i-cecile
Copy link
Copy Markdown
Collaborator

alice-i-cecile commented Apr 16, 2023

scrollbar_width deviates from the spec. The spec only allow auto, thin or none and the user agent must decide what actual widths these correspond to. Whereas I've allowed a pixel value to be specified. I think this difference is justified because Taffy is aimed at "user agents" rather than end users.

I agree: that's an unhelpful spec setup.

I've made scrollbar_width a u8 to save on space. Does this seem reasonable, or do you think I should make it an f32 like everything else?

I think we should probably be consistent rather than clever here. I can't imagine this making a performance difference, and fractional sizes are reasonable.

I've used a single scrollbar_width for both axis (the overflow property still applies separately in each axis). So IF there is a scrollbar in both axis then they must both be the same size, does this seem reasonable.

That seems sensible: uneven scroll bars definitely seem wrong.

@nicoburns nicoburns force-pushed the feat/overflow-scroll branch from 2fbd91c to b56f22f Compare April 18, 2023 18:28
@nicoburns nicoburns force-pushed the feat/overflow-scroll branch from b56f22f to bffafc7 Compare April 23, 2023 19:39
@nicoburns nicoburns force-pushed the feat/overflow-scroll branch from bffafc7 to 95407ec Compare April 23, 2023 20:18
@nicoburns nicoburns marked this pull request as ready for review April 23, 2023 21:47
@nicoburns nicoburns changed the title WIP: Support Overflow::Scroll Support Overflow::Scroll Apr 23, 2023
alice-i-cecile
alice-i-cecile approved these changes Apr 23, 2023
View reviewed changes
Copy link
Copy Markdown
Collaborator

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Good docs, clean code. Great tests as always.

@alice-i-cecile alice-i-cecile merged commit daf4164 into DioxusLabs:main Apr 23, 2023
@nicoburns nicoburns mentioned this pull request Apr 27, 2023
Closed
16 tasks
@nicoburns nicoburns mentioned this pull request Jun 2, 2023
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

@Weibye Weibye Awaiting requested review from Weibye

@TimJentzsch TimJentzsch Awaiting requested review from TimJentzsch

Assignees

No one assigned

Labels

breaking-change A change that breaks our public interface enhancement New feature or request

Projects

None yet

Milestone

0.4

Development

Successfully merging this pull request may close these issues.

2 participants

@nicoburns @alice-i-cecile