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

Difference in less compilation between php & nodejs library (grunt) with complicated calc expressions #37841

Open
5 tasks
hostep opened this issue Aug 3, 2023 · 11 comments · May be fixed by #37842
Open
5 tasks
Assignees
Labels
Area: Framework Component: Frontend Issue: Confirmed Gate 3 Passed. Manual verification of the issue completed. Issue is confirmed Priority: P2 A defect with this priority could have functionality issues which are not to expectations. Progress: PR in progress Reported on 2.4.x Indicates original Magento version for the Issue report. Reproduced on 2.4.x The issue has been reproduced on latest 2.4-develop branch Triage: Dev.Experience Issue related to Developer Experience and needs help with Triage to Confirm or Reject it

Comments

@hostep
Copy link
Contributor

hostep commented Aug 3, 2023

Preconditions and environment

  • Magento version 2.4-develop
  • PHP 8.1 or 8.2
  • Nodejs 16

Other things to prepare here:

  1. Setup vanilla Magento install
  2. Run the following:
$ cp package.json.sample package.json
$ cp Gruntfile.js.sample Gruntfile.js
$ npm install
$ bin/magento deploy:mode:set developer  # so that when compiling less via php, it's not being minified, so that comparing files is easier
  1. In the backoffice, go to Stores >Attributes > Product, and for both news_from_date and news_to_date perform the following change:
    • Storefront Properties > Use in Search > Yes
    • Storefront Properties > Visible in Advanced Search > Yes

Steps to reproduce

  1. First run the less compilation through the less library in php:
$ rm -R var/view_preprocessed/* pub/static/*
$ bin/magento setup:upgrade # needed to be able to visit frontend without js errors
$ bin/magento setup:static-content:deploy -f en_US
  1. Now copy the resulting files in some other directory:
$ cp -r pub/static/ /tmp/magento-scd-comparison
  1. In one tab in your browser, visit the frontend of the shop and click on Advanced Search in the footer and keep this open
  2. Now run the less compilation through grunt (so via nodejs instead of php):
$ rm -R var/view_preprocessed/* pub/static/*
$ bin/magento setup:upgrade # needed to be able to visit frontend without js errors
$ grunt clean:luma && grunt exec:luma && grunt less:luma
  1. Now compare the resulting files with the ones you copied before, I'm focusing here on something very specific (not all changes are relevant or important here):
$ diff -u /tmp/magento-scd-comparison/frontend/Magento/luma/en_US/css/styles-m.css pub/static/frontend/Magento/luma/en_US/css/styles-m.css  | grep -C3 calc

Output:

-.form.search.advanced .fields.range .field.date input,
-.form-giftregistry-edit .field.date input {
-  margin-right: 10px;
-  width: calc(100% - 33px);
+.form-giftregistry-search .fields-specific-options .datetime-picker {
+  margin-right: 10px;width: calc(100% - 23px + 10px);
 }
 .field .control._with-tooltip {
   position: relative;
 }
 .field .control._with-tooltip input {
-  margin-right: 10px;
-  width: calc(100% - 36px);
+  margin-right: 10px;width: calc(100% - 21px + 10px + 5px);
 }
 .checkout-index-index .modal-popup .field-tooltip .field-tooltip-content,
 .shipping-policy-block.field-tooltip .field-tooltip-content {
  1. In a second tab in your browser, visit the frontend of the shop and click on Advanced Search in the footer and compare it to your first tab

Expected result

The width: calc expressions are the same between the PHP less library & nodejs less library:

less.js:
1. width: calc(100% - 23px + 10px);
2. width: calc(100% - 21px + 10px + 5px);

less.php:
1. width: calc(100% - 23px + 10px); or width: calc(100% - 13px);
2. width: calc(100% - 21px + 10px + 5px); or width: calc(100% - 6px);

Actual result

The width: calc expressions are different between the PHP less library & nodejs less library:

less.js:
1. width: calc(100% - 23px + 10px);   => simplified: width: calc(100% - 13px);
2. width: calc(100% - 21px + 10px + 5px); => simplified: width: calc(100% - 6px);

less.php:
1. width: calc(100% - 33px);
2. width: calc(100% - 36px);

How it looks with less.js:

Screenshot 2023-08-03 at 17 41 03

How it looks with less.php:

Screenshot 2023-08-03 at 17 38 36

Additional information

You may be surprised by the screenshots, but reading the original less code (which contains a bug), it should be rendered like the less.js result (the bug can be fixed by changing + @indent__s to - @indent__s which makes a lot more sense)

I've tried this again but with v4 of wikimedia/less.php (which is being used in Magento as php library to compile less, currently v3 is being installed). And v4 fixes this problem, I'm pretty much convinced this fixed it: wikimedia/less.php@5ca673d

Fix this by recognising calc() higher up as something that server-side LESS math should not be performed on in the first place.

So the solution for Magento is to switch from v3 to v4 of wikimedia/less.php. An additional benefit is that v4 is slightly faster than v3, about 1 second faster average when running bin/magento setup:static-content:deploy with all 3 default Magento themes.

I'm currently working on a PR that will upgrade wikimedia/less.php to v4, but it's more complicated, some less code in Magento is not written correctly (especially around those calc expressions) and we'll need to tweak it bit.
Expect a PR soon (maybe today, maybe later this week)

Release note

No response

Triage and priority

  • Severity: S0 - Affects critical data or functionality and leaves users without workaround.
  • Severity: S1 - Affects critical data or functionality and forces users to employ a workaround.
  • Severity: S2 - Affects non-critical data or functionality and forces users to employ a workaround.
  • Severity: S3 - Affects non-critical data or functionality and does not force users to employ a workaround.
  • Severity: S4 - Affects aesthetics, professional look and feel, “quality” or “usability”.
@m2-assistant
Copy link

m2-assistant bot commented Aug 3, 2023

Hi @hostep. Thank you for your report.
To speed up processing of this issue, make sure that the issue is reproducible on the vanilla Magento instance following Steps to reproduce. To deploy vanilla Magento instance on our environment, Add a comment to the issue:


Join Magento Community Engineering Slack and ask your questions in #github channel.
⚠️ According to the Magento Contribution requirements, all issues must go through the Community Contributions Triage process. Community Contributions Triage is a public meeting.
🕙 You can find the schedule on the Magento Community Calendar page.
📞 The triage of issues happens in the queue order. If you want to speed up the delivery of your contribution, join the Community Contributions Triage session to discuss the appropriate ticket.

@m2-community-project m2-community-project bot added this to Ready for Confirmation in Issue Confirmation and Triage Board Aug 3, 2023
@hostep hostep changed the title Difference in less compilation with complicated calc when using less php library compared to less compiling via grunt Difference in less compilation between php & nodejs library (grunt) with complicated calc expressions Aug 3, 2023
@engcom-Dash engcom-Dash added the Reported on 2.4.x Indicates original Magento version for the Issue report. label Aug 3, 2023
@hostep hostep linked a pull request Aug 3, 2023 that will close this issue
5 tasks
@engcom-Dash engcom-Dash added Issue: Confirmed Gate 3 Passed. Manual verification of the issue completed. Issue is confirmed and removed Issue: ready for confirmation labels Oct 4, 2023
@m2-community-project m2-community-project bot moved this from Ready for Confirmation to Confirmed in Issue Confirmation and Triage Board Oct 4, 2023
@m2-community-project m2-community-project bot moved this from Ready for Confirmation to Confirmed in Issue Confirmation and Triage Board Oct 4, 2023
@github-jira-sync-bot github-jira-sync-bot removed the Issue: Confirmed Gate 3 Passed. Manual verification of the issue completed. Issue is confirmed label Oct 4, 2023
@github-jira-sync-bot
Copy link

Unfortunately, not enough information was provided to create a Jira ticket. Please make sure you added the following label(s): Reproduced on 2.4.x, ^Area:.*

Once all required labels are present, please add Issue: Confirmed label again.

@engcom-Dash
Copy link

@magento give me 2.4-develop instance

@magento-deployment-service
Copy link

Hi @engcom-Dash. Thank you for your request. I'm working on Magento instance for you.

@magento-deployment-service
Copy link

@engcom-Dash engcom-Dash added the Triage: Dev.Experience Issue related to Developer Experience and needs help with Triage to Confirm or Reject it label Oct 5, 2023
@engcom-November engcom-November self-assigned this Oct 10, 2023
@m2-assistant
Copy link

m2-assistant bot commented Oct 10, 2023

Hi @engcom-November. Thank you for working on this issue.
In order to make sure that issue has enough information and ready for development, please read and check the following instruction: 👇

  • 1. Verify that issue has all the required information. (Preconditions, Steps to reproduce, Expected result, Actual result).
  • 2. Verify that issue has a meaningful description and provides enough information to reproduce the issue.
  • 3. Add Area: XXXXX label to the ticket, indicating the functional areas it may be related to.
  • 4. Verify that the issue is reproducible on 2.4-develop branch
    Details- Add the comment @magento give me 2.4-develop instance to deploy test instance on Magento infrastructure.
    - If the issue is reproducible on 2.4-develop branch, please, add the label Reproduced on 2.4.x.
    - If the issue is not reproducible, add your comment that issue is not reproducible and close the issue and stop verification process here!
  • 5. Add label Issue: Confirmed once verification is complete.
  • 6. Make sure that automatic system confirms that report has been added to the backlog.

@engcom-November
Copy link
Contributor

Hello @hostep,

Thank you for the report and collaboration!

We were able to reproduced the issue on 2.4-develop.
Please take a look at the screenshot:
image

We have followed your steps, and it can be seen that width: calc expression are different for less js and less php.
Hence confirming the issue.

Thank you.

@engcom-November engcom-November added Area: Frontend Reproduced on 2.4.x The issue has been reproduced on latest 2.4-develop branch Component: Frontend Issue: Confirmed Gate 3 Passed. Manual verification of the issue completed. Issue is confirmed labels Oct 10, 2023
@m2-community-project m2-community-project bot moved this from Ready for Confirmation to Confirmed in Issue Confirmation and Triage Board Oct 10, 2023
@github-jira-sync-bot
Copy link

❌ Something went wrong. Cannot create Jira issue.

@engcom-November engcom-November removed the Issue: Confirmed Gate 3 Passed. Manual verification of the issue completed. Issue is confirmed label Oct 10, 2023
@m2-community-project m2-community-project bot moved this from Confirmed to Ready for Confirmation in Issue Confirmation and Triage Board Oct 10, 2023
@engcom-November engcom-November added Issue: Confirmed Gate 3 Passed. Manual verification of the issue completed. Issue is confirmed and removed Issue: ready for confirmation labels Oct 10, 2023
@m2-community-project m2-community-project bot moved this from Ready for Confirmation to Confirmed in Issue Confirmation and Triage Board Oct 10, 2023
@github-jira-sync-bot
Copy link

❌ Something went wrong. Cannot create Jira issue.

@engcom-November engcom-November added Area: Framework and removed Area: Frontend Issue: Confirmed Gate 3 Passed. Manual verification of the issue completed. Issue is confirmed labels Oct 10, 2023
@m2-community-project m2-community-project bot moved this from Confirmed to Ready for Confirmation in Issue Confirmation and Triage Board Oct 10, 2023
@engcom-November engcom-November added Issue: Confirmed Gate 3 Passed. Manual verification of the issue completed. Issue is confirmed and removed Issue: ready for confirmation labels Oct 10, 2023
@m2-community-project m2-community-project bot moved this from Ready for Confirmation to Confirmed in Issue Confirmation and Triage Board Oct 10, 2023
@m2-community-project m2-community-project bot moved this from Ready for Confirmation to Confirmed in Issue Confirmation and Triage Board Oct 10, 2023
@github-jira-sync-bot
Copy link

✅ Jira issue https://jira.corp.adobe.com/browse/AC-9712 is successfully created for this GitHub issue.

@m2-assistant
Copy link

m2-assistant bot commented Oct 10, 2023

✅ Confirmed by @engcom-November. Thank you for verifying the issue.
Issue Available: @engcom-November, You will be automatically unassigned. Contributors/Maintainers can claim this issue to continue. To reclaim and continue work, reassign the ticket to yourself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Framework Component: Frontend Issue: Confirmed Gate 3 Passed. Manual verification of the issue completed. Issue is confirmed Priority: P2 A defect with this priority could have functionality issues which are not to expectations. Progress: PR in progress Reported on 2.4.x Indicates original Magento version for the Issue report. Reproduced on 2.4.x The issue has been reproduced on latest 2.4-develop branch Triage: Dev.Experience Issue related to Developer Experience and needs help with Triage to Confirm or Reject it
Projects
High Priority Backlog
  
Pull Request In Progress
Development

Successfully merging a pull request may close this issue.

5 participants