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

Trigger page load listeners when no longer loading #23313

Conversation

johnhughes1984
Copy link
Contributor

Description (*)

Trigger JavaScript page load listeners to fire when document is no longer 'loading' rather than waiting until it is marked as 'complete' as this can lead to severe delays in rendering JavaScript content.

Fixed Issues (if relevant)

  1. requirejs/domReady.js can severely delay rendering of content #22909: requirejs/domReady.js can severely delay rendering of content

Manual testing scenarios (*)

  1. Add a non blocking asset to the page with a severe delay e.g. <img src="https://httpstat.us/404?sleep=10000" /> (I added this to Magento_Theme::html/absolute_footer.phtml)
  2. Visit the homepage
  3. The loading status indicator in the browser should be visible for at least the time set in the sleep value of the asset, however private / JavaScript content should render before this finishes (I used the Default welcome msg! text in the header to verify this)

Questions or comments

N/A

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds are green)

* Switch from waiting until complete which could lead to severe rendering delays
@m2-assistant
Copy link

m2-assistant bot commented Jun 18, 2019

Hi @johnhughes1984. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento give me test instance - deploy test instance based on PR changes
  • @magento give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

@magento-cicd2
Copy link
Contributor

magento-cicd2 commented Jun 18, 2019

CLA assistant check
All committers have signed the CLA.

@magento-engcom-team
Copy link
Contributor

Hi @ihor-sviziev, thank you for the review.
ENGCOM-5305 has been created to process this Pull Request
✳️ @ihor-sviziev, could you please add one of the following labels to the Pull Request?

Label Description
Auto-Tests: Covered All changes in Pull Request is covered by auto-tests
Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests
Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests

@magento-engcom-team
Copy link
Contributor

@johnhughes1984 thank you for contributing. Please accept Community Contributors team invitation here to gain extended permissions for this repository.

@ihor-sviziev ihor-sviziev added the Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests label Jun 18, 2019
@ihor-sviziev
Copy link
Contributor

Not sure if it’s possible to cover with some js tests, but would be really great

@DrewML
Copy link

DrewML commented Jun 18, 2019

@magento give me test instance

@magento-engcom-team
Copy link
Contributor

Hi @DrewML. Thank you for your request. I'm working on Magento instance for you

@magento-engcom-team
Copy link
Contributor

Hi @DrewML, here is your new Magento instance.
Admin access: https://pr-23313.instances.magento-community.engineering/admin
Login: admin Password: 123123q

@DrewML
Copy link

DrewML commented Jun 18, 2019

I added <img src="https://httpstat.us/404?sleep=10000" /> to the homepage content in the provided demo instance, and verified that:

  1. mage-init content is no longer relying on the load event, but instead DOMContentLoaded as intended
  2. That the specific conditional change in this PR was actually hit/executed during my testing

Thanks @johnhughes1984!

@engcom-Delta
Copy link
Contributor

✔️ QA passed

@VladimirZaets
Copy link
Contributor

Hi @ihor-sviziev. I think test coverage in this case not applicable. In general, we can cover the functionality of domReady by tests, but it's 3rd party library and it doesn't show real broken cases, we can check it just by functional tests that covered direct part of the functionality

@VladimirZaets VladimirZaets removed the Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests label Jul 10, 2019
@engcom-Alfa
Copy link
Contributor

✔️ QA Passed

@sidolov sidolov changed the base branch from 2.3-develop to 2.4-develop December 5, 2019 17:20
@engcom-Golf engcom-Golf added Auto-Tests: Covered All changes in Pull Request is covered by auto-tests and removed Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests labels Dec 9, 2019
@DrewML
Copy link

DrewML commented Dec 9, 2019

@magento-engcom-team Anything pending before we can merge this?

@slavvka
Copy link
Member

slavvka commented Dec 16, 2019

@magento run all tests

@slavvka
Copy link
Member

slavvka commented Dec 16, 2019

The PR is currently in the delivery queue. Please be patient.

@VladimirZaets
Copy link
Contributor

Please fix failed CE, EE, B2B FAT

@slavvka
Copy link
Member

slavvka commented Jan 8, 2020

@magento run all tests

@ihor-sviziev
Copy link
Contributor

@engcom-Golf @engcom-Foxtrot ,
Could you fix failing b2b / ee functional tests?

@magento-engcom-team magento-engcom-team merged commit 85ccb52 into magento:2.4-develop Jan 24, 2020
@m2-assistant
Copy link

m2-assistant bot commented Jan 24, 2020

Hi @johnhughes1984, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.