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

Avoid new stack context for task list items #20377

Closed
wants to merge 5 commits into from

Conversation

Gusted
Copy link
Contributor

@Gusted Gusted commented Jul 15, 2022

  • Avoid creating a new stack context on a task list item, this PR does that by removing the position: relative which causes a new stack context to be created on that element. And thereby solving that popups weren't able to utilize their z-index and weren't able to show properly.

Before:
image

After:
image

- Avoid creating a new stack context on a task list item, this PR does
that by removing the `position: relative` which causes a new stack
context to be created on that element. And thereby solving that popups
weren't able to utilize their z-index and weren't able to show properly.
@Gusted Gusted added this to the 1.18.0 milestone Jul 15, 2022
Gusted pushed a commit to Gusted/gitea that referenced this pull request Jul 15, 2022
- Backport go-gitea#20377
  - Avoid creating a new stack context on a task list item, this PR does that by removing the `position: relative` which causes a new stack context to be created on that element. And thereby solving that popups weren't able to utilize their z-index and weren't able to show properly.

  Before:
  ![image](https://user-images.githubusercontent.com/25481501/179135078-63805ccc-17bc-4e11-8393-cdb6f1916cdd.png)

  After:
  ![image](https://user-images.githubusercontent.com/25481501/179135105-09996426-85da-40d7-8777-46a16a9413fe.png)
@Gusted Gusted added the backport/done All backports for this PR have been created label Jul 15, 2022
Copy link
Contributor

@zeripath zeripath left a comment

Choose a reason for hiding this comment

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

I can't quite understand why they ever had the relative position set

@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Jul 15, 2022
@wxiaoguang
Copy link
Contributor

Oh wait, to be more careful ..... I think this is incorrect. See the screenshot.

image

@Gusted
Copy link
Contributor Author

Gusted commented Jul 15, 2022

Thanks for testing @wxiaoguang, f15f897 fixes the issue by making the checkbox relative to the parent element.

@wxiaoguang
Copy link
Contributor

Better but there are still other cases. I think there should be more comments about all the cases.

image

@silverwind
Copy link
Member

silverwind commented Jul 15, 2022

The root problem is that the popups are part of the regular DOM structure where unintended interactions with stacking context happen with surrounding elements.

The real fix is to "portal" out the popup to be a direct descendant of body, positioned via popper.js or similar. Through that, all problems related to stacking context dissappear. We need to switch to something better than fomantic popups which can't do this.

@silverwind
Copy link
Member

silverwind commented Jul 15, 2022

Maybe fomantic can actually do popup portalling. Worth a try adding a #popup element to body, and then initialize via options:

$el.popup({
  popup: document.getElementById('popup'),
  movePopup: false,
  preserve: true,
});

@Gusted
Copy link
Contributor Author

Gusted commented Jul 16, 2022

Maybe fomantic can actually do popup portalling. Worth a try adding a #popup element to body, and then initialize via options:

We already use fomantic's popup functionality. However portalling should work by specifying inline: false(default value) as it then should append the popup to the body, however this is currently not the case/not working and just act as inline: true, likely because we're specifying a pre-formatted popup.

@Gusted
Copy link
Contributor Author

Gusted commented Jul 16, 2022

Popper looks interesting, don't have any experience with it. @silverwind would you like to take over this PR(or create a new one) otherwise I can have a look into popper some time soon.

@6543
Copy link
Member

6543 commented Jul 16, 2022

are things fixed ?!?

@Gusted
Copy link
Contributor Author

Gusted commented Jul 16, 2022

are things fixed ?!?

Adding status/wip

@Gusted Gusted added the pr/wip This PR is not ready for review label Jul 16, 2022
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Jul 17, 2022

My (draft) proposal is : https://github.com/wxiaoguang/gitea/tree/fix-stack-context (35631a7) , remove the position: relative. It hasn't got fine tuned yet, just for demo.

image

@silverwind
Copy link
Member

Popper looks interesting, don't have any experience with it. @silverwind would you like to take over this PR(or create a new one) otherwise I can have a look into popper some time soon.

Rewrite to popper (or maybe even the newer floating-ui, but it's quite complex to implement) will be more involved. It'd be awesome if we get portalling popups to work with Fomantic, otherwise I guess a workaround like done here is borderline acceptable.

@silverwind
Copy link
Member

silverwind commented Jul 17, 2022

#20393 is a replacement for this PR which uses portalling, so surrounding CSS won't interfere anymore with these popups.

silverwind added a commit to silverwind/gitea that referenced this pull request Jul 17, 2022
By appending the tooltips to document.body, we can avoid any stacking context issues caused by surrounding element's CSS.

I wasn't able to get this portalling to work with Fomantic popups, so this uses tippy.js instead of Fomantic popups. We should aim to replace all Fomantic popups with this eventually.

Fixes: go-gitea#20377
@Gusted Gusted closed this Jul 17, 2022
@Gusted
Copy link
Contributor Author

Gusted commented Jul 17, 2022

Closing as #20393 is better solution to this.

@Gusted Gusted deleted the fix-stack-context branch July 17, 2022 20:08
@lunny lunny removed this from the 1.18.0 milestone Dec 20, 2022
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport/done All backports for this PR have been created lgtm/need 1 This PR needs approval from one additional maintainer to be merged. pr/wip This PR is not ready for review type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants