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

Fixed positioned headers force Kee panels too low in the document #305

Open
luckyrat opened this issue Feb 18, 2021 · 0 comments
Open

Fixed positioned headers force Kee panels too low in the document #305

luckyrat opened this issue Feb 18, 2021 · 0 comments

Comments

@luckyrat
Copy link
Member

luckyrat commented Feb 18, 2021

Typically this isn't easy to notice but for those with a keen eye, you might occasionally see a website where the Kee panels launched from the in-field icons do not appear to be "attached" to the relevant form field.

If a header with position: fixed is included in the host page, this will throw off the calculations for where Kee's position: absolute panel should be rendered.

We already use requestAnimationFrame to update the position of the panel in the case that the field's position in the page changes. Perhaps this can also be used to take account of scroll position and thus allow us to use position: fixed instead of absolute?

Unfortunately that approach to fix this issue is definitely going to be less efficient because the browser will have to perform a re-layout operation with every new scroll position. I'm not sure requestAnimationFrame will even result in a smooth scroll operation so other ways to track and respond to scroll changes would be required.

The other possible approach would be if we can reliably detect the height of all fixed components that are affecting the top position of the main container the form field is contained within.

Adjusting the positioning code like:

        const parentOffsetTop = this.container.offsetParent?.getBoundingClientRect()?.top ?? 0;

        const targetTop = this.targetRelativeRect.top + window.scrollY - parentOffsetTop;
        const targetBottom = this.targetRelativeRect.bottom + window.scrollY - parentOffsetTop;

would work except that offsetParent is null when we calculate the position because it has yet to be attached to the DOM. Either we can come up with a way to guarantee the correct answer to that calculation before attaching to the DOM or we can force an immediate re-positioning once it has been attached. The latter would cause a visual glitch for users which might be worse than the current status quo. Maybe for the former idea we could attach a dummy div to the DOM first and determine the offsetParent from there.

In any case, there are apparently some HTML spec bugs which mean that the offsetParent may still remain null in certain circumstances but at least we would cover a lot of cases.

Given the lack of serious impact for this bug and the lengthy testing that would be required to certify any change as not regressing on any website, I'm not planning to address it soon. I'll accept a PR but only after the author has used their development version in the real world for a number of weeks. Please feel free to open a PR to discuss any code changes you propose testing.

E.g. (Feb 2021): https://secure.tesco.com/account/en-GB/login?from=/

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

No branches or pull requests

1 participant