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

Scrolling performance in Files is really bad on iOS #22347

Closed
oparoz opened this issue Feb 12, 2016 · 17 comments
Closed

Scrolling performance in Files is really bad on iOS #22347

oparoz opened this issue Feb 12, 2016 · 17 comments

Comments

@oparoz
Copy link
Contributor

oparoz commented Feb 12, 2016

8.2 8.1? introduced a severe performance regression on mobile. I'm not sure exactly which change has created the problem, perhaps the sidebar, but if you scroll through files on mobile, it's very ugly.
There has been an attempt to fix the situation, but it broke things even more on other platforms.

The problem has been solved in Gallery and you can compare scrolling performance by switching between the 2 views to understand the problem.
A similar fix could be applied to files, but it involves modifying the structure of the templates in order to take the fixed controls bar outside of the scrolling div and may not be the best solution for all apps.

Diagnostics

8.2

Files/logged-in: slow
Files/public: slow
Gallery/logged-in: was slow, had to change the structure
Gallery/public: was slow

8.1

Files/logged-in: slow. Has controls, navigation panel
Files/public: smooth. Has controls
Gallery/logged-in: slow. Has controls
Gallery/public: slow. Has controls

@jancborchardt @Henni @PVince81 @MorrisJobke @rullzer

@PVince81 PVince81 added this to the 8.2.3-current-maintenance milestone Feb 12, 2016
@PVince81
Copy link
Contributor

Would be good to bisect that to find out the cause.

@oparoz oparoz changed the title Scrolling performance is really bad on mobile Scrolling performance in Files is really bad on mobile Feb 12, 2016
@oparoz
Copy link
Contributor Author

oparoz commented Feb 12, 2016

I've had a look at the template and it hasn't changed for 8.2, so it must come from the CSS and it has to be fixed app by app as controls is not in the base template.

@jancborchardt
Copy link
Member

So you’re saying the fixed controls bar could be a cause? Does it work fine in the Mail app and others which don’t have that bar?

@oparoz
Copy link
Contributor Author

oparoz commented Feb 12, 2016

It's more about the choice of scrollable element.
On 8.1, on the public page, scrolling is smooth, but the structure is different. the scrollable element is outside the app, including the header. The fixed elements are on top of the scrollable element or that scrollable element is maybe not directly above the fixed element? I'm not sure because I can't find which element is scrolling.

8 1_scroll

On 8.2, you have the new structure (aligned with the logged-in side), which is better visually, but which kills acceleration, because the fixed elements are inside the scrollable element.

8 2_scroll

If you move controls outside of the content, then you get to keep the scrollbar "inside", but you have to rewrite some CSS to make it work with the navigation panel and maybe the sidebar.

@oparoz
Copy link
Contributor Author

oparoz commented Feb 12, 2016

So,

<scrollable>
    <fixed></fixed>
    <content></content>
</scrollable>

BAD

but

<fixed></fixed>
<scrollable>
    <content></content>
</scrollable>

GOOD

@oparoz
Copy link
Contributor Author

oparoz commented Feb 14, 2016

So any fixed element inside #app-content is going crazy if -webkit-overflow-scrolling: touch is used. That was the reason the fix mentioned in the OP had to be reverted. So if you ever want mobile performance back, fixed elements such as #controls or #app-sidebar have to be moved outside or the scrolling action has to be moved to a sibling. In the example above that means making #content scrollable and turning #scrollable into a container.

@ghost ghost modified the milestones: 8.2.4-next-maintenance, 8.2.3-current-maintenance Feb 23, 2016
@ghost ghost assigned rperezb Mar 9, 2016
@ghost
Copy link

ghost commented Mar 9, 2016

@rperezb can you have someone in mobile have a look?

@PVince81
Copy link
Contributor

The problem here is that we already changed the container structure and changing it again, especially in a backport, is likely to cause more regressions. I'd rather move this to 9.1 which is another opportunity to reconsider the container layouts.

Moving to 9.1 CC @cmonteroluque

@PVince81 PVince81 modified the milestones: 9.1-current, 8.2.4-current-maintenance Apr 19, 2016
@oparoz
Copy link
Contributor Author

oparoz commented Apr 19, 2016

Approved

@ghost
Copy link

ghost commented Apr 19, 2016

I agree. CC @MTRichards as a heads up

@MTRichards
Copy link
Contributor

As long as we actually cover it, sounds like a plan for 9.1.

@PVince81
Copy link
Contributor

@MTRichards @cmonteroluque to clarify, this is about the web UI on mobile, not mobile apps.

@MTRichards
Copy link
Contributor

MTRichards commented Apr 20, 2016

Yes, understood. thx.

@ghost
Copy link

ghost commented Apr 20, 2016

right

@skjnldsv
Copy link
Contributor

skjnldsv commented May 6, 2016

I worked on a pr on the mail app this week and did exactly the same stuff.
Using fixed div are ok but only out of the page flow, like @oparoz said.

Currently we have a sidebar with the settings button fixed on the bottom.
I think we should change this structure, and put the scroll on the ul part.

<div id="app-navigation">
    <fixed-top element></fixed-top element> 
    <ul>this is the list we usually have</ul>         <--- scroll
    <fixed-bottom element></fixed-bottom element>
</div>

To do that we can also drop the float left of the #app-navigation to replace it with some beautiful display:flex. And for the scoll part of the ul, we should use the css function calc which work perfectly: height: calc(100% - fixedelementheight)

Refs: owncloud-archive/mail#1476

@oparoz oparoz changed the title Scrolling performance in Files is really bad on mobile Scrolling performance in Files is really bad on iOS May 17, 2016
@PVince81 PVince81 modified the milestones: 9.1-current, 9.1.1-next-maintenance Jun 30, 2016
@PVince81 PVince81 modified the milestones: 9.1.2, 9.1.1 Sep 21, 2016
@PVince81 PVince81 modified the milestones: 9.1.3, 9.1.2 Oct 20, 2016
@PVince81 PVince81 modified the milestones: 9.2, 9.1.3 Nov 30, 2016
@PVince81 PVince81 modified the milestones: backlog, 10.0 Apr 6, 2017
@ownclouders
Copy link
Contributor

Hey, this issue has been closed because the label status/STALE is set and there were no updates for 7 days. Feel free to reopen this issue if you deem it appropriate.

(This is an automated comment from GitMate.io.)

@lock
Copy link

lock bot commented Jul 31, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jul 31, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants