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

Bugfix: Revert iframe z-index to -1 in resize listener #5448 #5742

Merged
merged 5 commits into from
Jan 2, 2021
Merged

Bugfix: Revert iframe z-index to -1 in resize listener #5448 #5742

merged 5 commits into from
Jan 2, 2021

Conversation

nick-vincent
Copy link
Contributor

Problem:

As discussed in issue #5448:

When an element uses block-level bindings (e.g. bind:clientHeight), the resize listener appends an "invisible" <iframe> inside the element with a z-index of the element minus 1.

However, when the element has other children elements with z-index: auto, this results in the <iframe> being layered "above" the children. To illustrate:

resize-bound element: z-index: 100
|- child: z-index: auto
|- child: z-index: auto
|- iframe: z-index: 99 <-- layers above the children

Although the <iframe> has pointer-events: none, it still blocks scrolling on child elements in certain browsers, namely macOS Safari and iOS browsers:

Prior to this diff, the <iframe> was assigned z-index: -1, which effectively layers it below any children elements.

Screenshot:

101125889-29193980-35af-11eb-9f75-8ab86aead056

Solution:

This change reverts back to setting the <iframe> to z-index: -1 which no longer obscures children of the bound element.

@nick-vincent
Copy link
Contributor Author

I see that some CI tests were cancelled, possibly due to timeouts? Not sure how to rerun them.

@benmccann
Copy link
Member

@nick-vincent I wouldn't worry about it if it's just a timeout. I re-triggered them and everything passed

I'm not very familiar with the svelte codebase, but is there anyway we can add a test for this? If this is an area where we've hit some edge cases it seems like a useful place to make sure those are covered

@nick-vincent
Copy link
Contributor Author

@benmccann Thanks for kicking the builds for me. I've added a regression test for this fix.

Also, as a sanity check, I'd appreciate if @mrkishi or @Conduitry could weigh in on why this changed in the first place. I couldn't find any discussion about it in the relevant pull request.

@mrkishi
Copy link
Member

mrkishi commented Dec 8, 2020

This looks good!

I don't remember why I did that. I must have had a separate issue and blundered an incorrect fix, because z-index - 1 seems wrong indeed, regardless of it being an iframe or object. My apologies!

@nick-vincent
Copy link
Contributor Author

Thanks for taking a look, @mrkishi! Didn't want to inadvertently introduce another regression.

@nick-vincent
Copy link
Contributor Author

Looks like this PR is good to go... is there any other action I should take to get this merged?

@Conduitry Conduitry merged commit f7d4eef into sveltejs:master Jan 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants