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.

Sign up for GitHub

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

Jump to bottom

fix(auto-prefixer): resolve perf impacts as reported by LightHouse #283

Merged
merged 1 commit into from
May 25, 2017
Merged

fix(auto-prefixer): resolve perf impacts as reported by LightHouse #283

merged 1 commit into from
May 25, 2017

Conversation

ThomasBurleson
Copy link
Contributor

@ThomasBurleson ThomasBurleson commented May 10, 2017 •
edited
Loading

Internal AutoPrefixer updated based on output from prefix rules for n-2 evergreen browsers.

  • 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

@ThomasBurleson ThomasBurleson mentioned this pull request May 11, 2017
Closed
@ThomasBurleson
Copy link
Contributor Author

ThomasBurleson commented May 11, 2017 •
edited
Loading

@jelbourn, @karlhaas - please review.

I would like to get this merged into master asap to facilitate community pre-release usages.

@kara kara requested a review from jelbourn May 16, 2017 15:54
jelbourn
jelbourn reviewed May 20, 2017
View reviewed changes
src/lib/utils/auto-prefixer.ts Outdated

switch (key) {
case 'display':
if (value === 'flex') {
target['display'] = [
'-webkit-box',
'-moz-box',
'-ms-flexbox',
'-webkit-flex',
Copy link
Member

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)

Copy link
Contributor Author

@ThomasBurleson ThomasBurleson May 20, 2017 •
edited
Loading

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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?

karlhaas
karlhaas reviewed May 20, 2017
View reviewed changes
src/lib/utils/auto-prefixer.ts Outdated
let value = target[key];
let value = target[key] || "";
let boxValue = toBoxValue(value);
let boxDirection = toBoxDirection(value);
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx @karlhaas.

@ThomasBurleson ThomasBurleson force-pushed the thomas/issue-282 branch 2 times, most recently from 1a0cf6a to 5615983 Compare May 20, 2017 15:57
@ThomasBurleson
Copy link
Contributor Author

@karlhaas - Can you check these recent changes on iOS8 ? Thx.

karlhaas
karlhaas reviewed May 20, 2017
View reviewed changes
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;
Copy link
Contributor

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@ThomasBurleson
Copy link
Contributor Author

@jelbourn - please presubmit.

@jelbourn jelbourn assigned tinayuangao and unassigned jelbourn May 22, 2017
@ThomasBurleson
fix(auto-prefixer): resolve perf impacts as reported by LightHouse
49cf7dc 
* 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
@ThomasBurleson
Copy link
Contributor Author

@tinayuangao - rebase is done.

@tinayuangao tinayuangao merged commit bc0c900 into master May 25, 2017
@ThomasBurleson ThomasBurleson deleted the thomas/issue-282 branch August 7, 2017 22:36
@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 Sep 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jelbourn jelbourn jelbourn left review comments

@karlhaas karlhaas karlhaas left review comments

Assignees
No one assigned
Labels
cla: yes pr: needs presubmit
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

fix(auto-prefixer): resolve perf impacts as reported by LightHouse
6 participants
@ThomasBurleson @jelbourn @karlhaas @kara @googlebot @tinayuangao