Skip to content
This repository has been archived by the owner on Jun 11, 2021. It is now read-only.

Run onResize callback before updating scrollbars #1

Closed
wants to merge 1 commit into from

Conversation

richvdh
Copy link
Member

@richvdh richvdh commented Mar 24, 2016

No description provided.

@richvdh
Copy link
Member Author

richvdh commented Mar 24, 2016

@ara4n: This should make your forceUpdate() redundant

@ara4n
Copy link
Member

ara4n commented Mar 24, 2016

I experimented with this change last night and it actually broke my fix
in the main react-sdk PR...

On 24/03/2016 10:44, Richard van der Hoff wrote:

@ara4n https://github.com/ara4n: This should make your
forceUpdate() redundant

— You are receiving this because you were mentioned. Reply to this
email directly or view it on GitHub
#1 (comment)

@richvdh
Copy link
Member Author

richvdh commented Mar 24, 2016

The reason this makes a difference appears to be related to text wrapping. For whatever reason, calling update first means that text has been rewrapped before the onResize callback happens, so checkScroll does the right thing. If onResize is called first, checkScroll sees the un-wrapped version of the text; the text is then rewrapped after checkScroll runs, messing up the offset.

The question is, why does that make a difference, and can we come up with a more robust way of handling it? Apart from general unease about fragility, if geminipanel.update sees the un-wrapped text, it will lead to scrollbar-got-lost problems (element-hq/element-web#1216 etc)

@richvdh
Copy link
Member Author

richvdh commented Mar 24, 2016

It looks like Chrome rewraps the text when update writes to this._viewElement.style.

@richvdh
Copy link
Member Author

richvdh commented Mar 24, 2016

OH GOD I AM AN IDIOT

obviously chrome hasn't reflowed the text before update has been called, because update is the thing that is responsible for doing the resizing of the element.

@richvdh richvdh closed this Mar 24, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants