Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Styling fix for new styling pr #34308

Closed
wants to merge 16 commits into from
Closed

Conversation

matsko
Copy link
Contributor

@matsko matsko commented Dec 9, 2019

This is just here to test something in G3.

mhevery and others added 16 commits December 8, 2019 16:57
This change introduces several functions for manipulating items in an array in an efficient (binary search) way.

- `arraySplice` a faster version of `Array.splice()`.
- `arrayInsert` a faster version of `Array.splice(index, 0, value)`.
- `arrayInsert2` a faster version of `Array.splice(index, 0, value1, value2)`.
- `arrayInsertSorted` a way to insert a value into sorted list.
- `arrayRemoveSorted` a way to remove a value from a sorted list.
- `arrayIndexOfSorted` a way to find a value in a sorted list.
- `ArrayMap` Efficient implementation of `Map` as an `Array`.
- `arrayMapSet`, `arrayMapGet`, `arrayMapIndexOf`, and `arrayMapDelete` for manipulating `ArrayMap`s.
This change introduces class/style reconciliation algorithm for DOM elements.
NOTE: The code is not yet hooked up, it will be used by future style algorithm.

Background:
Styling algorithm currently has [two paths](https://hackmd.io/@5zDGNGArSxiHhgvxRGrg-g/rycZk3N5S)
when computing how the style should be rendered.
1. A direct path which concatenates styling and uses `elemnent.className`/`element.style.cssText` and
2. A merge path which uses internal data structures and uses `element.classList.add/remove`/`element.style[property]`.

The situation is confusing and hard to follow/maintain. So a future PR will remove the merge-path and do everything with
direct-path. This however breaks when some other code adds class or style to the element without Angular's knowledge.
If this happens instead of switching from direct-path to merge-path algorithm, this change provides a different mental model
whereby we always do `direct-path` but the code which writes to the DOM detects the situation and reconciles the out of bound write.

The reconciliation process is as follows:
1. Detect that no one has modified `className`/`cssText` and if so just write directly (fast path).
2. If out of bounds write did occur, switch from writing using `className`/`cssText` to `element.classList.add/remove`/`element.style[property]`.
   This does require that the write function computes the difference between the previous Angular expected state and current Angular state.
   (This requires a parser. The advantage of having a parser is that we can support `style="width: {{exp}}px" kind of bindings.`)
   Compute the diff and apply it in non destructive way using `element.classList.add/remove`/`element.style[property]`

Properties of approach:
- If no out of bounds style modification:
  - Very fast code path: Just concatenate string in right order and write them to DOM.
  - Class list order is preserved
- If out of bounds style modification detected:
  - Penalty for parsing
  - Switch to non destructive modification: `element.classList.add/remove`/`element.style[property]`
  - Switch to alphabetical way of setting classes.
This patch removes all usage of the merge styling algorithm
(the one that used `TStylingContext`) in favor of a direct
apply algorithm that writes directly to the style attribute
and the className property. By iterating over all bindings
one by one, the final concatenated string value can be applied
once once. This change improves the performance of the styling
features in Angular and also reduces the overall bundle size.
@googlebot
Copy link

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
Copy link

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.

@matsko matsko closed this Dec 9, 2019
@matsko matsko deleted the styling_misko_fix branch December 9, 2019 23:10
@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Jan 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants