-
Notifications
You must be signed in to change notification settings - Fork 773
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
fix(auto-prefixer): resolve perf impacts as reported by LightHouse #283
Conversation
aa41e12
to
c7b64c3
Compare
c7b64c3
to
435fe61
Compare
435fe61
to
8a32c88
Compare
src/lib/utils/auto-prefixer.ts
Outdated
|
||
switch (key) { | ||
case 'display': | ||
if (value === 'flex') { | ||
target['display'] = [ | ||
'-webkit-box', | ||
'-moz-box', | ||
'-ms-flexbox', | ||
'-webkit-flex', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you removed the wrong one here; you want to keep -webkit-flex
and remove -webkit-box
(same for the other related properties throughout)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jelbourn - I am very confused here. AutoPrefixer uses -webkit-box
and does NOT use -webkit-flex
.
Your comment ^ and some online research proves that AutoPrefixer source is not correct. Here is what I found:
{
display: -webkit-box; /* OLD - iOS 6-, Safari 3.1-6, BB7 */
display: -ms-flexbox; /* TWEENER - IE 10 */
display: -webkit-flex; /* NEW - Safari 6.1+. iOS 7.1+, BB10 */
display: flex; /* NEW, Spec - Firefox, Chrome, Opera */
}
I will restore the -webkit-flex
settings. Thx for the oversight.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can confirm that the layout does not work anymore in iOS8. I will check again after the -webkit prefixes have been restored.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@karlhaas - can you confirm now with the lastest updates to this PR?
src/lib/utils/auto-prefixer.ts
Outdated
let value = target[key]; | ||
let value = target[key] || ""; | ||
let boxValue = toBoxValue(value); | ||
let boxDirection = toBoxDirection(value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think toBoxDirection and toBoxOrient will fail with non string parameters. I did a quick test with your changes and it fails with numeric values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thx @karlhaas.
1a0cf6a
to
5615983
Compare
@karlhaas - Can you check these recent changes on iOS8 ? Thx. |
src/lib/utils/auto-prefixer.ts
Outdated
target['-ms-align-content'] = toAlignContentValue(value); | ||
target['-ms-flex-line-pack'] = toAlignContentValue(value); | ||
target['-webkit-align-content'] = value; | ||
target['order'] = isNaN(value) ? '0' : value; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ThomasBurleson Thx! the layout seems to work. According to https://www.w3schools.com/cssref/css3_pr_order.asp order needs a prefix as well. I would suggest something like
case 'order':
target['order'] = target['-webkit-order'] = isNaN(value) ? '0' : value;
In our app there are still some problems with material 2 (eg. dialogs). I would try to continue the work on angular/components#4385 and reduce the prefixes like you did as soon as somebody from the angular 2 team signals that they will accept the changes ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
5615983
to
c5abc35
Compare
@jelbourn - please presubmit. |
* Updated based on output from https://autoprefixer.github.io/ * Removed Mozilla Firefox prefixes as not required for the evergreen browser * Remove support for IE10 prefixes (`-ms-`); as only IE 11 and Edge are supported * Remove support for `-webkit-box` required for OLD iOS , Safari 3, BB7 * Fix value transforms for flex-related keys Fixes #282
c5abc35
to
49cf7dc
Compare
@tinayuangao - rebase is done. |
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Internal AutoPrefixer updated based on output from prefix rules for n-2 evergreen browsers.
-ms-
); as only IE 11 and Edge are supported-webkit-box
required for OLD iOS , Safari 3, BB7Fixes #282