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.

Sign up for GitHub

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

Jump to bottom

Overlay stack breaks body height 100% #803

Closed
adixon-adobe opened this issue Aug 4, 2020 · 13 comments
Closed

Overlay stack breaks body height 100% #803

adixon-adobe opened this issue Aug 4, 2020 · 13 comments
Labels
bug Something isn't working Component: Overlay

Comments

@adixon-adobe
Copy link
Collaborator

Expected Behaviour

If you have height: 100% set on body, child elements can also set height: 100%

Actual Behaviour

The new shadow root added to the overlay stack breaks this. Looks like this was introduced here:
27a0b53#diff-c3039ae6864cf1ed51034cbdfc4040deR49

Sample Code that illustrates the problem

This should show a red background, but doesn't:

import '@spectrum-web-components/overlay/active-overlay.js'
<body style="height: 100%">
  <div style="background-color: red; height: 100%">
  </div>
</body>
@adixon-adobe adixon-adobe added bug Something isn't working Component: Overlay labels Aug 4, 2020
@adixon-adobe
Copy link
Collaborator Author

Looks like a height: inherit will fix this. I could see the position: relative and z-index: 0 styles causing issues as well.

@Westbrook
Copy link
Contributor

For clarity of testing:

<body style="height: 100%">
  <div style="background-color: red; height: 100%">
  </div>
</body>

Does not full the viewport whether the overlay code is loaded or not as body/html aren't given the whole viewport to fill by default. 100vh would need to be used on the body or the div in this case. When applied to the div, this issue would be avoided, but when applied to body having height: inherit seems like a good call. Another option would be to use height: 100% which would allow for the element to fill the more complex rules that might come together with use of min-height/max-height/height on the body.

Thoughts?

@adixon-adobe
Copy link
Collaborator Author

Would height: inherit, min-height: inherit, and max-height: inherit work? The more this avoids taking over the better.

Also, fwiw it looks like when the body is position: fixed the div#actual in the shadow DOM is laying out with full height (at least in Chrome). This isn't something I knew the behavior of, and I'm having a little trouble confirming that it's defined behavior, though I'm guessing it is.

@Westbrook
Copy link
Contributor

Westbrook commented Aug 4, 2020 •
edited
Loading

inherit is tricky, because id <body style="height: 50%"> then your page will actually be 25% because of inherit. Whereas, height: 100% just fills whatever the <body> is set to without further thought. If you don't use the right rules on the <body> stays at 0, use 50%, fills that whole space, etc.

Do you have code, or a screenshot of:

Also, fwiw it looks like when the body is position: fixed the div#actual in the shadow DOM is laying out with full height (at least in Chrome).

As I understand it, it should happen, but in my mind I may not be seeing what you're looking at.

@adixon-adobe
Copy link
Collaborator Author

What I've found is that the new shadow DOM doesn't break this layout, though I was expecting it to once I started digging in:

<body style="position: fixed;top=0;left=0;right=0;bottom=0;height:100%">
  <div style="height=100%"></div>
</body>

If I add an extra div in the main DOM though like this (similar to what's getting added to the shadow DOM) it does break: https://codepen.io/dixo0015/pen/WNwNvEG

@Westbrook
Copy link
Contributor

This is why I think having the height: 100% should address this:

https://codepen.io/Westbrook/pen/poyoJdw

The blank div in you example will break this because it won't inherit any sizing, and the height: 100% in the lowest child will have not context to resolve "100%".

With that addition, the shadow DOM would be roughly equivalent to:

<body style="height:100%;position: fixed;top:0;left:0;right:0;bottom:0">
  <div style="position: relative; z-index: 0; height: 100%">
    <div style="background-color:red;height:100%">
    </div>
  </div>
</body>

@adixon-adobe
Copy link
Collaborator Author

Yeah -- what's weird is that you don't need the height: 100% for the fixed case in the shadow DOM on Chrome (that's the part that tripped me up -- "why isn't this broken too?").

I.e. this is broken:

<body style="height:100%">
  <shadowdiv style="position: relative; z-index: 0;">
    <div style="background-color:red;height:100%">
    </div>
  </shadowdiv>
</body>

But this isn't (which seems weird):

<body style="height:100%;position: fixed;top:0;left:0;right:0;bottom:0">
  <shadowdiv style="position: relative; z-index: 0; height: 100%">
    <div style="background-color:red;height:100%">
    </div>
  </shadowdiv>
</body>

@adixon-adobe
Copy link
Collaborator Author

It does look like height: 100%. I still have some concerns that this won't necessarily work for all body styles (I can't think of a way to break it right now, but I'm not convinced there isn't yet either).

I'll run this by a few more folks on my team to see if they can poke any holes in it.

@Westbrook
Copy link
Contributor

I did just recently participate in a twitter convo around the "need" for height: stretch: where I learned there is much in this area to know, so please do. Later this week, if this is at least iteratively better, we might move forward anyways (if there are no new holes), and then come back to it in the future if we need to.

@adixon-adobe
Copy link
Collaborator Author

yes, very much agreed! I'm actually working on a PR to move forward since this might be "fixed, practically speaking", even if it's hypothetically broken for unlikely cases.

@adixon-adobe adixon-adobe mentioned this issue Aug 4, 2020
Merged
1 task
@cuberoot
Copy link
Collaborator

cuberoot commented Aug 5, 2020

I am a bit amazed at how hard a problem it is to get focus, tabs and all working as you would expect in all places. I cannot thing of a breaking case off the top of my head. I guess we proceed and see how we go!

I cannot remember who said that "everybody should be using web components, but very few people should be writing them," but that seems true.

@Westbrook
Copy link
Contributor

@adixon-adobe did this get fully cleaned up with #807 or do we have more work you'd like to see here?

@adixon-adobe
Copy link
Collaborator Author

I'll close this out. No one has thought of anything this would break (outside of conflicting web component frameworks).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working Component: Overlay
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@cuberoot @Westbrook @adixon-adobe