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

[7.1.1] Some overflow settings for table parent do not allow to display the table #6344

Closed
AMBudnik opened this issue Oct 15, 2019 · 13 comments
Closed

Comments

@AMBudnik
Copy link
Contributor

@AMBudnik AMBudnik commented Oct 15, 2019

Description

Handsontable is not visible when we use overflow CSS property.

Steps to reproduce

  1. Compare the two versions below

7.1.0

image

7.1.1

image

Conclusions

  • there are no console errors.
  • table is built in the DOM
  • if applied without a parent they work as expected
  • Affected by scroll, hidden, auto. Not affected by inherit, initial, overlay, visible and of course unset.

Demo

http://jsfiddle.net/handsoncode/3e4r78vn/ (7.1.0. - working)
http://jsfiddle.net/handsoncode/ycrmaxkL/ (7.1.1 - broken)

Your environment

  • Handsontable version: 7.1.1+
  • Browser Name and version: Chrome 77
  • Operating System: Windows 10

Inform forum

@AMBudnik

This comment has been minimized.

Copy link
Contributor Author

@AMBudnik AMBudnik commented Oct 15, 2019

Also, the weird behavior of adding the rows on render (offset) is described here #4141 (comment)

@aninde

This comment has been minimized.

Copy link

@aninde aninde commented Oct 15, 2019

The bug is replicable in latest release 7.2.0

v. 7.2.0 http://jsfiddle.net/aninde/g2hL4a63/

@pnowak

This comment has been minimized.

Copy link
Member

@pnowak pnowak commented Oct 15, 2019

This is the intentional behaviour - #6074 - if we haven't height property in the handsontable settings or in the parent node and we have in parent overflow scroll or hidden we don't display hot table.

@AMBudnik

This comment has been minimized.

Copy link
Contributor Author

@AMBudnik AMBudnik commented Oct 15, 2019

I can agree with hidden, but we would need to check if it should do the same for scroll. And if the answer is yes then we'd need to create an official statement about why it should work this way.

@wojciechczerniak

This comment has been minimized.

Copy link
Member

@wojciechczerniak wojciechczerniak commented Oct 15, 2019

@AMBudnik If the container size is not defined Handsontable default is height: 100%. Since the container size is 0 the 100% from 0 is 0. You can set the height: 'auto' and Handsontable will stretch the container (v7.2.0):

overflow: hidden: https://jsfiddle.net/zb2fj1dm/
overflow: scroll: https://jsfiddle.net/vprLbo4t/
overflow: auto: https://jsfiddle.net/vprLbo4t/1/

Maybe height: 'auto' should be the default behavior? We can discuss the pros and cons.

I agree with @AMBudnik that overflow: scroll should be handled better. Should behave the same as overflow: hidden. We've missed this one @pnowak ?

Second minor issue is that with default height Handsontable will take 100% of the container but with license visible there is an additional scroll: https://jsfiddle.net/vprLbo4t/2/ In production env the license should be always set either way, so rather low priority on this one.

@pnowak pnowak added this to the October 2019 milestone Oct 16, 2019
@pnowak pnowak self-assigned this Oct 16, 2019
@pnowak

This comment has been minimized.

Copy link
Member

@pnowak pnowak commented Oct 17, 2019

The clue of this problem is that our fix - #6074 - doesn't work on Firefox.

@pnowak pnowak removed the Regression label Oct 17, 2019
pnowak added a commit that referenced this issue Oct 18, 2019
@pnowak pnowak mentioned this issue Oct 21, 2019
2 of 6 tasks complete
pnowak added a commit that referenced this issue Oct 23, 2019
pnowak added a commit that referenced this issue Oct 23, 2019
pnowak added a commit that referenced this issue Oct 23, 2019
pnowak added a commit that referenced this issue Oct 23, 2019
* After CR: use overflow auto in cloned node and add tests #6344
@aninde aninde added the Firefox label Oct 29, 2019
jansiegel added a commit that referenced this issue Nov 4, 2019
* After CR: use overflow auto in cloned node and add tests #6344
@aninde

This comment has been minimized.

Copy link

@aninde aninde commented Nov 14, 2019

I'm not sure what the final assumption should be on tested 7.3.0 v.. @AMBudnik can you double check it?
Without height auto table is still do not rendering on Chrome 78/Firefox 70/Safari 13
http://jsfiddle.net/xb7scr9L/1/
Screenshot 2019-11-14 at 17 32 03

Height: auto

Overflow: scroll / Chrome 78
Screenshot 2019-11-14 at 17 21 25

Overflow: hidden / Chrome 78
Screenshot 2019-11-14 at 17 21 11

There is still additional scroll https://jsfiddle.net/aninde/u0kogcaj/

@AMBudnik

This comment has been minimized.

Copy link
Contributor Author

@AMBudnik AMBudnik commented Nov 15, 2019

Hey @aninde In the end, we should NOT have a visible container with overflow: scroll and without height defined. That was an issue for FF.

@aninde

This comment has been minimized.

Copy link

@aninde aninde commented Nov 15, 2019

OK, so in 7.3.0 v. the result on Firefox with overflow: scroll without height http://jsfiddle.net/aninde/udamv0fz/
Screenshot 2019-11-15 at 08 28 00
@pnowak do we understand the assumptions well? Was this the planned effect?

@pnowak

This comment has been minimized.

Copy link
Member

@pnowak pnowak commented Nov 15, 2019

Yes, this is the planned effect.

@aninde

This comment has been minimized.

Copy link

@aninde aninde commented Nov 15, 2019

So this issue is fixed by 7.3.0

@aninde

This comment has been minimized.

Copy link

@aninde aninde commented Dec 9, 2019

On currently tested latest build of 7.3.0 v (6.12) everything works according to assumptions. The result is correct and the same like in above http://jsfiddle.net/aninde/fecL4q7z/

@AMBudnik

This comment has been minimized.

Copy link
Contributor Author

@AMBudnik AMBudnik commented Dec 13, 2019

That was an interesting case. Thanks for solving guys!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.