bootstrap-hover-dropdown icon indicating copy to clipboard operation
bootstrap-hover-dropdown copied to clipboard

Prevent hover from acting on navbar dropdowns in collapsed mode

Open neil-h opened this issue 11 years ago • 18 comments

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.

neil-h avatar Aug 18 '14 23:08 neil-h

Do you think we should then get rid of the

if('ontouchstart' in document) return this;

line?

CWSpear avatar Aug 22 '14 17:08 CWSpear

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.

neil-h avatar Aug 22 '14 17:08 neil-h

What about #68?

CWSpear avatar Aug 22 '14 17:08 CWSpear

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 avatar Aug 22 '14 17:08 neil-h

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.

neil-h avatar Aug 22 '14 17:08 neil-h

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

CWSpear avatar Aug 22 '14 18:08 CWSpear

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 :)

neil-h avatar Aug 22 '14 18:08 neil-h

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.

sloothword avatar Sep 13 '14 20:09 sloothword

I've incorporated the changes suggested by Sloothword.

neil-h avatar Oct 22 '14 23:10 neil-h

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.

CWSpear avatar Oct 23 '14 06:10 CWSpear

//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();
        });
    };
});

thanhdevapp avatar Mar 11 '15 10:03 thanhdevapp

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

gadgetto avatar Mar 16 '15 22:03 gadgetto

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

CWSpear avatar Mar 17 '15 16:03 CWSpear

@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

gadgetto avatar Mar 18 '15 11:03 gadgetto

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.

CWSpear avatar Mar 19 '15 00:03 CWSpear

@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?

gadgetto avatar Mar 24 '15 15:03 gadgetto

@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".

neil-h avatar Jun 12 '15 00:06 neil-h

@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.

CWSpear avatar Jun 12 '15 00:06 CWSpear