fix(tabs): avoid width calculation deviations. #8293
Conversation
* Currently when using centered tabs, the `updateInkBarStyles` method is getting executed infinitely (even without $$rAF, sync) This is impacting the performance significant, because we're calculating the styles on each function call. The problem is, that the pagination wrapper uses another rounding process than the `updateInkBarStyle` method. Now both calculations are using the same rounding process / logic and the width's are now correct. Fixes angular#8289.
| } | ||
|
|
||
| function calcTabsWidth(tabs) { | ||
| var width = 0; |
There was a problem hiding this comment.
@robertmesserle Not sure, why the width was initially at 1px?
There was a problem hiding this comment.
AFAIR, there was a specific reasoning for this.
There was a problem hiding this comment.
I do not recall the reason at all, unfortunately.
There was a problem hiding this comment.
I believe this was to account for a wrapping bug regarding pagination where the last item was wrapping to a new line without it; however, this looks like a band-aid that was hastily thrown in rather than diagnosing the root cause.
There was a problem hiding this comment.
I'll also note that this is added in response to issues with tabs living inside of dialogs, so you might try creating that sort of test case and see if any bugs show up.
|
Have you tried this in IE/Edge? Maybe it would help with #4940 as well. |
|
@crisbeto Thanks for the idea - but unfortunately it's not related to that issue, because this bug is only appearing when using |
|
@robertmesserle - can you review this please. |
|
@ThomasBurleson @devversion LGTM, but I'd like to confirm with a demo of tabs inside of a dialog before merging - just to confirm that |
|
@devversion - can you create a codePen with a dialog that contains tabs ? |
|
@ThomasBurleson Yeah, it works as expected But unfortunately with |
Initially, the tabs used DOMSubtreeModified to listen for changes and update the height, pagination and inkbar styles. At some point, we used a different method to update the height, but we were still watching all DOM changes and updating the pagination and inkbar styles whenever the contents changed. Fix this with the following two changes: 1. Move the DOM watching to a new md-tabs-dummy-wrapper directive which only watches for changes to the tab's labels (not the content too) so that the updates are called less frequently. 2. Replace DOMSubtreeModified watching with new MutationObservers which are more drastically more performant. Additionally, some recent changes caused some bugs in the tabs by using a cached version of the internal elements instead of calling `getElements()` which searches the DOM and refreshes the cached elements. Fix by adding more calls to `getElements()` where appropriate to properly update the cache after tabs may have changed. Fixes angular#5681. References angular#4940, and might cause PR angular#8293 to need a rebase if this goes in first.
Initially, the tabs used DOMSubtreeModified to listen for changes and update the height, pagination and inkbar styles. At some point, we used a different method to update the height, but we were still watching all DOM changes and updating the pagination and inkbar styles whenever the contents changed. Fix this with the following two changes: 1. Move the DOM watching to a new md-tabs-dummy-wrapper directive which only watches for changes to the tab's labels (not the content too) so that the updates are called less frequently. 2. Replace DOMSubtreeModified watching with new MutationObservers which are more drastically more performant. Additionally, some recent changes caused some bugs in the tabs by using a cached version of the internal elements instead of calling `getElements()` which searches the DOM and refreshes the cached elements. Fix by adding more calls to `getElements()` where appropriate to properly update the cache after tabs may have changed. Fixes #5681. References #4940, and might cause PR #8293 to need a rebase if this goes in first. Closes #8496
|
@topherfangio Can you take a look at this PR as well? (since you recently made some calculation changes) |
|
@devversion Code looks good, but it seems this introduces a small issue in Safari. In the "Dynamic Tabs" demo, if you add a new tab (I named mine "woot"), it does not properly scroll it into view. It seems to work fine in master on Safari, and it also works correctly in Chrome/Firefox with and without your PR, so I'm not sure why it's different in Safari. If we can figure this out and get it fixed, I think it's good to go.
Note: The tab is indeed there and you can scroll it into view using the arrows; it just doesn't appear immediately. |
|
@ThomasBurleson Should be ready for review/merge. I chatted with @devversion and realized my version of Safari is older and he can't reproduce the issue I mentioned above on the latest version, so I think it's good to go. |
Splaktar
added
pr: merge ready
updateInkBarStylesmethod is getting executed infinitely (even without $$rAF, sync)This is impacting the performance significant, because we're calculating the styles on each function call.
The problem is, that the pagination wrapper uses another rounding process than the
updateInkBarStylemethod.Now both calculations are using the same rounding process / logic and the width's are now correct.
Fixes #8289.