fix(aio): revert resize to HostListener; delete device svc by wardbell · Pull Request #16143 · angular/angular

wardbell · 2017-04-19T04:44:38Z

Angular change detection bug -> no page update on resize.
Reverting to @HostListener('window:resize', ['$event.target.innerWidth']) cures it.
Delete DeviceService which no longer serves a purpose.
Adjusted affected AppComponent and LiveExample tests.

To see bug, checkout the 2nd commit from PR #15887 (or any subsequent commit before this PR) and do the following:

Make browser narrower than 1032 (the trigger width)
Nav to any guide page
Note that there are no top menu items
Make browser wider than 1032.
Note that there are still no top menu items.
Click anywhere on the page. Now the top menu and side nav appear ... as they should have after resize.

Set a breakpoint in AppComponent#onResize

  onResize(width) {
    this.isSideBySide = width > this.deviceService.sideBySideWidth;
  }

This will confirm that the event is caught and propagated via DeviceService to the method. The value of this.isSideBySide becomes true, the template binding should trigger nav panels to appear. But they don't. ChangeDetectorRef.detectChanges() and setTimeout tricks are fruitless.

I gave up wondering why. Reversion to the following gives the expected behavior.

  @HostListener('window:resize', ['$event.target.innerWidth'])
  onResize(width) {
    this.isSideBySide = width > this.sideBySideWidth;
  }

Project timeline dictates moving forward and leaving the lingering questions unanswered.

Please check if the PR fulfills these requirements

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce? (check one with "x")

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

mary-poppins · 2017-04-19T04:56:40Z

The angular.io preview for 6c539976ad4e1cecb88472ddb555a1eefa2e659b is available here.

Angular change detection bug -> no page update on resize. Reverting to `@HostListener('window:resize', ['$event.target.innerWidth'])` cures it. Delete DeviceService which no longer serves a purpose. Adjusted affected AppComponent and LiveExample tests.

mary-poppins · 2017-04-19T07:30:33Z

The angular.io preview for 06aef2d is available here.

gkalpak · 2017-04-19T09:32:16Z

