feat(ivy): properly apply style="", [style], [style.foo] and [attr.style] bindings

[matsko]

No description provided.

[mary-poppins]

You can preview 4a50cd9 at https://pr24602-4a50cd9.ngbuilds.io/.

[matsko]

@mhevery please look at this line and give feedback.

[mhevery]

Not sure I understand your question. But what you are doing in code seems right.

[mary-poppins]

You can preview 07214ca at https://pr24602-07214ca.ngbuilds.io/.

[trotyl]

Could this be unified in @angular/core and being shared?

Core:

angular/packages/core/src/view/services.ts

Lines 475 to 477 in cb31381

	function camelCaseToDashCase(input: string): string {
	return input.replace(CAMEL_CASE_REGEXP, (...m: any[]) => '-' + m[1].toLowerCase());
	}

AnimationBrowser (Opposite only):

angular/packages/animations/browser/src/util.ts

Lines 230 to 232 in cb31381

	export function dashCaseToCamelCase(input: string): string {
	return input.replace(DASH_CASE_REGEXP, (...m: any[]) => m[1].toUpperCase());
	}

Compiler (Opposite only):

angular/packages/compiler/src/util.ts

Lines 16 to 18 in cb31381

	export function dashCaseToCamelCase(input: string): string {
	return input.replace(DASH_CASE_REGEXP, (...m: any[]) => m[1].toUpperCase());
	}

PlatformBrowser (both direction):

angular/packages/platform-browser/src/dom/util.ts

Lines 15 to 21 in cb31381

	export function camelCaseToDashCase(input: string): string {
	return input.replace(CAMEL_CASE_REGEXP, (...m: string[]) => '-' + m[1].toLowerCase());
	}
	export function dashCaseToCamelCase(input: string): string {
	return input.replace(DASH_CASE_REGEXP, (...m: string[]) => m[1].toUpperCase());
	}

Element:

angular/packages/elements/src/utils.ts

Lines 53 to 55 in cb31381

	export function camelToDashCase(input: string): string {
	return input.replace(/[A-Z]/g, char => `-${char.toLowerCase()}`);
	}

[alfaproject]

It could even be exported to the outside world. We have our own version of this as well, and I'm sure a lot of people do too. (:

[mhevery]

Please, don't export it to the outside world. It seems like a good idea on a surface, but it becomes part of public API which has to be maintained, deprecated, documented, etc...

[mhevery]

Unification is hard, since compiler needs to compile core hence it can't depend on core.

[trotyl]

But it should at least be placed into angular/packages/compiler/src/util.ts along with its counterpart, shouldn't it?

[trotyl]

by an styles?

[alfaproject]

Did anything change on Ivy status document?

[mary-poppins]

You can preview e544ee6 at https://pr24602-e544ee6.ngbuilds.io/.

[mhevery]

I think the names should be consistent in the prefix. So first word should be element second style and last the sub-action. How about:

elementStyle <= elementInitStyiling
elementStyleMap <= elementStyleMulti
elementStyleProp <= elementStyleSingle
elementStyleApply: => more consistent with i18nApply instruction

Also rename the short name appropriately. so ɵsi not ɵis, etc...

[mhevery]

Here is my initial pass.

[mhevery]

The word string is unnecessary, also the return type is not needed. Should just be parseStyle

[mhevery]

since parenCount is a number we should say pranCount === 0 since that would be more readable. same for currentQuote

[mhevery]

rename to parenDepth

[mhevery]

I think you should convert this to const enum

const enum CharCode {
  ParenOpen = 40,
  ParenClose = 41,
...
}

NOTE: Put Close as last so that they sort alphabetically next to each other.

[mhevery]

rename currentQuote to const quoteMode:0|39|34 = 0; So that it is clear what mode we are in.

You could go all out like this:

const enum QuoteMode {
  None = 0,
  Singe = 39
  Double = 34
}

const quoteMode: QuoteMode

[mhevery]

Not sure I understand your question. But what you are doing in code seems right.

[mhevery]

This is public API, so it needs a much mored detailed doc section.

[mhevery]

Public API, needs detailed docs section.

[mhevery]

Use for over forEach for perf code.

[mhevery]

This needs to move to where it is needed. We don't always need to read it.
I see that lElement.native is used in many places. Should be pulled out to local var.

[mary-poppins]

You can preview 578c392 at https://pr24602-578c392.ngbuilds.io/.

[mary-poppins]

You can preview 0dbac5d at https://pr24602-0dbac5d.ngbuilds.io/.

[mary-poppins]

You can preview adb6ff2 at https://pr24602-adb6ff2.ngbuilds.io/.

[mary-poppins]

You can preview 7bb9f4b at https://pr24602-7bb9f4b.ngbuilds.io/.

[mhevery]

