Use resizeobserver when available #213
Conversation
|
Cool, can you pls remove that tabs? |
|
Done |
src/ResizeSensor.js
Outdated
| }; | ||
| var reset = function() { | ||
| // Check if element is hidden | ||
| if (initialHiddenCheck){ |
src/ResizeSensor.js
Outdated
| return; | ||
| } | ||
| // Stop checking | ||
| else{ |
src/ResizeSensor.js
Outdated
| var initialHiddenCheck = true, resetRAF_id; | ||
|
|
||
|
|
||
| var resetExpandShrink_ = function () { |
src/ResizeSensor.js
Outdated
| if (invisible) { | ||
| element.style.display = saveDisplay; | ||
| }; | ||
| var reset = function() { |
src/ResizeSensor.js
Outdated
| { | ||
| width: lastWidth, | ||
| height: lastHeight | ||
| }); |
There was a problem hiding this comment.
element.resizedAttached.call(size) should be enough, no?
src/ResizeSensor.js
Outdated
| requestAnimationFrame(reset); | ||
| } | ||
|
|
||
| if (typeof ResizeObserver != "undefined") { |
| width: elem.contentRect.width, | ||
| height: elem.contentRect.height | ||
| } | ||
| ); |
There was a problem hiding this comment.
how about callback.call(this, elem.contentRect)?
Don't know the definition of contentRect tho.
There was a problem hiding this comment.
I construct my own object because contentRect has more properties (like top,left,...)
So to avoid errors and keep it in line with the current implementation, I just pass these two
|
Just saw that you included the commit of #208, that should really not be part of this PR. |
|
Alright, then lets keep it simple and keep it. We close then both other PRs when this is merged. Please adjust then the requested changes that were coming in from the other PRs. :) |
|
I hope i got everything ;-) |
| // Check in next frame | ||
| if (!resetRAF_id){ | ||
| resetRAF_id = requestAnimationFrame(function(){ | ||
| resetRAF_id = 0; |
There was a problem hiding this comment.
that variable name is not so great, but I'll adjust it in master directly after merge.
There was a problem hiding this comment.
I also would like to throttle the calls, so maybe only every 100th requestAnimationFrame call do something, because I guess reading scrollTop and scrollLeft can be expensive. will adjust afterwards
There was a problem hiding this comment.
I agree, didn't think about that
There was a problem hiding this comment.
@marcj you might want to consider that throttling to that level (1 every 100 animationFrame) will be to slow to detect the resizing.
1 frame is around 1/60th of a second, so that makes it's around 1 check every 1,6 sec.
Use cases for this Initial detection are mostly for cases where something takes time to load, so it's hidden for 1 or 2 seconds at most.
But it's important to correctly detect when it's visible. (That is, at least, my own use case, where it's important to not miss the timing).
There was a problem hiding this comment.
1 check every 10th frame or so would be a good middle ground
every frame is to much 1/100 is not enough
There was a problem hiding this comment.
Not always. When I use stuff in Angular for example, I have plenty of objects in the DOM but not visible (display: none). If there's a resize-sensor attached it will check for each element for its scrollTop/scrollLeft option every 1/100 second, so if I have some elements like that then the CPU will be molten, I guess.
There was a problem hiding this comment.
I meant by since scrollTop/scrollTop === 0 is valid use-case, right, that when I have a initial hidden element lying at 0,0 (position: absolute; left: 0; top: 0), then scrollTop will always return 0, right? So it will never be detected as "it's now visible". Is my assumption wrong?
There was a problem hiding this comment.
well, HTML resize notification, while not native, should not be used extensively, and with great care.
In my case, I prefer exact performance instead of delayed detection. Maybe you can manage that with a configuration object ?
There was a problem hiding this comment.
yeah if we don't find a good balanced value, we should go with a configuration option.
There was a problem hiding this comment.
about 2nd comment:
I think not, because the scrollTop will detect scrolling area positioning, not the element absolute positioning.
And this very property is being set to 10000 by the reset() function.
Here I'm exploiting a "bug" in how the browser manage the scroll properties: when an element is not visible, you can't set its scrollTop propertie: it will be immediately reset to 0.
So taking advantage of this and as it's on "our" detection HTML element, this will correctly detect when the element will become visible, without much cpu cost.
|
Looks very good, thank you very much! : ) |
|
Thanks all, this is huge! Can we cut a new release with |
due to incompatibility with dom elements detection
|
This has been reverted in 45334e9 due to errors |
|
Just curious: what kind of errors ? I had been using this patch with no problems so far. Thank you! |
|
This line broke since the ResizeObserver does not inject a new HTML element as property called css-element-queries/src/ElementQueries.js Line 421 in 45334e9 |
This fixes #210 and #184 by using resizeobserver when available (afaik only chrome 64+ supports this at the moment)
The native resizeobserver is way faster than the current implementation.
This pr is based on #212