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

Menu: fix selecting wrong element in tests #1861

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bennieswart
Copy link

Copied from https://bugs.jqueryui.com/ticket/15271:

During automated testing the autocomplete menu sometimes selects the wrong item upon click.

This can be reproduced by making a page with a normal autocomplete input with a couple of options. The tests populate the text of the input after which the menu opens. Then, the desired item is clicked. Even though the click event reports the correct item was clicked, the item which is actually selected by the autocomplete plugin may be a different one.

I could only reproduce this in automated tests and not by hand. The issue is most probably related to the tests not firing all events that would fire during normal operation. This is actually mentioned in a comment in Menu#select (​https://github.com/jquery/jquery-ui/blob/master/ui/widgets/menu.js#L673):

// TODO: It should never be possible to not have an active item at this
// point, but the tests don't trigger mouseenter before click.

The item to be selected is determined by the following line:

this.active = this.active || $( event.target ).closest( ".ui-menu-item" );

Thus, if this.active has not been properly set by preceding events, the wrong element may be selected.

I propose the following:

this.active = $( event.target ).is( ".ui-menu-item" ) ? $( event.target ) :
    this.active || $( event.target ).closest( ".ui-menu-item" );

In other words, if the target element is a menu item, then it is used as the active element. Otherwise, determine the active element as per usual. One might even add an explicit check for the event type, if necessary.

I'm seeing this in 1.11.4, but the error is still present in master at the time of writing (1.12.1). Additionally:

  • jQuery 2.1.4
  • Chrome 63
  • Ubuntu 16.04 and Arch

@jsf-clabot
Copy link

jsf-clabot commented May 15, 2018

CLA assistant check
All committers have signed the CLA.

Base automatically changed from master to main February 19, 2021 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

3 participants