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

proposed fix for bug 873574 #10336

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@arasbm
Contributor

arasbm commented Jun 12, 2013

  • adds indicators to top and bottom of the list to show that there is more content to scroll toward
  • besides the code, I think UX should review this interaction and visuals. Please let me know if you want to see video of it
@arasbm

This comment has been minimized.

Show comment
Hide comment
@arasbm

arasbm Jun 17, 2013

Contributor

Based on the feedback I got on the bug report, I changed the gradient color to black so it is more subtle now. I have attached an image showing various different states and corresponding visual changes. Please let me know if this is good to go, or I need to make other changes. Thanks!

alarms_scroll_second_try

Contributor

arasbm commented Jun 17, 2013

Based on the feedback I got on the bug report, I changed the gradient color to black so it is more subtle now. I have attached an image showing various different states and corresponding visual changes. Please let me know if this is good to go, or I need to make other changes. Thanks!

alarms_scroll_second_try

@jrburke

View changes

Show outdated Hide outdated apps/clock/js/alarm.js Outdated

@arasbm arasbm closed this Jun 21, 2013

@arasbm arasbm reopened this Jun 21, 2013

@arasbm

This comment has been minimized.

Show comment
Hide comment
@arasbm

arasbm Jun 21, 2013

Contributor

@jrburke It make sense to use AlarmList object in the scope. I will try the make this change and update the PR. And will not to accidentally close/reopen it this time. Thanks for your comments!

Contributor

arasbm commented Jun 21, 2013

@jrburke It make sense to use AlarmList object in the scope. I will try the make this change and update the PR. And will not to accidentally close/reopen it this time. Thanks for your comments!

@arasbm

This comment has been minimized.

Show comment
Hide comment
@arasbm

arasbm Jun 24, 2013

Contributor

I have updated this PR based on the feedback I got above. Please let me know if there are any issues, or if you need any more information or video etc. Thanks!

Contributor

arasbm commented Jun 24, 2013

I have updated this PR based on the feedback I got above. Please let me know if there are any issues, or if you need any more information or video etc. Thanks!

@arasbm

View changes

Show outdated Hide outdated apps/clock/style/alarm.css Outdated
@ian-liu

This comment has been minimized.

Show comment
Hide comment
@ian-liu

ian-liu Aug 8, 2013

Contributor

@arasbm
The improving UX is pretty cool for me. A user is able to know what boundary of the index is located via the gradient. But I'm wondering the lower frame rate while alarm list is scrolling. Especially, alarm list is scrolled to the top and bottom. We could see jitter frame while applying CSS style for alarm list.

My suggestion is as following:

  • Have some criteria to decrease the computing time ''showHideScrollIndicators()'' while receive scroll event
  • Apply CSS style while alarm list scroll completely
Contributor

ian-liu commented Aug 8, 2013

@arasbm
The improving UX is pretty cool for me. A user is able to know what boundary of the index is located via the gradient. But I'm wondering the lower frame rate while alarm list is scrolling. Especially, alarm list is scrolled to the top and bottom. We could see jitter frame while applying CSS style for alarm list.

My suggestion is as following:

  • Have some criteria to decrease the computing time ''showHideScrollIndicators()'' while receive scroll event
  • Apply CSS style while alarm list scroll completely
@arasbm

This comment has been minimized.

Show comment
Hide comment
@arasbm

arasbm Aug 12, 2013

Contributor

@ian-liu I have made a small change in an attempt to improve performance of showHIdeScrollIndicators. It now checks to see if it is necessary before attempting to modify CSS. I think it is a bit faster now, but it is hard to tell. I test this patch using a keon and I can not see any jitters. When I enable the FPS in the developer tools I can see FPS may drop to around 30 when scrolling but there is no visible difference.

I thought about your second suggestion which if I understand correctly mean limiting the checks for applying the indicators to top and bottom of the list, but I think there is a the danger there for breaking this feature. It is hard to predict scrolling behaviour on various future devices, so I think it is best to check every time a scroll event is received -- of course as long as this does not introduce performance problems.

Please take a look again and let me know if this version is acceptable, or if you have some other advice for improving the performance. Thanks!

Contributor

arasbm commented Aug 12, 2013

@ian-liu I have made a small change in an attempt to improve performance of showHIdeScrollIndicators. It now checks to see if it is necessary before attempting to modify CSS. I think it is a bit faster now, but it is hard to tell. I test this patch using a keon and I can not see any jitters. When I enable the FPS in the developer tools I can see FPS may drop to around 30 when scrolling but there is no visible difference.

I thought about your second suggestion which if I understand correctly mean limiting the checks for applying the indicators to top and bottom of the list, but I think there is a the danger there for breaking this feature. It is hard to predict scrolling behaviour on various future devices, so I think it is best to check every time a scroll event is received -- of course as long as this does not introduce performance problems.

Please take a look again and let me know if this version is acceptable, or if you have some other advice for improving the performance. Thanks!

arasbm added some commits Aug 20, 2013

performance improvements for alarm list scroll indicators
 * limit execution of `showHideScrollIndicators` to one per 150ms
 * set threshold for showing and hiding indicators relative to alarm cell height
 * remove animation as it is not needed with the new threshold
@arasbm

This comment has been minimized.

Show comment
Hide comment
@arasbm

arasbm Aug 20, 2013

Contributor

@ian-liu Based on discussion with @jrburke I have added a commit to this PR with changes to address the concerns about performance of scrolling in the alarm list. I left the commit out to make it easier to see the changes. If you like, I can also rebase the PR into one commit. I look forward to hear back from you. Thanks!

Contributor

arasbm commented Aug 20, 2013

@ian-liu Based on discussion with @jrburke I have added a commit to this PR with changes to address the concerns about performance of scrolling in the alarm list. I left the commit out to make it easier to see the changes. If you like, I can also rebase the PR into one commit. I look forward to hear back from you. Thanks!

@ian-liu

This comment has been minimized.

Show comment
Hide comment
@ian-liu

ian-liu Dec 16, 2013

Contributor

@arasbm
Even if we use a flag and setTimeout to avoid computing times, we still could see the frame rate lower than before. I think we run in showHIdeScrollIndicators function during scrolling element. It will make the frame rate lower:(. But I still can not find out a good way to give you suggestion. This is a good topic to study advanced implementation for the rendering layout dynamically. Really thanks for your contribution. Since the bug is closed invalid, I leave my reviewing flag here.

Contributor

ian-liu commented Dec 16, 2013

@arasbm
Even if we use a flag and setTimeout to avoid computing times, we still could see the frame rate lower than before. I think we run in showHIdeScrollIndicators function during scrolling element. It will make the frame rate lower:(. But I still can not find out a good way to give you suggestion. This is a good topic to study advanced implementation for the rendering layout dynamically. Really thanks for your contribution. Since the bug is closed invalid, I leave my reviewing flag here.

@mozilla-autolander-deprecated

This comment has been minimized.

Show comment
Hide comment
@mozilla-autolander-deprecated

mozilla-autolander-deprecated Oct 1, 2014

Contributor

This pull request has been closed due to tree stability issues. Please rebase and re-open the pull request if you still need to land this. Ensure the gaia-try run is green before landing. Sorry for any inconvenience.

Contributor

mozilla-autolander-deprecated commented Oct 1, 2014

This pull request has been closed due to tree stability issues. Please rebase and re-open the pull request if you still need to land this. Ensure the gaia-try run is green before landing. Sorry for any inconvenience.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment