refactor(ivy): Switch styling to reconcile algorithm by mhevery · Pull Request #34616 · angular/angular

mhevery · 2020-01-02T17:00:31Z

refactor(ivy): Switch styling to reconcile algorithm

This change deletes old styling code and replaces it with a simplified styling algorithm.

The mental model for the new algorithm is:

Create a linked list of styling bindings in the order of priority. All styling bindings ere executed in compiled order and than a linked list of bindings is created in priority order.
Flush the style bindings at the end of advance() instruction. This implies that there are two flush events. One at the end of template advance instruction in the template. Second one at the end of hostBindings advance instruction when processing host bindings (if any).
Each binding instructions effectively updates the string to represent the string at that location. Because most of the bindings are additive, this is a cheap strategy in most cases. In rare cases the strategy requires removing tokens from the styling up to this point. (We expect that to be rare case) Because, the bindings are presorted in the order of priority, it is safe to resume the processing of the concatenated string from the last change binding.

PR Checklist

Please check if your PR fulfills the following 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)

PR Type

What kind of change does this PR introduce?

Bugfix
Feature
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Documentation content changes
angular.io application / infrastructure changes
Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

Yes
No

Other information

googlebot · 2020-01-03T00:40:21Z

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

ℹ️ Googlers: Go here for more info.

googlebot · 2020-01-03T21:15:26Z

All (the pull request submitter and all commit authors) CLAs are signed, but 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 by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

pkozlowski-opensource

Another batch of reviews - this time mostly for the existing acceptance tests (packages/core/test/acceptance/styling_spec.ts)

pkozlowski-opensource · 2020-01-21T10:50:38Z

@@ -829,6 +1190,14 @@ describe('styling', () => {
            expect(element.style.opacity).toBeFalsy();


The 2 above asserts don't look right too me (and sound like a breaking change wrt to VE). The previous contract was that any falsy value meant "remove" while now it seems to mean "I don't have an opinion". But this doesn't totally right to me as a developer has an opinion.

I think this change counter-intuitive and breaking. Could we discuss it more?

yes this is technically a breaking change.

you can think of null vs undefined as a new feature. It is being introduced now, because doing it later will be harder, because as people will start using priorities they will use null or undefined interchangeably which will make introducing this feature harder.

Let's discuss on VC with @kara

pkozlowski-opensource · 2020-01-21T10:59:57Z

-            // there is no need to flush styling since the styles are applied directly
-            expect(ngDevMode !.flushStyling).toEqual(0);
+            // once for the template flush and again for the host bindings
+            expect(ngDevMode !.flushStyling).toEqual(2);


nit: most of this test should pass in VE as well - maybe remove onlyInIvy from it and only guard this assert ?

It is not clear to me which onlyInIvy do you want me to remove (Should I remove all and see which tests pass?)

I thought per earlier comment you did not like when I put conditional asserts between VE/Ivy.

Sorry I am just confused as to what I should do here.

pkozlowski-opensource · 2020-01-21T13:46:32Z

        fixture.detectChanges();

-        expect(element.style.width).toEqual('200px');
+        expect(element.style.width).toEqual('300px');


I would love to re-discuss undefined vs. null, especially wrt to the style bindings prioritisation. This assert seems off to me.

Let's discuss

What is wrong with the assert? (Test pass, looks right to me)

pkozlowski-opensource · 2020-01-21T16:22:01Z

+    expect(div1.className).toEqual('s1');
+    // VE has weird behavior where it calls the @Input('class') with either `class="static` or
+    // `[class]="dynamic"` but never both. This is determined at compile time. Due to locality we
+    // don't know if `[class]` is coming if we see `class` only. So we need to combine the two


For the future consideration / helping to re-word the comment / stating the obvious: this behaviour is not specific to class / [class] but applies to any property binding that has a corresponding attribute... This is another manifestation of the syntax / "shadowing rules" where the left-hand side doesn't identify binding target precisely enough. In this sense VE is not particularly "weird" - it is just how the syntax works / can be abused atm....

It is specific to class in the sense that we do the concatenation only in class.

It is unclear what is my action item here.

googlebot · 2020-01-21T20:29:36Z

All (the pull request submitter and all commit authors) CLAs are signed, but 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 by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

Compiler keeps track of number of slots (`vars`) which are needed for binding instructions. Normally each binding instructions allocates a single slot in the `LView` but styling instructions need to allocate two slots. PR Close angular#34616

…g bindings" (angular#34616) This change reverts angular#28711 NOTE: This change deletes code and creates a BROKEN SHA. If reverting this SHA needs to be reverted with the next SHA to get back into a valid state. The change removes the fact that `NgStyle`/`NgClass` is special and colaborates with the `[style]`/`[class]` to merge its styles. By reverting to old behavior we have better backwards compatiblity since it is no longer treated special and simply overwrites the styles (same as VE) PR Close angular#34616

…4616) NOTE: This change deletes code and creates a BROKEN SHA. If reverting this SHA needs to be reverted with the next SHA to get back into a valid state. PR Close angular#34616

NOTE: This change must be reverted with previous deletes so that it code remains in build-able state. This change deletes old styling code and replaces it with a simplified styling algorithm. The mental model for the new algorithm is: - Create a linked list of styling bindings in the order of priority. All styling bindings ere executed in compiled order and than a linked list of bindings is created in priority order. - Flush the style bindings at the end of `advance()` instruction. This implies that there are two flush events. One at the end of template `advance` instruction in the template. Second one at the end of `hostBindings` `advance` instruction when processing host bindings (if any). - Each binding instructions effectively updates the string to represent the string at that location. Because most of the bindings are additive, this is a cheap strategy in most cases. In rare cases the strategy requires removing tokens from the styling up to this point. (We expect that to be rare case)S Because, the bindings are presorted in the order of priority, it is safe to resume the processing of the concatenated string from the last change binding. PR Close angular#34616

…lar#34616)

googlebot · 2020-01-22T00:40:58Z

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

ℹ️ Googlers: Go here for more info.

…lar#34616)

… styling bindings" (angular#34616)

googlebot · 2020-01-22T05:00:37Z

All (the pull request submitter and all commit authors) CLAs are signed, but 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 by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

googlebot · 2020-01-22T05:01:13Z

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

ℹ️ Googlers: Go here for more info.

googlebot · 2020-01-22T06:26:19Z

All (the pull request submitter and all commit authors) CLAs are signed, but 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 by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

googlebot · 2020-01-22T06:31:37Z

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

ℹ️ Googlers: Go here for more info.

googlebot · 2020-01-22T06:44:21Z

All (the pull request submitter and all commit authors) CLAs are signed, but 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 by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

googlebot · 2020-01-22T06:45:30Z

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

ℹ️ Googlers: Go here for more info.

googlebot · 2020-01-22T17:26:27Z

All (the pull request submitter and all commit authors) CLAs are signed, but 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 by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

kara · 2020-01-22T18:50:52Z

It looks like we are keeping the "head" marker pointing to the "template head" and inserting new host bindings into the middle (in front of the template head but behind previous host bindings). I take it we are not prepending host bindings to maintain their priority order (last directive has highest priority)?

I don't fully follow your question.

The items get inserted into the list in a way which results bindings to be sorted in priority order.

This means that component/directive get inserted in front of e "head" so that the final list has the desired order.

Should we VC?

Probably easier to discuss over VC, yeah. I was asking why we insert new host bindings behind previous host bindings but before template bindings (instead of just prepending to the very beginning)

kara

Two more typos

…lar#34616)

googlebot · 2020-01-23T22:00:04Z

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

ℹ️ Googlers: Go here for more info.

mhevery · 2020-01-24T19:29:51Z

Merged into styling

angular-automatic-lock-bot · 2020-02-24T16:08:19Z

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

googlebot added the cla: no label Jan 2, 2020

mhevery changed the base branch from master to styling January 2, 2020 17:02

mhevery force-pushed the styling branch from a719656 to f004195 Compare January 2, 2020 17:03

mhevery force-pushed the styling-integration branch 2 times, most recently from 7662302 to 0a96469 Compare January 2, 2020 18:46

mhevery added cla: yes and removed cla: no labels Jan 2, 2020

mhevery force-pushed the styling-integration branch from 0a96469 to 35c3645 Compare January 2, 2020 20:09

googlebot added cla: no and removed cla: yes labels Jan 2, 2020

mhevery force-pushed the styling-integration branch from 35c3645 to 3e4f380 Compare January 2, 2020 22:36

mhevery added cla: yes and removed cla: no labels Jan 2, 2020

mhevery force-pushed the styling-integration branch from 3e4f380 to fb4855d Compare January 3, 2020 00:08

googlebot added cla: no and removed cla: yes labels Jan 3, 2020

mhevery added cla: yes and removed cla: no labels Jan 3, 2020

mhevery force-pushed the styling-integration branch from fb4855d to b6478a4 Compare January 3, 2020 00:39

googlebot added cla: no and removed cla: yes labels Jan 3, 2020

mhevery added cla: yes and removed cla: no labels Jan 3, 2020

mhevery force-pushed the styling-integration branch from b6478a4 to 099f4bc Compare January 3, 2020 21:15

googlebot added cla: no and removed cla: yes labels Jan 3, 2020

mhevery force-pushed the styling-integration branch from 099f4bc to 36720db Compare January 3, 2020 23:31

mhevery removed the cla: no label Jan 3, 2020

pkozlowski-opensource reviewed Jan 21, 2020

View reviewed changes

matsko and others added 6 commits January 21, 2020 16:12

refactor(ivy): Delete all styling code. (Part of next SHA) (angular#3…

1895b42

…4616) NOTE: This change deletes code and creates a BROKEN SHA. If reverting this SHA needs to be reverted with the next SHA to get back into a valid state. PR Close angular#34616

fixup! refactor(ivy): Switch styling to new reconcile algorithm (angu…

ac42f5b

…lar#34616)

fixup! refactor(ivy): Switch styling to new reconcile algorithm (angu…

8ad50f7

…lar#34616)

mhevery added 2 commits January 21, 2020 20:59

fixup! refactor(ivy): Switch styling to new reconcile algorithm (angu…

b08cbfc

…lar#34616)

fixup! Revert: "feat(ivy): convert [ngStyle] and [ngClass] to use ivy…

f24f89b

… styling bindings" (angular#34616)

kara reviewed Jan 22, 2020

View reviewed changes

Comment thread packages/core/test/render3/styling_next/style_differ_spec.ts Outdated

Comment thread packages/core/src/render3/styling/style_binding_list.ts Outdated

mhevery added 5 commits January 22, 2020 13:42

fixup! refactor(ivy): Switch styling to new reconcile algorithm (angu…

b06c20c

…lar#34616)

fixup! refactor(ivy): Switch styling to new reconcile algorithm (angu…

7adeb83

…lar#34616)

fixup! refactor(ivy): Switch styling to new reconcile algorithm (angu…

8757637

…lar#34616)

fixup! refactor(ivy): Switch styling to new reconcile algorithm (angu…

1e2dedc

…lar#34616)

fixup! refactor(ivy): Switch styling to new reconcile algorithm (angu…

a1bc763

…lar#34616)

mhevery mentioned this pull request Jan 26, 2020

Angular is changing order of css classes in class attribute #33111

Closed

		@@ -829,6 +1190,14 @@ describe('styling', () => {
		expect(element.style.opacity).toBeFalsy();

Conversation

mhevery commented Jan 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Uh oh!

googlebot commented Jan 3, 2020

Uh oh!

googlebot commented Jan 3, 2020

Uh oh!

pkozlowski-opensource left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

googlebot commented Jan 21, 2020

Uh oh!

googlebot commented Jan 22, 2020

Uh oh!

googlebot commented Jan 22, 2020

Uh oh!

googlebot commented Jan 22, 2020

Uh oh!

googlebot commented Jan 22, 2020

Uh oh!

googlebot commented Jan 22, 2020

Uh oh!

googlebot commented Jan 22, 2020

Uh oh!

googlebot commented Jan 22, 2020

Uh oh!

googlebot commented Jan 22, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kara left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mhevery commented Jan 2, 2020 •

edited

Loading

mhevery commented Jan 24, 2020 •

edited

Loading