Skip to content
This repository has been archived by the owner on Oct 2, 2019. It is now read-only.

fix(ui-select): avoid dropdown flickering on search #1557

Closed

Conversation

m3co-code
Copy link

I hope commit message and subject are descriptive enough :) This is a fix for #1298 with no other side effects then the flickering.

@wesleycho
Copy link
Contributor

This LGTM

@@ -331,9 +338,6 @@ uis.directive('uiSelect',
return;
}

// Hide the dropdown so there is no flicker until $timeout is done executing.
dropdown[0].style.opacity = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Removing this introduces a problem when dropdown is positioned up

http://plnkr.co/edit/rM3rdaekwqG8wN77Hgb6?p=preview

Enter text in select2 "up" and delete with backspace quickly (I typed ada to get most results).

With this line removed the select gets shifted/overlayed with the list as a flicker. (Comment/uncomment line ~1261)

We need to detect if dropdown is positioned up and apply this along with your additions on line 298.

@m3co-code
Copy link
Author

Thanks a lot for feedback! I will have a look tomorrow.

setting the opacity to 0 on each call to calculateDropdownPos brought
the dropdown to flicker when you typed a search text. Now it is only
doing this on initially opening the dropdown which avoids a flickering
on the opening effect.
@m3co-code
Copy link
Author

I had a look again in this issue. Sorry for the time delay guys..

I see the point of @user378230, when you reduce your search and thus extend the list again, the dropdown flickers for a short moment below the placeholder and then positions up. Though I can not figure out what is actually causing this or how to detect it..

I set up a plunkr with a couple of log messages where I tried to figure this out. Maybe you can give some useful input where to continue the investigation.

Please have a look in this Plunkr on lines 1259 and following. http://plnkr.co/edit/7NGlK9SRi95jNYTqpX1D?p=preview

@user378230
Copy link
Contributor

user378230 commented Apr 16, 2016

@marco-jantke a shot in the dark but I wonder if the changes in #1123 help at all?

edit: I tested in the plunkr and it helps but I wonder maybe your PR isn't required if #1123 is merged instead...?

For reference I commented these:

var setDropdownPosUp = function(offset, offsetDropdown){

          offset = offset || uisOffset(element);
          offsetDropdown = offsetDropdown || uisOffset(dropdown);

          //dropdown[0].style.position = 'absolute';
          //dropdown[0].style.top = (offsetDropdown.height * -1) + 'px';
          element.addClass(directionUpClassName);

        };

        var setDropdownPosDown = function(offset, offsetDropdown){

          element.removeClass(directionUpClassName);

          offset = offset || uisOffset(element);
          offsetDropdown = offsetDropdown || uisOffset(dropdown);

          //dropdown[0].style.position = '';
          //dropdown[0].style.top = '';

        };

Because of changes implemented since #1123 was made

@rpocklin
Copy link
Contributor

rpocklin commented May 9, 2016

This fixes the flicker on search, but there was a reason this was added in the first place
// Hide the dropdown so there is no flicker until $timeout is done executing.

The goal should be to avoid the opacity reset if the dropdown is already visible, i've created a tweaked PR in #1594

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

Successfully merging this pull request may close these issues.

None yet

5 participants