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

Prevent hover from acting on navbar dropdowns in collapsed mode #75

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

Conversation

neil-h
Copy link

@neil-h neil-h commented Aug 18, 2014

this commit fixes #73

This will prevent hover from activating a dropdown in a navbar in collapsed mode.
If someone has set their navbar collapse breakpoint to something other than the default, then they will need to enter that into the settings.

Added a setting for navbar breakpoint in defaults
Added check in openDropdown to see if dropdown is a descendant of .navbar-collapse and window width is less than the navbar breakpoint
changed from tabs to 4 spaces
@CWSpear
Copy link
Collaborator

CWSpear commented Aug 22, 2014

Do you think we should then get rid of the

if('ontouchstart' in document) return this;

line?

@CWSpear CWSpear mentioned this pull request Aug 22, 2014
@neil-h
Copy link
Author

neil-h commented Aug 22, 2014

I don't think so. That line is to disable the plugin on touch enabled devices. My updates are to disable the plugin when the menu is in collapsed mode (i.e. acting like an accordion).
In collapsed mode each dropdown can have a different height, and so, when moving the mouse from the top down, some items may be skipped if opening on hover.
It's not about "mobile" or not, it's about whether the menu is layed out vertically or horizontally.

@CWSpear
Copy link
Collaborator

CWSpear commented Aug 22, 2014

What about #68?

@neil-h
Copy link
Author

neil-h commented Aug 22, 2014

Well, like I say, it's a different issue.
#68 is trying to figure out how to make hover work in situations where there is both touch and mouse control.
Here I'm saying, touch or not, you don't want hover to be opening menu items when in collapsed mode. for example you have a menu like:

item 1
-sub 1
-sub 2
-sub 3
item 2
-sub 1
item 3
-sub 1

In collapsed mode, when you move the mouse off the bottom of item 1-sub 3 the menu collapses down, and you will skip over item 2 entirely (maybe item 3 as well, not testing right now).
That's why in collapsed mode you want to require a click to open, even if you have a mouse.

@neil-h
Copy link
Author

neil-h commented Aug 22, 2014

Here's an example of the effect I'm discussing http://www.bootply.com/3dPu0COV1A
Be sure to size down your browser to less than 768px wide.

@CWSpear
Copy link
Collaborator

CWSpear commented Aug 22, 2014

So is the use case people with really really small laptops?

@neil-h
Copy link
Author

neil-h commented Aug 22, 2014

I'll admit it is a less common use case (I'm not sure if I noticed it before reading issue #75).
I'm developing a site where I have my collapse breakpoint at 992px, and I'm sure some have it higher, but still you'd be unlikely to run into this issue except on a very small netbook, or if someone is browsing without their browser maximised.
Maybe the most common case would be someone with their browser at half screen. I know I do that at times, but I'm a web developer, not a normal human being :)

@sloothword
Copy link

Just running into the same issue. I also admit to having more than one window open :-)
I cannot think of any case where an user wants hover functionality in the collapsed menu.

To eliminate the extra needed config for the breakpoint i would change the width < breakpoint check to .navbar-toggle is visible

Addendum:

Also the delayed close after mouseover (line 57) needs this check. Otherwise the menu items in the collapsed menu close automatically.

Instead of having a variable for nav breakpoint we now check if .navbar-toggle is visible.
We also have added the check to the close on mouseout, so the menu doesn't close when we hover outside the submenu.
Made a function isCollapsed so we can have our logic in one place.
Was: if ( isCollapsed ){ return; }
Now: if ( isCollapsed() ){ return; }
@neil-h
Copy link
Author

neil-h commented Oct 22, 2014

I've incorporated the changes suggested by Sloothword.

@CWSpear
Copy link
Collaborator

CWSpear commented Oct 23, 2014

This is looking better. I will try and go over it and see how it behaves

I am nervous to basically change anything at this point without getting some tests to make sure we don't regress. So many little edge cases! See #69.

@thanhdevapp
Copy link

//Disabled menu hover < 768px .disabled menu hover. It work (with name class bootstraps) you can try 

$(window).resize(function () {
    if ($(window).width() < 768) {
        var horizontalmenu = $("body").find(".horizontal-menu");
        horizontalmenu.find(".nav li.dropdown .dropdown-toggle").each(function (e) {
            $(this).removeAttr("data-hover");
        });
        $(".horizontal-menu").html(horizontalmenu.html());
    } else {
        $(".nav li.dropdown .dropdown-toggle").each(function (e) {
            $(this).dropdownHover();
        });
    };
});

@gadgetto
Copy link

I implemented the changes form @neil-h (commit d0320cb and 5ed1400)
Tested in Safari and Chrome without problems.
Will also test in iOS Safari...

@CWSpear
Copy link
Collaborator

CWSpear commented Mar 17, 2015

So does this not break buttons (stuff not in navbars) when on mobile?

@gadgetto
Copy link

@CWSpear can't imagine why this should break any stuff outside the navbar!?
Tested on iPhone = OK
Tested on iPAD = OK
Tested on Desktop Safari = OK
Tested on Desktop Chrome = OK
Tested on Desktop Windows IE 11 = OK

@CWSpear
Copy link
Collaborator

CWSpear commented Mar 19, 2015

Sorry, I was thinking about it backwards.

But looking at this PR, if you don't have a navbar, etc,

$(".navbar-toggle").filter(":visible").length

will always be 0, and mess up that isCollapsed() function.

@gadgetto
Copy link

@CWSpear you are right - didn't think about. But this should be fixable easily. I'm not so into JavaScript but how about a first check if the .navbar-toggle exist and then do the length check?

@CWSpear CWSpear added this to the v3.0 milestone Apr 28, 2015
@neil-h
Copy link
Author

neil-h commented Jun 12, 2015

@CWSpear Is your concern that some people will have a navbar without a toggle icon? I'm not sure of a solution for that, but remember that the isCollapsed function works to -disable- the plugin. If there is no .navbar-toggle, then it will return false, and thus the items you hover over -will- dropdown.
Only if the item is inside a .navbar-collapsed, and a .navbar-toggle is visible, will the isCollapsed function work to prevent a hover from activating the dropdown.

@gadgetto $(".navbar-toggle").filter(":visible").length basically means "if there is at least one .navbar-toggle, which is visible".

@CWSpear
Copy link
Collaborator

CWSpear commented Jun 12, 2015

@neil-h ok, I see what you're saying about it really only affecting the nav bar, but it looks like it is still a little dangerous.

By the way, this issue goes away if we use PointerEvents (see commit 6d163831).

That approach requires PointerEvents tho (duh), which are currently only supported on IE, but coming to Chrome and Firefox, and requires jQuery's PointerEvent Polyfill in the interim. However it's the only solution I've found that really solves issues like this one and #68 in a real, solid manner.

This sort of stuff is also very hard to test in an automated fashion, and one of the goals for v3.0 of this plugin is tests. We haven't been able to really get any help on that front, and so the going is very slow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Destroy function?
5 participants