Hey, I think we need to do one more perf optimization. I wrote it up in detail, and I can pair with you tomorrow if you need more explanations or help implementing it.

[mhevery]

I thought we agreed that elementStyle => elementStyling?
we can than rename elementStyleMap => elementStyle

[mhevery]

1. you are converting the array into object literal which is not efficient.
2. You are creating the same object literal for each instance of the template.

see comment below.

[mhevery]

I am looking at the initial, multi and single and I am not liking the fact that we have object literals which we have to enumerate every time the code executes, which is slow. It would be much better if all of them would be stored as arrays. This way the iteration over them would be much quicker. Something like this:

interface ElementStyleRecord {
  update: boolean;
  initial: string[];
  multi: string[];
  single: string[];
}

However initial is already an array so we should just use it.
The next issues is that we will have too many arrays, and they are somewhat expensive (70 bytes per array). Could we just have single array. (https://github.com/angular/angular/blob/master/packages/core/src/render3/PERF_NOTES.md)

const enum StyleFlags {
  None = b0000; 
  Multy = b0001;  // Marker that this is part of the Multi section
  Dirty = b0010;   // Marker when a new style arrives and will need to be flushed/applied
  Remove = b0100; // Marker when the style has been remove and will need to be flushed/applied
}

interface ElementStyleRecord extends Array<string>{
  [0]: string[]; // Pointer to the initial array data.
  [1]: StyleFlags; // A single place to check if any other flags are dirty
}

So imagine that you have:

initial: ['color': 'red'];
multi: ['color': 'blue', width: '10px'];
single: ['height': '10px'];

this would be:

const initialConst = ['color', 'red'];
styleRecord = [
  initialConst, 
  StyleFlags.Dirty,
  StyleFlags.Multi | StyleFlags.Dirty, 'color', 'blue', 
  StyleFlags.Multi | StyleFlags.Dirty, 'width', '10px', 
  StyleFlags.Dirty, 'height': '10px'];

Applying this to element would be easy since you would just have to apply properties from left to right, letting style be overwrite as needed. The order of array would be the order of overwriting. Since overwrites are relatively rare, you would not even need to track them, you could just write the style twice.

Once the apply method would run the data structure would be processed from left to right and the flags would be updated like so:

const initialConst = ['color', 'red'];
styleRecord = [
  initialConst, 
  StyleFlags.None,
  StyleFlags.Multi, 'color', 'blue', 
  StyleFlags.Multi, 'width', '10px', 
  StyleFlags.Single, 'height': '10px'];

By updating the flags we know that we don't have to write to element more than is necessary.

A removal would be marked as so.

const initialConst = ['color', 'red'];
styleRecord = [
  initialConst, 
  StyleFlags.Dirty,
  StyleFlags.Multi | StyleFlags.Remove, 'color', 'red', 
  StyleFlags.Multi, 'width', '10px', 
  StyleFlags.Single, 'height': '10px'];

And after update the data structure would look like so:

const initialConst = ['color', 'red'];
styleRecord = [
  initialConst, 
  StyleFlags.None,
  StyleFlags.Note, null, null, 
  StyleFlags.Multi, 'width', '10px', 
  StyleFlags.Single, 'height': '10px'];

NOTE: we don't have to shrink the array since it is likely that the style will come back. So we just leave a hole there. (But we would have to grow it if new styles show up.

[mary-poppins]

You can preview daef3f8 at https://pr24602-daef3f8.ngbuilds.io/.

[mary-poppins]

You can preview 4f4ad9e at https://pr24602-4f4ad9e.ngbuilds.io/.

[alfaproject]

You can merge these 2 cases for smaller code generation:

      case Char.QuoteSingle:
      case Char.QuoteDouble:
        // valueStart needs to be there since prop values don't
        // have quotes in CSS
        valueHasQuotes = valueHasQuotes || valueStart > 0;
        if (quote === Char.QuoteNone) {
          quote = token;
        } else if (quote === token && value.charCodeAt(i - 1) !== Char.BackSlash) {
          quote = Char.QuoteNone;
        }
        break;

[mary-poppins]

You can preview fdc0343 at https://pr24602-fdc0343.ngbuilds.io/.

[trotyl]

This util could possibly benefit #19645 as well, should it be ivy-only?

[trotyl]

Any reason for not using ===?

[mhevery]

fixed

[trotyl]

Should there be a semicolon for consistency?

[trotyl]

an StylingContext -> a StylingContext? (Also in following instructions)

[googlebot]

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

[mary-poppins]

You can preview cb2253a at https://pr24602-cb2253a.ngbuilds.io/.

[googlebot]

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

[mary-poppins]

You can preview 852e3a0 at https://pr24602-852e3a0.ngbuilds.io/.

[googlebot]

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

[mhevery]

http://test/OCL:203490346:BASE:203490360:1530896056601:18fec9ae

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