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

scrollWindow: fix view scrolling #9869

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

meven
Copy link
Contributor

@meven meven commented Aug 21, 2024

Change-Id: I54990645d17e1e352397f1d57b4cc671af6b5839

Summary

  • make scrollWindow height fix
  • special case data.horizontal.page_size == 0
  • improve computation of rowHeight
  • don't resize dynamically dimensions based on dom changes

Checklist

  • I have run make prettier-write and formatted the code.
  • All commits have Change-Id
  • I have run tests with make check
  • I have issued make run and manually verified that everything looks okay
  • Documentation (manuals or wiki) has been updated or is not required

@meven meven requested a review from eszkadev August 21, 2024 13:24
@meven meven force-pushed the meven/fix-scrollWindow-issue-9770 branch from a1daa0f to 2f28839 Compare August 21, 2024 16:39
@eszkadev
Copy link
Contributor

eszkadev commented Aug 22, 2024

It works nice in most of the cases, but still has problems with vertical scrolling in "managed by user" mode.

symbols-2024-08-22_10.36.24.webm

It worked in the past before regression commit, it was also not doing "scrolling animation" so UI looked less buggy/jumping.

Copy link
Contributor

@eszkadev eszkadev left a comment

Choose a reason for hiding this comment

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

please try to fix vertical scrolling

@meven
Copy link
Contributor Author

meven commented Aug 22, 2024

The use cases to test:

In calc:

  • function wizard, select AGGREGATE and try to scroll
  • insert symbol
  • properties > custom properties tab.

 * make scrollWindow height fix
 * special case data.horizontal.page_size == 0
 * improve computation of rowHeight
 * don't resize dynamically dimensions based on dom changes

Fixes CollaboraOnline#9770

Signed-off-by: Méven Car <[email protected]>
Change-Id: I54990645d17e1e352397f1d57b4cc671af6b5839
Signed-off-by: Méven Car <[email protected]>
Change-Id: I8bc67ecf11308f35791c5d6c0c4557f20505db86
@eszkadev
Copy link
Contributor

This works bad:
scroll-2024-08-30_08.54.08.webm

I used keyboard (arrow down) in symbol dialog.

Please see how it works with the reverted version (current master):
scroll-2024-08-30_08.57.03.webm

Can we just setup the size of scroll window in the file properties dialog for custom properties using CSS?
I think behavior now is ok, only look of custom properties tab is not good.

@meven
Copy link
Contributor Author

meven commented Sep 2, 2024

Can we just setup the size of scroll window in the file properties dialog for custom properties using CSS?

The content size is dynamic in the custom properties and function wizard, there it is not possible and in the symbol dialog it would break when we add new symbols.

@meven
Copy link
Contributor Author

meven commented Sep 2, 2024

This works bad: scroll-2024-08-30_08.54.08.webm

I used keyboard (arrow down) in symbol dialog.

I noticed it, I have an idea how to fix that.

@eszkadev
Copy link
Contributor

eszkadev commented Sep 2, 2024

Can we just setup the size of scroll window in the file properties dialog for custom properties using CSS?

The content size is dynamic in the custom properties and function wizard, there it is not possible and in the symbol dialog it would break when we add new symbols.

@meven with current master (with revert) it works well in all of these dialogs apart custom properties.

Really isn't it enough to fix custom properties sizing on top of current master ?
Like some CSS: to make scroll-window height depending on height of the dialog and rest of the items static.

@meven
Copy link
Contributor Author

meven commented Sep 3, 2024

Really isn't it enough to fix custom properties sizing on top of current master ?
Like some CSS: to make scroll-window height depending on height of the dialog and rest of the items static.

My patches sets the scroll-window "view" height, in browser/css/jsdialogs.css.
But that's the content-height that's dynamic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

Calc: Function Wizard: broken UI
3 participants