@@ -62,6 +51,8 @@ describe('LiveExampleComponent', () => {
        liveExampleDe.nativeElement.liveExampleContent = liveExampleContent;

        fixture.detectChanges();


Is this necessary?

gkalpak · 2017-04-19T09:35:30Z

-    }
-  }
-
-  onResize(width?: number) {


Couldn't you use @HostListener here? Would that work?

No. I tried it. Angular does not activate that decorator on a service. :( I think it should but that would be a new feature.

IgorMinar · 2017-04-19T13:27:58Z

I reduced the original issue to angular/zone.js#741, but I do agree with @gkalpak that the host listener solution is WAY cleaner.

JiaLiPassion · 2017-04-19T14:37:01Z

@wardbell , @IgorMinar , I just tried angular/aio, I think maybe the problem is angular/aio/package.json still use [email protected], after upgrade to [email protected], the problem is gone. Because in [email protected], window.onProperty is also patched by zone.js with this PR,angular/zone.js#687.

gkalpak · 2017-04-19T14:48:34Z

@JiaLiPassion, angular/zone#687 still doesn't seem to patch window.onresize (which is what we care about here).

JiaLiPassion · 2017-04-19T15:10:06Z

@gkalpak , I just tried with the example here like this.

<!DOCTYPE html>
<html>
<head>
  <script src="./zone.js"></script>
  <script>
    Zone.current.fork({name: 'resize'}).run(function() {
      window.onresize = function() {
        console.log('Zone name is ', Zone.current.name);
      }
    });
  </script>
</head>
<body>
</body>
</html>>

The output is

Zone name is resize

And I also tried angular/aio, and debug at DeviceService

export class DeviceService {

  // Show sidenav next to the main doc when display width on current device is greater than this.
  readonly sideBySideWidth = 1032;

  displayWidth = new ReplaySubject<number>(1);

  constructor() {

    if (window) {
      window.onresize = () => this.onResize();
      this.onResize();
    } else {
      // when no window, pretend the display is wide.
      this.onResize(this.sideBySideWidth + 1);
    }
  }

window.onresize is patched by zone.js and the Zone.current.name is angular.

Here is the screenshot.

gkalpak · 2017-04-19T15:16:58Z

@JiaLiPassion, you are right. I assumed that patchOnProperty would only patch the properties passed to it as a second argument, but it turns out is patches all on... properties it can find on the target.

In that case, upgrading to zone.js 0.8.5 should indeed solve the issue.

JiaLiPassion · 2017-04-19T15:20:14Z

@gkalpak , yes, it will loop all properties which starts with on, and try to patch them.
I have tried with the commit before this PR, with zone.js 0.8.4, resize not work, after upgrade to 0.8.5, it worked.

Please check it. Thanks!

IgorMinar · 2017-04-19T15:20:43Z

@JiaLiPassion great work! Thank you! We'll still go ahead with merge in this PR which simplifies our code greatly, but it's good to know that the newer version of zones doesn't suffer from this problem.

JiaLiPassion · 2017-04-19T15:21:33Z

@IgorMinar , sure, glad to help!

IgorMinar · 2017-04-20T09:45:59Z

That's not so easy. Host listeners need to be associated with host element. Services don't have any element associated with them so I don't know how this would work. Defaulting to window is not a good option.

…

On Wed, Apr 19, 2017, 11:49 PM Ward Bell ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In aio/src/app/shared/device.service.ts <#16143 (comment)>: > - readonly sideBySideWidth = 1032; - - displayWidth = new ReplaySubject<number>(1); - - constructor() { - - if (window) { - window.onresize = () => this.onResize(); - this.onResize(); - } else { - // when no window, pretend the display is wide. - this.onResize(this.sideBySideWidth + 1); - } - } - - onResize(width?: number) { No. I tried it. Angular does not activate that decorator on a service. :( I think it should but that would be a new feature. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#16143 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AANM6Dz-WP_zZ9fgpVpAan4GCBNR5Ec1ks5rxoFQgaJpZM4NBMVM> .

…6143) Angular change detection bug -> no page update on resize. Reverting to `@HostListener('window:resize', ['$event.target.innerWidth'])` cures it. Delete DeviceService which no longer serves a purpose. Adjusted affected AppComponent and LiveExample tests. PR Close angular#16143

angular-automatic-lock-bot · 2019-09-11T09:22:25Z

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.}

wardbell added the comp: aio label Apr 19, 2017

wardbell self-assigned this Apr 19, 2017

googlebot added the cla: yes label Apr 19, 2017

wardbell changed the title ~~fix(air): revert resize to HostListener; delete device svc~~ fix(aio): revert resize to HostListener; delete device svc Apr 19, 2017

wardbell force-pushed the aio-fix-resize-logic branch from 6c53997 to 06aef2d Compare April 19, 2017 07:17

wardbell added the action: review The PR is still awaiting reviews from at least one requested reviewer label Apr 19, 2017

gkalpak approved these changes Apr 19, 2017

View reviewed changes

IgorMinar mentioned this pull request Apr 19, 2017

window.onresize is not intercepted by zones angular/zone.js#741

Closed

IgorMinar self-requested a review April 19, 2017 13:28

IgorMinar approved these changes Apr 19, 2017

View reviewed changes

IgorMinar added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Apr 19, 2017

mhevery closed this in f99cb96 Apr 20, 2017

wardbell deleted the aio-fix-resize-logic branch April 21, 2017 18:34

angular-automatic-lock-bot Bot locked and limited conversation to collaborators Sep 11, 2019

		@@ -62,6 +51,8 @@ describe('LiveExampleComponent', () => {
		liveExampleDe.nativeElement.liveExampleContent = liveExampleContent;

		fixture.detectChanges();

Conversation

wardbell commented Apr 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mary-poppins commented Apr 19, 2017

Uh oh!

mary-poppins commented Apr 19, 2017

Uh oh!

gkalpak Apr 19, 2017

Choose a reason for hiding this comment

Uh oh!

gkalpak Apr 19, 2017

Choose a reason for hiding this comment

Uh oh!

wardbell Apr 19, 2017

Choose a reason for hiding this comment

Uh oh!

IgorMinar commented Apr 19, 2017

Uh oh!

JiaLiPassion commented Apr 19, 2017

Uh oh!

gkalpak commented Apr 19, 2017

Uh oh!

JiaLiPassion commented Apr 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gkalpak commented Apr 19, 2017

Uh oh!

JiaLiPassion commented Apr 19, 2017

Uh oh!

IgorMinar commented Apr 19, 2017

Uh oh!

JiaLiPassion commented Apr 19, 2017

Uh oh!

IgorMinar commented Apr 20, 2017 via email

Uh oh!

angular-automatic-lock-bot Bot commented Sep 11, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

wardbell commented Apr 19, 2017 •

edited

Loading

JiaLiPassion commented Apr 19, 2017 •

edited

Loading