Fix a regression in adaptive trimming #1970
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Deployed to Cloudflare Pages
|
csillag
force-pushed
the
csillag/more-adaptive-trigger-fixes
branch
from
2a7ab2b to
f7d3cde
Compare
Added detailed explanation to the PR description. |
Add more resilience about React components mounting and unmounting during the renders and resizes. (In some rare circumstances, it was possible to get the process stuck.)
csillag
force-pushed
the
csillag/more-adaptive-trigger-fixes
branch
from
f7d3cde to
04d001c
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a follow-up to #1963.
(I found a corner case to break it after merging.)
This change add more resilience to the adaptive trimming process in situations when
React components are mounting and unmounting while the process is ongoing.
(Earlier, in some rare circumstances, the process could get stuck.)
Explanation
This is how the adaptive trimming method has evolved
The original method
In the original implementation of the adaptive trimmer, the idea was that we find out the largest fitting length of text by watching for overflow happening on the element directly containing the content to shorten. This requires all the elements further up in the DOM to have exactly the right CSS settings, otherwise the overflow won't happen, or won't happen at the right place. This was a bit fragile, because changing something in the DOM at a higher level could break the adaptive trimming of the contained adaptive trimmers. (Like it happened in #1943)
Relaxing the CSS requirements
To avoid this fragility, the behavior has been changed to look for overflow happening not only on the exact wrapping element, but at any level above this node, that is, on all parents up to the root. (Sometimes there is already overflow even without our text being too long, so first we do a baseline measurement with a very short text, record which is the first level where we find overflow, and then compare the new situation to this saved baseline when experimenting with longer texts.)
This approach makes it unnecessary to add specific CSS configuration to all wrapping levels, therefore makes the system more robust. But there is a problem:
Solving the interplay of multiple adaptive trimmers working at the same time
In the new approach, we let the components resize, and therefore change the layout of a larger sub-tree of the dom, until they hit an overflow. This works fine if there is only one adaptive trimmer doing this, but if multiple adaptive trimmers are doing this at the same time, then the results are unpredictable. (I.e. we won't know which adaptive trimmer has too long text, causing the overflow at an upper level.) The solution to this was adding a singleton controller object, which orchestrates the whole process:
This way, there will be no confusion with the test results, because at each point in time, there is exactly one adaptive trimmer that is attempting to adapt (to the available size). This has been implemented as part of #1963, and solved several issues. But there is one more problem which was not handled...
Solving the interplay of window resizing, component unmounting and ongoing adaptation
When the window is being resized, we get multiple resize events. Reacting to these events, we do a full round of adaptation. This is relatively fast, but not instantaneous. It's possible that we receive multiple events while still busy, but that is not a problem, we can handle that. The tricky situation is when as the result of enlarging the window size, we go over the limit to desktop territory, so we no longer want to do any trimming at all. This causes the adaptive trimmer to be removed from the DOM altogether. The problem happens if the component removed was the one that has the permission from the controller to do the resizing. This means that the resizing of that component never finishes (since it is removed from the DOM altogether), but the controller is still waiting it to finish, before giving the task to the next instance. This basically causes the adaptive process to freeze. This doesn't happen very often, but can triggered by resizing the window a lot. This is the bug that is currently present in the master branch. The solution to this issue is to make the code for selecting the current/next task more robust, and also add some reactive code to handle when the currently active adapting instance is removed. This PR does that.
At this point, no further problems have been detected, and the current mechanism (with this fix) seems to work reliably, but more testing is always welcome.