Tradeshift / tradeshift-ui

The Tradeshift UI Library & Framework
https://ui.tradeshift.com
Other
33 stars 45 forks source link

[Table] Column header disappear on element.scrollIntoView() #595

Open n-srg opened 6 years ago

n-srg commented 6 years ago

Bug report

Tradeshift UI version affected

v10.0.19, also appears for v10.0.21

Expected Behavior

Column headers should be shown in anyway

Actual Behavior

Column headers disappear

Steps to reproduce

  1. go to http://ui.tradeshift.com/v10/#components/table/demo.html
  2. open developer tools
  3. inspect any of the row checkboxes (example var checkbox = $('.ts-table-checkbox-button')[0])
  4. execute checkbox.scrollIntoView()
  5. take a look on the column headers

Screenshots (optional)

image

wiredearp commented 6 years ago

@n-srg I unfortunately cannot absolutely guarantee that this can be fixed, it is a classic problem with the scrollIntoView method, but we can of course give it a try. I suggest the workaround of not using this method and instead rely on scrollTop to control the scrolling. The following properties could be involved in a method that is safe for work and perhaps there are jQuery plugins or something that will use these properties instead of relying on the sketchy, native method.

something.scrollTop = thing.offsetParent - thing.offsetHeight; // or something like that

Chrome and Safari has a funny method called scrollIntoViewIfNeeded or something similar, this at least will not destroy the layout unless there is a good reason.

n-srg commented 6 years ago

okay. just found that while debugging our tests. so we definitely should remove usages the scrollIntoView to avoid any flaky behavior

wiredearp commented 6 years ago

Well, it does work in some or most cases but apparently not in this case. I've seen the method destroy UI before while trying to use it for a user-facing feature, so it is unfortunately flaky if you attempt to use it on complicated layout. I guess we could attempt to pseudo-polyfill the feature using safe properties such as scrollTop, but I am guessing that we'll not be in a position to prioritize it right now. If you find that you do need the feature, let us know so we can change our priorities.