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

[polyfill] Polyfill the behavior of position: sticky; in thead/tfoot #507

Merged
merged 1 commit into from
Apr 14, 2018

Conversation

pzuraq
Copy link
Contributor

@pzuraq pzuraq commented Apr 13, 2018

The previous implementation of fixed headers/footers wouldn't actually
work for headers and footers that have more than 1 row because it fixed
td/th elements directly due to a bug in the Edge/Chrome versions of
sticky. This PR adds a polyfill which does several things:

  • It positions the headers/footers correctly individually based
    on a measurement of the row height
  • It repositions whenever the row height changes by using a ResizeSensor
  • It repositions whenever cells or rows are added by using
    MutationObservers

This is definitely suboptimal, but it's much better than our previous
setup and should be temporary.

The polyfills have been moved into private, and the legacy polyfill has
been refactored to match the style of the new one. They're both class
based now and use a WeakMap to tie themselves directly to an element
rather than the component class itself. This means we don't dirty the
component (or modify its hidden class) and we and we keep all of our
implementation details completely private.

One final change was to add an explicit width on the table itself. The
previous hack of setting width: 0; doesn't actually work in IE11, it
collapses the entire table, so we need to actually set it to the real
width (as defined by the columns).

position: sticky; polyfill, and should not be used as such.
*/

let LEGACY_POLYFILL_MAP = new WeakMap;
Copy link
Contributor

Choose a reason for hiding this comment

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

should be WeakMap()

@pzuraq pzuraq force-pushed the pzuraq/polyfill/polyfill-thead-tfoot-sticky branch from 10d86fa to b67326a Compare April 13, 2018 21:20
@billy-addepar
Copy link
Contributor

Do we have to reposition on every single cell? Is that possible to reposition for every row since it looks like all cells have the same offset?

this.setupResizeSensors();
this.setupRowMutationObservers();

this.mutationObserver = new MutationObserver(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

From what I understand, this observer reacts to every DOM event. In a complicated app, we would have lots of DOM events and this would be triggered multiple times. Why is using ResizeSensor not enough in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It reacts to specifically to children being added and removed, that's what the childList: true option does here. We need to reposition if a node was added or removed, because that node has not yet been positioned and could affect the positioning of other nodes.

This allows us to have a generic polyfill without tying into Ember directly.

@pzuraq
Copy link
Contributor Author

pzuraq commented Apr 13, 2018

Table rows have the same issue as thead and tfoot. According to the Chrome bug the underlying issue is that position: relative; is not supported on rows when it should be. Until this is fixed we must position on th and td directly.

@pzuraq pzuraq force-pushed the pzuraq/polyfill/polyfill-thead-tfoot-sticky branch 2 times, most recently from d92de6b to f35ad5d Compare April 13, 2018 23:53
The previous implementation of fixed headers/footers wouldn't actually
work for headers and footers that have more than 1 row because it fixed
td/th elements directly due to a bug in the Edge/Chrome versions of
sticky. This PR adds a polyfill which does several things:

* It positions the headers/footers correctly individually based
on a measurement of the row height
* It repositions whenever the row height changes by using a ResizeSensor
* It repositions whenever cells or rows are added by using
MutationObservers

This is definitely suboptimal, but it's much better than our previous
setup and should be temporary.

The polyfills have been moved into private, and the legacy polyfill has
been refactored to match the style of the new one. They're both class
based now and use a WeakMap to tie themselves directly to an element
rather than the component class itself. This means we don't dirty the
component (or modify its hidden class) and we and we keep all of our
implementation details completely private.

One final change was to add an explicit width on the table itself. The
previous hack of setting `width: 0;` doesn't actually work in IE11, it
collapses the entire table, so we need to actually set it to the real
width (as defined by the columns).
@pzuraq pzuraq force-pushed the pzuraq/polyfill/polyfill-thead-tfoot-sticky branch from f35ad5d to 42cc7eb Compare April 14, 2018 00:04
@pzuraq pzuraq merged commit d1095ce into master Apr 14, 2018
@bantic bantic deleted the pzuraq/polyfill/polyfill-thead-tfoot-sticky branch July 11, 2019 19:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants