Improve rounding

[atomiks]

Fixes #713

It appears that Math.round() is always preferred except with placements top and bottom (not a variation) and where Math.floor() is preferred (and only on the left and right values)...

See the playground: https://codepen.io/anon/pen/Pxjdoo

[FezVrasta]

Should the playground include already your fix?

I see some misalignment:

[atomiks]

Yeah, line 1243...

Perhaps we need to take into account the odd/even width then.

When I resize the window using PR code:

• pop = even, ref = odd === perfect
• pop = even, ref = even === jittery / sometimes incorrect position

When I resize the window always using Math.round():

• pop = even, ref = odd === jittery / sometimes incorrect position
• pop = even, ref = even === perfect

Can you reproduce that?

[FezVrasta]

Yes I can repro

[atomiks]

I think this is the pattern:

• If the reference & popper are the same oddness in width, then use Math.round()
• If they're different oddness in width, then use Math.floor()

This still only applies to horizontal placement and the left/right values

[atomiks]

Check the CodePen when the above is taken into account, I can't see jitters anymore

[FezVrasta]

top and bottom are working fine as they are already?

[atomiks]

It appears so yes. Horizontally it appears flush with the reference, and you can change the code at the very bottom to left / left-end as an example too.

However I just noticed that pop = odd, ref = odd doesn't like Math.round() (but doesn't jitter). Another edge case? 😓

[FezVrasta]

Please, let's make sure the behavior is consistent across browsers also.

[atomiks]

This might be getting stranger by the second..

If both are odd width, then we need

left: toInteger(bothOddWidth && !isVariation ? popper.left - 1 : popper.left),

I'm going to check Firefox and Safari now

[FezVrasta]

Considering the number of edge cases you are finding I think it would make sense to write some tests to make sure everything is working as expected.

[atomiks]

On Firefox it looks like the element is positioned on a half pixel, no matter what if we use a full pixel then it can't be positioned correctly. My suggestion there would be not to round if the screen is HiDPI (so there won't be blurriness). If you don't round at all, FF appears perfect, same with Safari.

Something like window.devicePixelRatio >= 2 ? dontRound : round

[FezVrasta]

this thing is getting complex 😅

may we limit this PR to the most basic use case (non HiDPI screens) and think about the HiDPI screens in a new one?

[atomiks]

Yeah it's kinda crazy for something that should be simple..

FWIW here is what it would look like to check for that:

const isVertical = ['left', 'right'].indexOf(data.placement) !== -1;
const isVariation = data.placement.indexOf('-') !== -1;
const sameWidthOddness = reference.width % 2 === popper.width % 2;
const bothOddWidth = reference.width % 2 === 1 && popper.width % 2 === 1;
const isHiDPIScreen = window.devicePixelRatio >= 2;
const noRound = v => v;

// We don't need to round if the device has a high pixel density
const horizontalToInteger = isHiDPIScreen
  ? noRound
  : isVertical || isVariation || sameWidthOddness
  ? Math.round
  : Math.floor;
const verticalToInteger = isHiDPIScreen ? noRound : Math.round;

const offsets = {
  left: horizontalToInteger(
    bothOddWidth && !isVariation && !isHiDPIScreen
      ? popper.left - 1
      : popper.left
  ),
  top: verticalToInteger(popper.top),
  bottom: verticalToInteger(popper.bottom),
  right: horizontalToInteger(popper.right)
};

And yes I agree it needs tests due to all the logic 😅

Edit: Aand Chrome prefers to be rounded.... 🤣 so it needs:

• Device pixel density check
• User agent check
• Popper + reference width check
• Placement variation check

[FezVrasta]

if you add the tests I'll merge it 😜

[atomiks]

Will try to write some tests in a sec

[atomiks]

@FezVrasta are the tests appropriate?

[FezVrasta]

Thanks yes they look solid!