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

Make possible for apps to disable the navigation bar slide gesture #7526

Conversation

danxuliu
Copy link
Member

On narrow screens a slide gesture can be used to open or close the navigation bar. However that gesture could conflict at times with the gestures used by certain apps (for example, if the right sidebar is open the user may expect to close it by dragging it to the right, but that could open the navigation bar instead depending on how the events are handled). This pull request makes possible for apps to disallow and allow again that slide gesture.

This is a prerequisite to add support in Nextcloud Talk for opening and closing the right sidebar by dragging it (I tried every other option I could think of that did not need a server change, but they were all flawed in some way :-( ); it will be used too to fix a bug when dragging files in the Files app.

The slide gesture is enabled or disabled depending on the width of the
browser window. In order to easily control that width the karma-viewport
plugin is now used in the unit tests.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
On narrow screens a slide gesture can be used to open or close the
navigation bar. However that gesture could conflict at times with the
gestures used by certain apps (for example, if the right sidebar is open
the user may expect to close it by dragging it to the right, but that
could open the navigation bar instead depending on how the events are
handled). This commit makes possible for apps to disallow and allow
again that slide gesture.

In any case, note that applications can only disallow the gesture,
but they can not enable it. That is, they can prevent the gesture from
being used on narrow screens, but they can not make the gesture work on
wide screens; they are always limited by the base rules set by the core.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
When a Snap was disabled it stopped listening to the events, but if a
drag gesture was being performed it was kept as active. Thus, when the
Snap was enabled again move events were handled as if the Snap had never
been disabled, causing the gesture handling to continue where it was
left.

When the Snap for the navigation bar is disabled by an app it could be
as a result of a different gesture being recognized by the app (for
example, a vertical swipe) once both gestures have started. In that case
when the other gesture ends and the Snap is enabled again any pointer
movement will cause the navigation bar to slide until an "up" event is
triggered again (obviously not the desired behaviour).

Due to all this now when the Snap for the navigation bar is disabled by
an app the current drag gesture for the navigation bar is ended.

Note that this was added as a parameter to "Snap.disable()" instead of
done unconditionally to keep back-compatibility with the previous
behaviour (probably not really needed as it is unlikely that any app is
using the Snap library relying on that behaviour... but just in case).

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
@danxuliu danxuliu added the 3. to review Waiting for reviews label Dec 15, 2017
@danxuliu danxuliu added this to the Nextcloud 13 milestone Dec 15, 2017
Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

We should do this consistently, definitely not let apps decide. Actually I’d say let’s simply deactivate the gesture since it results in more issues and glitches than value. The menu icon on top left is always there.

@jancborchardt
Copy link
Member

(And for the Talk app, the sidebar could be invoked with an icon-menu in the top right.)

@danxuliu
Copy link
Member Author

@jancborchardt

We should do this consistently, definitely not let apps decide.

If there is a slide gesture for the navigation bar then apps must be able to decide if its handler is enabled or not. Otherwise the slide gesture for the navigation bar could conflict with a more specific gesture from the app. For example, swipping the right sidebar to close it, or changing the current photo in a fullscreen gallery.

In any case note that the idea here is to disable the gesture only when it is really necessary, like when a conflicting and more specific gesture is being performed (and only when it is being performed). If an app disables the gesture in other situations then that is a bug in the app, not a problem with the API.

Also in some cases explicitly disabling the gesture may not be even necessary if the propagation of events is stopped before they reach its handler, although sometimes it is not possible to apply that technique (and hence this pull request).

Actually I’d say let’s simply deactivate the gesture since it results in more issues and glitches than value. The menu icon on top left is always there.

I would keep the gesture, at least with the current design for the navigation bar and the app content. Being able to show the navigation bar by dragging gives a more native feel in touch screens.

(And for the Talk app, the sidebar could be invoked with an icon-menu in the top right.)

There is already a button to open the right sidebar. Dragging it is just an additional way to do it, and as I just said one that feels more natural in touch screens.

@jancborchardt
Copy link
Member

jancborchardt commented Dec 15, 2017

Well, that will result in inconsistency. :) There are already 3 examples of main apps where it would be better to disable the swipe gesture for sidebar:

  • Files: cause it would conflict with drag & drop
  • Gallery: would conflict with swipe to change picture
  • Talk: conflicting with right sidebar

And those are pretty core apps. Any discrepancy will be experienced as a bug. We should either have the gesture consistently everywhere, or not at all. The global UX is not for specific app developers to decide.

@danxuliu
Copy link
Member Author

Well, that will result in inconsistency. :) There are already 3 examples of main apps where it would be better to disable the swipe gesture for sidebar:

  • Files: cause it would conflict with drag & drop

There is no need to disable the swipe gesture, except when a file is being dragged. And that is not an inconsistency, it is a logical behaviour! If I am dragging a file and the navigation bar is opened just because I moved the pointer slightly to the side instead of in a straight vertical line that would be a bug, not a consistent behaviour.

In fact, right now in master the swipe gesture is not handled while the dragging gesture is being performed. The problem is that, once the dragging ends, the handler for the swipe gesture keeps handling the move events and causing the navigation bar to open. It is related to how events are handled, but my point is that the gesture is already disabled; this pull request would not change anything, except for making possible to fix the bug (in this specific case there is another option to fix it, but it is more a hack and not the right way to solve it).

  • Gallery: would conflict with swipe to change picture

No, it will not. First, because right now there is no navigation bar in the Gallery app (inconsistency, anyone? :-P ).

But let's suppose that there was a navigation bar. Even in that case the navigation bar should not be shown when the images are in fullscreen mode, and the swipe gesture to change pictures would be used in fullscreen mode. In the same way that there would not be an icon to open the navigation bar when in fullscreen mode swiping should not cause the navigation bar to be opened. The gallery app would need to disable the gesture, but only when in fullscreen mode.

Is it inconsistent that in this case swipe causes the image to change instead of opening the navigation bar? Just as inconsistent as not having a button to open the navigation bar when in fullscreen mode. And I guess that we are not going to remove the navigation bar from everywhere just because it does not make sense when the Gallery app is in fullscreen mode. Being consistent is good, but there are always exceptions, and those exceptions should not condition the general case.

  • Talk: conflicting with right sidebar

There is no need to disable the swipe gesture, except when the right sidebar is being dragged. If the right sidebar is open and I drag its left border to the right I would expect it to close, not the navigation bar to open. But if I drag the app contents then I would expect the navigation bar to open, no matter if the right sidebar is open or closed, just like it is done everywhere else.

Any discrepancy will be experienced as a bug. We should either have the gesture consistently everywhere, or not at all.

And the gesture is consistent everywhere*: if you drag the app content the navigation bar is shown. However, if you drag an element which is in front of the app content it could make sense to not open the navigation bar, and that would not be an inconsistent behaviour at all. As a user, if I dragged the right sidebar from its left border and it was closed I would not see that as an inconsistency, it would be exactly what I expected to happen. And once the right sidebar is closed, if I dragged the app contents and then the navigation bar opened that would be the expected behaviour too.

*An exception would be a lateral swipe on a file in the file list, as in that case it would be handled by a drag of the file instead of a movement to open the navigation bar. But that is a bug in the Files app, not something that warrants to remove the gesture from everywhere else.

The global UX is not for specific app developers to decide.

Again, this pull request is not something to make possible for apps to decide on the global UX. It is something to make possible for apps to add specific gestures on top of the global UX, not instead of. Not merging this does not prevent apps from messing with the global UX; it simply prevents them from working more nicely with it.

@jancborchardt
Copy link
Member

Ok, so I misunderstood - it's for kinda temporarily preventing gestures messing with each other? :)

@danxuliu
Copy link
Member Author

it's for kinda temporarily preventing gestures messing with each other? :)

Yes :-)

@MorrisJobke MorrisJobke mentioned this pull request Jan 2, 2018
30 tasks
@rullzer
Copy link
Member

rullzer commented Jan 2, 2018

So what to do here. Beta4 is nearing. And it either gets in now or is moved. I don't have a preference. But doing this so late in the cycle mess with apps expeting this to work since they developed against the beta.

@danxuliu
Copy link
Member Author

danxuliu commented Jan 2, 2018

@rullzer This should be in Nextcloud 13. It does not have any side effect in apps that do not use the feature, but as already explained it is a must for those that need it.

Copy link
Member

@rullzer rullzer left a comment

Choose a reason for hiding this comment

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

Fine by me. 👍 for the code and tests

@MorrisJobke MorrisJobke merged commit 9d43724 into master Jan 2, 2018
@MorrisJobke MorrisJobke deleted the make-possible-for-apps-to-disable-the-navigation-bar-slide-gesture branch January 2, 2018 23:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants