Possible not to memoize top offset?

[elmart]

Plugin currently memoizes element's top offset the first time it needs it. In topOffset():

    // We only want to calculate the vertical offset of the DOM element once,
    // both to be more efficient and because when the dropdown is rendered
    // upward we are changing its offset the first time we adjust its height.
    if (typeof $select2Results.data(OFFSET_KEY) === "undefined") {
      $select2Results.data(OFFSET_KEY, $select2Results.offset().top);
    }

Now, I have a select2 within a floating modal window. There, it's not rare for the user to move the window between one select2:open event and the next one. That makes memoized top offset obsolete, and consequent optimal height calculation wrong.

I tried to remove enclosing if, to always calculate the value, but then, I have wrong behaviour when rendering upwards. The comment above mentions something about this, but I'm not sure about which the correct fix would be.

Any indication about how to proceed, please?

[JacobEvelyn]

Thanks for the feedback! I won't be able to look into this before next week, but I'll be sure to get back to you with what I find. I don't remember the details of why not memoizing caused this behavior so there may be an alternate solution.

[JacobEvelyn]

Hey @elmart, sorry for the delay. I'm having trouble getting a reliable test case together because of issues that seem to be related to:

1. The hack that we use to determine whether the dropdown is displayed upwards or downwards. (At the time we currently check, Select2 hasn't had a chance to set this yet, so we initially treat every Select2 as though it displays downwards. Putting our hacky check in a timeout to force it after Select2's logic is probably the way to go.
2. Select2 v4.0.0 has a bug that causes the select2:open event to fire twice (https://github.com/select2/select2/pull/3507).
3. Select2 v4.0.1 has some rendering bugs (https://github.com/select2/select2/issues/3970) that are interfering with some of my test setups.

I'm going to keep digging but in the meantime if you might be able to produce a jsFiddle or other code snippet that demonstrates the problem you're facing, that would help a lot with me making sure a fix actually works for your use case.

[JacobEvelyn]

@elmart, would you be able to try the code in this commit (https://github.com/panorama-ed/maximize-select2-height/commit/fddb5a3fe3afce34c4ae6011da3c6cbaec9e0ef1) and see if that fixes your issue?

(You can just copy-paste the minified code here: https://raw.githubusercontent.com/panorama-ed/maximize-select2-height/fddb5a3fe3afce34c4ae6011da3c6cbaec9e0ef1/maximize-select2-height.min.js)

I think that should fix the issue for you as it removes the memoization that was at the heart of your problem, but I came across some bizarre edge cases, particularly with very small window sizes, that weren't well-supported by Select2's core and thus also behave badly with this plugin. I'm happy to go into more details if you like.

If it's helpful, the commit I linked to also includes the HTML file (with JS inlined) that I was using to play around with and test this behavior.

[elmart]

Hi guys. I'm very sorry I didn't respond before. I had my computer broken and it took some time to repair.

Proposed solution doesn't work in the dropdownDownwards case, but I had it working with a small fix (there was a mistaken sign). I'll comment the exact line in the PR. Apart from that, there's a little flickering which gets disturbing, but that's a different issue, and I'm afraid that would be more difficult/impossible to solve from outside select2.

So, once the appointed problem is solved, I consider this fixed.

Thx for your time.

[JacobEvelyn]

Good catch, @elmart! I was so busy testing my changes for the upwards direction that I totally missed the bug I had introduced for the downwards direction. This is now fixed. Hope the new changes (and new computer) work well for you!