Change static priority resolution to be same level as directive it belongs to #34938

mhevery · 2020-01-23T22:30:04Z

This change changes the priority order of static styling.

Current priority:

(least priority)
- Static 
  - Component
  - Directives
  - Template
- Dynamic Binding
  - Component
    - Map/Interpolation
    - Property
  - Directives
    - Map/Interpolation
    - Property
  - Template
    - Map/Interpolation
    - Property
(highest priority)

The issue with the above priority is this use case:

<div style="color: red;" directive-which-sets-color-blue>

In the above case the directive will win and the resulting color will be blue. However a small change of adding interpolation to the example like so. (Style interpolation is coming in #34202)

<div style="color: red; width: {{exp}}px" directive-which-sets-color-blue>

Changes the priority from static binding to interpolated binding which means now the resulting color is red. It is very surprising that adding an unrelated interpolation and style can change the color which was not changed. To fix that we need to make sure that the static values are associated with priority of the source (directive or template) where they were declared. The new resulting priority is:

(least priority)
- Component
  - Static
  - Map/Interpolation
  - Property
- Directives
  - Static
  - Map/Interpolation
  - Property
- Template
  - Static
  - Map/Interpolation
  - Property
(highest priority)

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-26T20:31:16Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

mhevery · 2020-01-26T21:21:01Z

presubmit

AndrewKushnir · 2020-01-27T05:30:39Z

Global Ivy Presubmit

pkozlowski-opensource

The first round of comments from the first pass. I must say that I wasn't able to de-cipher all the details of the data structures used and the algorithm. I guess we should sync-up so you can walk me through it so I've got some more background.

At the same I didn't notice anything that would look "wrong", so we are for a good start here!

packages/core/src/render3/interfaces/node.ts

packages/core/src/util/assert.ts

packages/core/src/render3/interfaces/styling.ts

AndrewKushnir · 2020-01-28T03:05:23Z

Global Ivy Presubmit #2

kara

Looks good for merge, though we will do some additional cleanup (docs, any stylistic nits, etc) in a follow-up

mhevery · 2020-01-29T21:54:31Z

presubmit

…ctive it belongs to This change changes the priority order of static styling. Current priority: ``` (least priority) - Static - Component - Directives - Template - Dynamic Binding - Component - Map/Interpolation - Property - Directives - Map/Interpolation - Property - Template - Map/Interpolation - Property (highest priority) ``` The issue with the above priority is this use case: ``` <div style="color: red;" directive-which-sets-color-blue> ``` In the above case the directive will win and the resulting color will be `blue`. However a small change of adding interpolation to the example like so. (Style interpolation is coming in angular#34202) ``` <div style="color: red; width: {{exp}}px" directive-which-sets-color-blue> ``` Changes the priority from static binding to interpolated binding which means now the resulting color is `red`. It is very surprising that adding an unrelated interpolation and style can change the `color` which was not changed. To fix that we need to make sure that the static values are associated with priority of the source (directive or template) where they were declared. The new resulting priority is: ``` (least priority) - Component - Static - Map/Interpolation - Property - Directives - Static - Map/Interpolation - Property - Template - Static - Map/Interpolation - Property (highest priority) ```

…as directive it belongs to

…ctive it belongs to (#34938) This change changes the priority order of static styling. Current priority: ``` (least priority) - Static - Component - Directives - Template - Dynamic Binding - Component - Map/Interpolation - Property - Directives - Map/Interpolation - Property - Template - Map/Interpolation - Property (highest priority) ``` The issue with the above priority is this use case: ``` <div style="color: red;" directive-which-sets-color-blue> ``` In the above case the directive will win and the resulting color will be `blue`. However a small change of adding interpolation to the example like so. (Style interpolation is coming in #34202) ``` <div style="color: red; width: {{exp}}px" directive-which-sets-color-blue> ``` Changes the priority from static binding to interpolated binding which means now the resulting color is `red`. It is very surprising that adding an unrelated interpolation and style can change the `color` which was not changed. To fix that we need to make sure that the static values are associated with priority of the source (directive or template) where they were declared. The new resulting priority is: ``` (least priority) - Component - Static - Map/Interpolation - Property - Directives - Static - Map/Interpolation - Property - Template - Static - Map/Interpolation - Property (highest priority) ``` PR Close #34938

PR Close #34938

…ctive it belongs to (#34938) This change changes the priority order of static styling. Current priority: ``` (least priority) - Static - Component - Directives - Template - Dynamic Binding - Component - Map/Interpolation - Property - Directives - Map/Interpolation - Property - Template - Map/Interpolation - Property (highest priority) ``` The issue with the above priority is this use case: ``` <div style="color: red;" directive-which-sets-color-blue> ``` In the above case the directive will win and the resulting color will be `blue`. However a small change of adding interpolation to the example like so. (Style interpolation is coming in #34202) ``` <div style="color: red; width: {{exp}}px" directive-which-sets-color-blue> ``` Changes the priority from static binding to interpolated binding which means now the resulting color is `red`. It is very surprising that adding an unrelated interpolation and style can change the `color` which was not changed. To fix that we need to make sure that the static values are associated with priority of the source (directive or template) where they were declared. The new resulting priority is: ``` (least priority) - Component - Static - Map/Interpolation - Property - Directives - Static - Map/Interpolation - Property - Template - Static - Map/Interpolation - Property (highest priority) ``` PR Close #34938

`TNode.directives` was introduced in angular#34938. Turns out that it is unnecessary because the information is already present it `TData` when combining with `TNode.directiveStart` and `TNode.directiveEnd` Mainly this is true (conceptually): ``` expect(tNode.directives).toEqual( tData.slice( tNode.directivesStart, tNode.directivesEnd - tNode.DirectivesStart -1 ) ); ``` The refactoring removes `TNode.directives` and adds `TNode.directiveStyling` as we still need to keep location in the directive in `TNode`

`TNode.directives` was introduced in #34938. Turns out that it is unnecessary because the information is already present it `TData` when combining with `TNode.directiveStart` and `TNode.directiveEnd` Mainly this is true (conceptually): ``` expect(tNode.directives).toEqual( tData.slice( tNode.directivesStart, tNode.directivesEnd - tNode.DirectivesStart -1 ) ); ``` The refactoring removes `TNode.directives` and adds `TNode.directiveStyling` as we still need to keep location in the directive in `TNode` PR Close #35050

angular-automatic-lock-bot · 2020-02-29T16:09:09Z

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 23, 2020

mhevery force-pushed the styling-static branch 3 times, most recently from 2127b9f to fb16cd7 Compare January 26, 2020 20:30

mhevery requested a review from kara January 26, 2020 20:31

mhevery force-pushed the styling-static branch from fb16cd7 to 91a8cd6 Compare January 26, 2020 20:31

mhevery requested a review from pkozlowski-opensource January 26, 2020 20:31

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

angular deleted a comment from googlebot Jan 26, 2020

mhevery marked this pull request as ready for review January 26, 2020 20:31

mhevery requested review from a team as code owners January 26, 2020 20:31

mhevery added comp: ivy target: patch This PR is targeted for the next patch release labels Jan 26, 2020

ngbot bot modified the milestone: needsTriage Jan 26, 2020

mhevery force-pushed the styling-static branch from 91a8cd6 to 4cfe19d Compare January 26, 2020 20:41

mhevery force-pushed the styling-static branch 3 times, most recently from 42e1416 to c4459fb Compare January 26, 2020 23:00

mhevery requested a review from a team January 26, 2020 23:00

pkozlowski-opensource reviewed Jan 27, 2020

View reviewed changes

mhevery force-pushed the styling-static branch 3 times, most recently from a6083ec to 03779b3 Compare January 28, 2020 21:22

mhevery force-pushed the styling-static branch from ff5e8ef to 60af7e0 Compare January 29, 2020 20:57

kara approved these changes Jan 29, 2020

View reviewed changes

mhevery added 7 commits January 29, 2020 14:37

refactor(ivy): make assert conditions more readable

e0d65d5

fixup! feat(ivy): Change static priority resolution to be same level …

32f9448

…as directive it belongs to

fixup! feat(ivy): Change static priority resolution to be same level …

2c3efcb

…as directive it belongs to

fixup! feat(ivy): Change static priority resolution to be same level …

f6d221b

…as directive it belongs to

fixup! feat(ivy): Change static priority resolution to be same level …

fb51e8c

…as directive it belongs to

fixup! feat(ivy): Change static priority resolution to be same level …

3caf09a

…as directive it belongs to

mhevery force-pushed the styling-static branch from 2a169ef to f115f53 Compare January 29, 2020 22:38

fixup! feat(ivy): Change static priority resolution to be same level …

4039238

…as directive it belongs to

mhevery force-pushed the styling-static branch from f115f53 to 4039238 Compare January 29, 2020 23:05

mhevery added the action: merge The PR is ready for merge by the caretaker label Jan 29, 2020

AndrewKushnir closed this in cf07d42 Jan 29, 2020

AndrewKushnir pushed a commit that referenced this pull request Jan 29, 2020

refactor(ivy): make assert conditions more readable (#34938)

dd00846

PR Close #34938

mhevery mentioned this pull request Jan 30, 2020

refactor(ivy): Remove TNode.directives in favor of TData #35050

Closed

14 tasks

mhevery mentioned this pull request Feb 6, 2020

Need a way for a component to add CSS classes to host non-destructively #7289

Closed

ericmartinezr mentioned this pull request Feb 12, 2020

Input with name "class" and host binding for class have a different behaviour in ng8 and ng9 #35383

Closed

angular-automatic-lock-bot bot locked and limited conversation to collaborators Feb 29, 2020

Uh oh!

Change static priority resolution to be same level as directive it belongs to #34938

Change static priority resolution to be same level as directive it belongs to #34938

Uh oh!

Conversation

mhevery commented Jan 23, 2020

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 26, 2020

Uh oh!

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

Uh oh!

Uh oh!

AndrewKushnir commented Jan 27, 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!

Uh oh!

AndrewKushnir commented Jan 28, 2020

Uh oh!

kara left a comment

Choose a reason for hiding this comment

Uh oh!

mhevery commented Jan 29, 2020

Uh oh!

angular-automatic-lock-bot bot commented Feb 29, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mhevery commented Jan 26, 2020 •

edited

Loading