Not all custom responsive directives work as expected

[Mufasa]

Bug Report

What is the expected behavior?

My customised responsive directives (fxLayout.x, fxHide.x, fxShow.x, fxFlex.x, and fxLayoutGap.x) should work as documented.

What is the current behavior?

• fxLayout.x directives seem to work as documented.
• fxHide.x directives seem to work as documented.
• fxShow.x directives do not seem to fire as expected but I have raised this as a separate issue: fxShow does not seem to work with custom breakpoints #758
• fxFlex.x directives do not seem to fire as expected. It always seems to pick the setting from fxFlex="...".
• fxLayoutGap.x directives do not seem to fire as expected. It always seems to pick the setting from fxLayoutGap="...". I have made some notes in the file src/app/app.component.html to note some odd behaviour of this directive although it may be related to error being thrown - see bottom of this issue for details.

What are the steps to reproduce?

StackBlitz example that demonstrates the issues: https://stackblitz.com/edit/flexlayout-eg9xpj

As you widen and narrow the browser window you should see the following custom breakpoints fire (defined in src/app/custom-breakpoints.ts):

import { BREAKPOINT } from '@angular/flex-layout';

const MOBILE = '(max-width: 599px)';
const S_TABLET = '(min-width: 600px) and (max-width: 719px)';
const L_TABLET = '(min-width: 720px) and (max-width: 1023px)';
const L_TABLET_LANDSCAPE = '(min-width: 1024px) and (max-width: 1439px)';
const WEB = '(min-width: 1440px)';

const SCREEN_TYPES = {
  MOBILE: `${MOBILE}`,
  S_TABLET: `${S_TABLET}`,
  L_TABLET: `${L_TABLET}`,
  L_TABLET_LANDSCAPE: `${L_TABLET_LANDSCAPE}`,
  WEB: `${WEB}`,
};

export const CUSTOM_BREAKPOINTS = [
  { 'alias': 'mobile', 'mediaQuery': SCREEN_TYPES.MOBILE },
  { 'alias': 's.tablet', 'mediaQuery': SCREEN_TYPES.S_TABLET },
  { 'alias': 'l.tablet', 'mediaQuery': SCREEN_TYPES.L_TABLET },
  { 'alias': 'l.tablet.landscape', 'mediaQuery': SCREEN_TYPES.L_TABLET_LANDSCAPE },
  { 'alias': 'web', 'mediaQuery': SCREEN_TYPES.WEB }
];

However, they only seem to fire correctly for fxLayout.x and fxHide.x directives.

The file src/app/app.component.html contains all the test HTML code.

What is the use-case or motivation for changing an existing behavior?

All these directive should respect custom breakpoints as documented.

Which versions of Angular, Material, OS, TypeScript, browsers are affected?

Flex-Layout v6.0.0-beta.16
Angular 6.0.3
Material 6.2.0
macOS High Sierra v10.13.4
Chrome Version 66.0.3359.181 (Official Build) (64-bit)

Is there anything else we should know?

In addition, if you look in the browsers Console then you will se these errors being reported intermittently as you adjust the browser windows width:

errors.ts:35 ERROR TypeError: Cannot read property 'addFlexToParent' of undefined
    at CustomFlexDirective.FlexDirective._validateValue (flex.ts:167)
    at CustomFlexDirective.FlexDirective._updateStyle (flex.ts:156)
    at eval (flex.ts:126)
    at ResponsiveActivation.eval [as _onMediaChanges] (base.ts:215)
    at ResponsiveActivation._onMonitorEvents (responsive-activation.ts:168)
    at SafeSubscriber.eval [as _next] (responsive-activation.ts:136)
    at SafeSubscriber.__tryOrUnsub (Subscriber.ts:270)
    at SafeSubscriber.next (Subscriber.ts:212)
    at Subscriber._next (Subscriber.ts:142)
    at Subscriber.next (Subscriber.ts:102)

[CaerusKaru]

The reason for the error on addFlexToParent is because you're passing undefined as the last argument in your super call in the CustomFlexDirective. Additionally, the LayoutDirective needs the SkipSelf decorator, not the Self decorator.

I can't replicate the behavior you're seeing for the FlexDirective. I am still investigating the other directives, but this may solve most of your issues.

[Mufasa]

@CaerusKaru I have updated my StackBlitz as advised (thanks for finding my mistakes). This fixes the custom fxFlex directives behaviour, so the only remaining ones with issues seem to be custom fxShow and fxLayoutGap directives.

Thanks again.

[CaerusKaru]

@Mufasa There's a lot to unpack in your StackBlitz demo. I've been unable to reproduce the fxLayoutGap issues you're facing locally in my own testing. I've kept the other fxShow issue open because I haven't been able to fully investigate that yet, but will post notes there.

Can you isolate the fxLayoutGap issue you're facing into a succinct StackBlitz?

[Mufasa]

@CaerusKaru Here is a StackBlitz that demonstrates the fxLayoutGap issue: https://stackblitz.com/edit/flexlayout-2al3cr

It looks like it only has issues when mixed in with fxLayout. I added two other sections that each have a fixed fxLayout and you can see that the fxLayoutGap directives in these sections work fine.

The top section has responsive fxLayout and responsive fxLayoutGap and that does not seem to work when the layout changes from row to column.

[charsleysa]

@Mufasa doing a quick investigation into this I can see that the reason a lot of issues are occurring is due to the nature of how directives work in Angular and there being conflicting updates.

For example, the issue with fxHide is caused by the fact the code for the base directive fxHide will run before your custom directive and the base directive modifying styles before your custom directive has a chance to read the default styles of the element. (Related #758)

So what occurs is the following:

1. the fxHide directive loads and reads the default value of the display style (which is usually block)
2. the fxHide directive then sets the display to none (as it's supposed to do)
3. your custom directive loads and reads the default value of the display styles (which is now none)
4. your custom directive then sets the display value to the default value instead of none which means it's supposed to be showing (but since the default is none it will never show)

In my opinion, the base directives need to expose an API which can be consumed by our custom directives instead of our custom directives extending the base directives so that our directives are just passing through extra values for the custom breakpoints instead of re-executing the functionality (which is causing conflicts).

I would envision something like this which would make adding custom breakpoints really simple;

@Directive({
  selector: `
    [fxLayoutGap.mobile],
    [fxLayoutGap.tablet],
    [fxLayoutGap.web]
  `
})
export class CustomLayoutGapDirective  {
  constructor(@Self() private _directive: LayoutGapDirective) {
    // Would pass the current instance of the custom directive to the current instance of the
    // base directive so that the base directive could connect / register / whatever it needed
    _directive.registerCustomDirective(this);
  }

  @Input('fxLayoutGap.mobile')
  set gapMobile(val) { this._directive.addCacheInput('gapMobile', val); }
  @Input('fxLayoutGap.tablet')
  set gapTablet(val) { this._directive.addCacheInput('gapTablet', val); }
  @Input('fxLayoutGap.web')
  set gapWeb(val) { this._directive.addCacheInput('gapWeb', val); }
}

[CaerusKaru]

@charsleysa @Mufasa the show-hide behavior was a nasty side-effect, and Stefan if only you posted this before I went down the rabbit hole that was #798, you would've saved me so much time. Regardless, that behavior was patched in #891. The other directives probably still need investigation for similar behavior.

I like your solution, but it has one problem: it still requires a user to register a default breakpoint on their host element. A user should be able to only specify one out of all registered directives and get intended behavior. Ideally, this would mean that they create a custom directive that extends the default directive, and then instead of importing FlexLayoutModule, they would import CoreModule, and then declare their own custom directive (which they would have to do anyway).

The real ideal solution is to have a centralized style marshaller that can act as both data store for all elements and style generator. Unfortunately the implementation for such an approach is very complex and time-consuming, and may not be worth the effort. If you want to try, I'd be more than happy to offer some tips to get started and would of course review any PR submitted.

[CaerusKaru]

@Mufasa Once #900 lands, can I count on you to test out the new API for custom breakpoint directives? It'll probably get merged early next week.

[Mufasa]

@CaerusKaru Certainly - I'll be glad to help, just ping me when you want me to start testing please.