fix(material/tabs): ensure the ink bar realigns when the tab header changes dimensions

[hilts-vaughan]

This should fix #6130 and also #4648?. It uses ResizeObserver to ensure we get notifications about the dimensions of the tabs when they change. The layout change will fire synchronously.

It may also fix #3048.

[crisbeto]

This needs to be feature detected since it could run in a server environment.

[hilts-vaughan]

Does Angular have their own mechanism for this or can I just do a window.ResizeObserver check?

[crisbeto]

You can get it from this._platform.isBrowser, although I think I'd prefer to use typeof ResizeObserver === 'function' since it would avoid errors on browsers that don't support ResizeObserver.

[hilts-vaughan]

I added a check, thanks.

[crisbeto]

You can get it from this._platform.isBrowser, although I think I'd prefer to use typeof ResizeObserver === 'function' since it would avoid errors on browsers that don't support ResizeObserver.

[hilts-vaughan]

I've cleaned it up a bit. If you have suggestions for the test, I would love to hear them. We can also do some other kind of test or just not a write at all if you're worried about it introducing flakiness if the component is expected to not be trouble in the future.

[crisbeto]

It seems like the CI didn't run for some reason 😕

[crisbeto]

This still hasn't been changed to runOutSizeAngular.

[hilts-vaughan]

Sorry, I forgot to push.

[crisbeto]

I don't think that we should be stubbing out an observable. You can use the EMPTY observable from rxjs instead.

[hilts-vaughan]

We're not. We're stubbing out the RO function. I could also return null and check for null down below. I don't think EMPTY works.

[crisbeto]

Hmm, I see. I find this side-effect-ey use of map confusing. Here's how I would've implemented it:

1. Keep the merge(dirChange, resize, this._items.changes) above as is.
2. Have a utility that looks like this:

private _itemsResized(): Observable<void> {
  if (typeof ResizeObserver !== 'function') {
    return EMPTY;
  }

  return this._items.changes.pipe(
    startWith(this._items),
    switchMap(
      (items: QueryList<MatPaginatedTabHeaderItem>) =>
        new Observable((observer: Observer<void>) =>
          this._ngZone.runOutsideAngular(() => {
            const resizeObserver = new ResizeObserver(() => observer.next());
            items.forEach(item => resizeObserver.observe(item.elementRef.nativeElement));
            return () => resizeObserver.disconnect();
          }),
        ),
    ),
    // Skip the first emit since the resize observer emits when an item is observed.
    skip(1),
  );
}

3. Change the merge call above to merge(dirChange, resize, this._items.changes, this._itemsResized()).

This approach fixes the following issues:

1. You generally don't want side effects from your map operator.
2. The ResizeObserver wasn't being disconnected when the subscription is dropped.
3. It avoids the extra call when the elements are observed for the first time.
4. The ResizeObserver itself wasn't being created outside the NgZone.

Note that this is slightly more wasteful when it comes to recreating resize observers, but I think that it's not a big deal in this case since tabs don't tend to change very often. If it becomes a problem, we can start keeping track of which items are being observed.

[hilts-vaughan]

I adopted a similar implementation. I did add an "startWith" at the end to fire a single emission which is what the old code was doing. This is still important for the initial layout IIRC.

[crisbeto]

You can reintroduce the first render by removing the skip(1). I added it, because the first emit seems redundant to me. It fires when the elements are observed for the first time, but the initial render should already be correct since we do it right after the elements are inserted.

[hilts-vaughan]

Removing skip will fire it on the RO layout which causes the rest to fail because as it turns out, the fakeAsyncZone doesn't actually capture the ResizeObserver tick (even though it technically runs before the paint, it's not before the call stack is cleared). This explains why that test passed and was not flaky.

We could remove the test and not bother or perhaps try to a stability check hack to fix that if you don't think the first synchronous emission is needed.

[crisbeto]

Okay, that explains why the test was passing. I'd rather avoid the test than have it fire on init, because it has the potential to cause layout thrashing.

[hilts-vaughan]

OK, I removed the test for now to keep it simple especially since MDC is coming "soon". I talked with some internal folks and these links should be OK to share:

Before: https://screencast.googleplex.com/cast/NTA3MjkzNjU0MjE0MjQ2NHw0NTU1YzBmNi1lOA
After: https://screencast.googleplex.com/cast/NjI3MjI0NjU2Njg3OTIzMnw0MjBiMDcxZS0zNQ

This shows the stuff working in lieu of some tests.

[crisbeto]

Shouldn't we get rid of the startWith(undefined) as well?

[hilts-vaughan]

It should be gone. I hadn't realize that pushing would immediately send it for re-review, unlike some other tools. :)

[crisbeto]

So the change LGTM, but for some reason the CI didn't run. Can you try cherry-picking the commit to a new branch and opening another PR?

[hilts-vaughan]

The CI system says it's missing some labels. Do I need to add some labels on the new PR? Or this one perhaps?

[crisbeto]

The label checks are separate. Usually it queues up another 11 jobs that don't show up here for some reason. This has happened for other people and CircleCI haven't been able to figure out why. There was a theory that it might be because you aren't watching the fork that you're opening the PR from.

[hilts-vaughan]

Opened #24932. I'll adjust the description etc if the CI runs.

[hilts-vaughan]

(It also tells me that the CI workflow is pending approval and links me to https://docs.github.com/en/actions/managing-workflow-runs/approving-workflow-runs-from-public-forks -- claiming a maintainer has to approve my CI run on this PR)

[crisbeto]

Did you try clicking the "Watch" button on your fork? Also the "Approve and run" button is only for deploying the dev app, it doesn't affect any of the other checks. You can check what other PRs look like, e.g. #24931

[hilts-vaughan]

I swapped the watch to "All Activity" just now.

[crisbeto]

Then I really don't know why it didn't run 😕. Anyway, I ran it locally and the only problem is this lint failure:

src/material/tabs/paginated-tab-header.ts:258:3
ERROR: 258:3  jsdoc-format  jsdoc is not formatted correctly on this line

[hilts-vaughan]

I tried fix it locally. Want me to run something else? Should I abandon #24932?

[crisbeto]

Yeah, you can abandon the other one.

[crisbeto]

Caretaker note: for some reason the CI didn't run on this PR. I've verified that everything passes on my machine.

[angular-automatic-lock-bot]

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}