Fix issue no touch event on mobile #37234

mrtuvn · 2023-03-17T13:23:47Z

Description (*)

Delivery patch for recent fix fotorama js issue touch image in product details page.
Feel free to leave any review or comment any your thoughts

Excellent docs from mozilla
https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener

https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener#improving_scrolling_performance_with_passive_listeners

Logic here
wheel -> passive true cause listener can't block page rendering while the user is scrolling
non wheel (mobile) -> passive false

object options with property passive will pass to event listener at third params

Related Pull Requests

Fixed Issues (if relevant)

Fixes Cannot slide images in product page in mobile in 2.4.6 only! #37232

Manual testing scenarios (*)

For image type

Apply version to 2.4.6
Go to any product details (product has media at least 2 images) on mobile device
Try to swipe image
Image should able to swipe to next image without problems (Previously before patch can't swipe image on mobile)

Also should Re-test for images-videos mixed case

Further tests
Go click on image to enter fullscreen mode
try next image and close fullscreen (X icon) expect works without problems

PC browser console no errors related with fotorama

Cross-browsers checklists

Chrome
Chrome (mobile/touch-events enabled)
iOS (Chrome & Safari - latest)
Chrome Android (latest)
Firefox

Questions or comments

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)
README.md files for modified modules are updated and included in the pull request if any README.md predefined sections require an update
All automated tests passed successfully (all builds are green)

m2-assistant · 2023-03-17T13:23:52Z

Hi @mrtuvn. Thank you for your contribution!
Here are some useful tips on 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.4-develop instance - deploy vanilla Magento instance

❗ Automated tests can be triggered manually with an appropriate comment:

@magento run all tests - run or re-run all required tests against the PR changes
@magento run <test-build(s)> - run or re-run specific test build(s)
For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names.

Allowed build names are:

Database Compare
Functional Tests CE
Functional Tests EE
Functional Tests B2B
Integration Tests
Magento Health Index
Sample Data Tests CE
Sample Data Tests EE
Sample Data Tests B2B
Static Tests
Unit Tests
WebAPI Tests
Semantic Version Checker

You can find more information about the builds here
ℹ️ Run only required test builds during development. Run all test builds before sending your pull request for review.

For more details, review the Code Contributions documentation.
Join Magento Community Engineering Slack and ask your questions in #github channel.

mrtuvn · 2023-03-17T13:33:57Z

@magento run all tests

magento-automated-testing · 2023-03-17T13:34:06Z

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

mrtuvn · 2023-03-18T02:02:42Z

@magento run Integration Tests, Functional Tests CE, Functional Tests B2B, Functional Tests EE

magento-automated-testing · 2023-03-18T02:02:53Z

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

mrtuvn · 2023-03-18T03:39:14Z

@magento give me test instance

magento-deployment-service · 2023-03-18T03:39:17Z

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

magento-deployment-service · 2023-03-18T03:44:18Z

Hi @mrtuvn, here is your Magento Instance: https://fbcfaa3464f382e6a29ca2d31449570c.instances-prod.magento-community.engineering
Admin access: https://fbcfaa3464f382e6a29ca2d31449570c.instances-prod.magento-community.engineering/admin_476e
Login: c4a2e381
Password: 12b8c1a146c3

ihor-sviziev

✔ Approved.

Failing tests look not related to changes from this PR.

mrtuvn · 2023-04-20T10:12:28Z

@magento run all tests

magento-automated-testing · 2023-04-20T10:12:34Z

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

ihor-sviziev · 2023-04-20T12:30:02Z

@magento run all tests

magento-automated-testing · 2023-04-20T12:30:11Z

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

cptX · 2023-04-21T08:50:22Z

The current patch with all these changes is not working. It's impossible to swipe the images.
What actually works is taking the original file and implementing only this change:

In file lib/web/fotorama/fotorama.js @1143

-    el.addEventListener ? el.addEventListener(e, fn, !!bool) : el.attachEvent('on' + e, fn);
+    const options = {
+        get passive() {
+            return e === 'wheel';
+        }
+    }
+    el.addEventListener ? el.addEventListener(e, fn, options) : el.attachEvent('on' + e, fn);

mrtuvn · 2023-04-21T12:38:41Z

The current patch with all these changes is not working. It's impossible to swipe the images. What actually works is taking the original file and implementing only this change:
In file lib/web/fotorama/fotorama.js @1143

-    el.addEventListener ? el.addEventListener(e, fn, !!bool) : el.attachEvent('on' + e, fn);
+    const options = {
+        get passive() {
+            return e === 'wheel';
+        }
+    }
+    el.addEventListener ? el.addEventListener(e, fn, options) : el.attachEvent('on' + e, fn);

thanks for feedbacks! But latest update also for video mixed case too (if your products have setup multiple images and also video from backend). Have you update with my latest one ? @cptX

I try keep not changes fotorama as much as possible! In this PR contain
1- Modernizr add tests for passive events
2- Add logic check for both non-passive and passive supports

cptX · 2023-04-21T12:48:01Z

I don't have videos so I cannot test. But because I have images and images were not sliding with your latest update I ended up using only this part of the patch...

mrtuvn · 2023-04-21T13:04:47Z

@magento give me test instance

magento-deployment-service · 2023-04-21T13:04:51Z

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

magento-deployment-service · 2023-04-21T13:09:13Z

Hi @mrtuvn, here is your Magento Instance: https://fbcfaa3464f382e6a29ca2d31449570c.instances-prod.magento-community.engineering
Admin access: https://fbcfaa3464f382e6a29ca2d31449570c.instances-prod.magento-community.engineering/admin_88e0
Login: 28714deb
Password: f8ddf95e1ba3

mrtuvn · 2023-04-21T14:55:24Z

After cross check browser i recognised results seem different between chrome - firefox (with options no capture option). Both scenarios use mobile with force touch enabled (simulator)
chrome: can swipe thumbnails, can next/navigate to image by dots below thumbnail BUT you can't trigger play video. Only way to open video is go to image at fullscreen mode and swipe to video
firefox: can swipe thumbnails, can next/navigate to image by dots below thumbnail -> can able to open video on click thumbnail video
Current look like chrome engine somehow not able allow to trigger video on mobile
cc: @cptX @rostilos any ideas

cptX · 2023-04-22T05:28:46Z

I was using firefox and I can ensure that swipe in mobile was not working when using the last patched fotorama.js file. The only patch that actually worked (and I have it in production now) is the patch @ line 1143 of the original code... It was like the rest changes in the file were blocking the change of the patch at line 1143. So for sure a complete test should be done including all senarios pc, mobile, images, videos etc...

mrtuvn · 2023-04-22T06:07:07Z

@cptX i have updated my code recently after your feedback. Give it a try if you need. Previous the patch i sent you works for images but it's not work well with mixed images-video case. I have changed some code after that. You can try the latest version in this merge and let me know the results. Have you try to add some video/youtube link combine with slide image product ?
The last weird thing i can't upload video (mov extension) to magento instance admin for test in real devices

mrtuvn · 2023-05-10T01:24:55Z

lib/web/fotorama/fotorama.js

@@ -1140,7 +1153,13 @@ fotoramaVersion = '4.6.4';

    function addEvent(el, e, fn, bool) {
        if (!e) return;
-        el.addEventListener ? el.addEventListener(e, fn, !!bool) : el.attachEvent('on' + e, fn);
+
+        const options = (Modernizr.passiveeventlisteners) ? {


just curiously do we actual need add passive for every dom element (or just add to body, window object) ? CC: @fredden
Can we add logic for check specific. Since browsers have updated a lot of changes and magento js system seem fall behind quite far

element window.document or window.document.body or window should be set passive false

Looking at this page on developer.mozilla.org, it seems that omitting 'passive' seems to be the best option as modern browsers* default the 'passive' option to a sensible value depending on the event.

* but not Safari. Safari seems to often lag when implementing features. For example, the 'loading=lazy' attribute for '<img>'.

yeap i think default passive enabled on safari and it's supported well
https://caniuse.com/passive-event-listener
only not support for retired IE browser

mrtuvn · 2023-05-12T11:19:53Z

Well seem internal already have updated code so i will mark this PR as draft
@cptX you can check code in this here
9d751c5#diff-fb7ac60e407ee78fc8e5ff56eef13de3f3bbe8a8ce52c14d674f0015f502e7c7
i saw some changes in lib/web/magnifier/magnify.js not only just fotorama. Please recheck with that too

ihor-sviziev · 2023-05-25T07:21:04Z

@mrtuvn, since the original issue already closed and marked as fixed, can we close this Pull Request?

mrtuvn · 2023-05-25T11:07:46Z

yeap let's close this one

Fix issue no touch event on mobile

0123048

mrtuvn mentioned this pull request Mar 17, 2023

Cannot slide images in product page in mobile in 2.4.6 only! #37232

Closed

5 tasks

m2-community-project bot added the Progress: pending review label Mar 17, 2023

mrtuvn marked this pull request as ready for review March 17, 2023 13:33

m2-community-project bot added the Priority: P1 Once P0 defects have been fixed, a defect having this priority is the next candidate for fixing. label Mar 17, 2023

mrtuvn added Reported on 2.4.6 Indicates original Magento version for the Issue report. Area: Lib/Frontend labels Mar 18, 2023

engcom-Hotel self-requested a review March 21, 2023 15:45

m2-community-project bot added Progress: review and removed Progress: pending review labels Mar 21, 2023

ihor-sviziev removed the Reported on 2.4.6 Indicates original Magento version for the Issue report. label Mar 22, 2023

m2-community-project bot added Progress: pending review and removed Progress: review labels Mar 22, 2023

ihor-sviziev added Award: bug fix Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests labels Mar 22, 2023

ihor-sviziev approved these changes Mar 22, 2023

View reviewed changes

m2-community-project bot assigned ihor-sviziev Mar 22, 2023

m2-community-project bot removed the Progress: pending review label Mar 22, 2023

leave passive with option only

fa7a9a4

mrtuvn added 2 commits May 10, 2023 08:18

Update code

0c73617

Update fotorama.js

9f2e517

mrtuvn commented May 10, 2023

View reviewed changes

mrtuvn requested a review from fredden May 10, 2023 15:20

mrtuvn marked this pull request as draft May 12, 2023 11:20

This comment was marked as off-topic.

Sign in to view

Merge branch '2.4-develop' into patch-fotorama-event-touch

d997cfc

ihor-sviziev added the Progress: needs update label May 25, 2023

m2-community-project bot removed the Progress: review label May 25, 2023

mrtuvn closed this May 25, 2023

m2-community-project bot removed the Progress: needs update label May 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issue no touch event on mobile #37234

Fix issue no touch event on mobile #37234

mrtuvn commented Mar 17, 2023 •

edited

Loading

m2-assistant bot commented Mar 17, 2023

mrtuvn commented Mar 17, 2023

magento-automated-testing bot commented Mar 17, 2023

mrtuvn commented Mar 18, 2023

magento-automated-testing bot commented Mar 18, 2023

mrtuvn commented Mar 18, 2023

magento-deployment-service bot commented Mar 18, 2023

magento-deployment-service bot commented Mar 18, 2023

ihor-sviziev left a comment

mrtuvn commented Apr 20, 2023

magento-automated-testing bot commented Apr 20, 2023

ihor-sviziev commented Apr 20, 2023

magento-automated-testing bot commented Apr 20, 2023

cptX commented Apr 21, 2023 •

edited

Loading

mrtuvn commented Apr 21, 2023 •

edited

Loading

cptX commented Apr 21, 2023 •

edited

Loading

mrtuvn commented Apr 21, 2023

magento-deployment-service bot commented Apr 21, 2023

magento-deployment-service bot commented Apr 21, 2023

mrtuvn commented Apr 21, 2023 •

edited

Loading

cptX commented Apr 22, 2023 •

edited

Loading

mrtuvn commented Apr 22, 2023 •

edited

Loading

mrtuvn May 10, 2023 •

edited

Loading

mrtuvn May 14, 2023

fredden May 14, 2023

mrtuvn May 15, 2023

mrtuvn commented May 12, 2023 •

edited

Loading

This comment was marked as off-topic.

ihor-sviziev commented May 25, 2023

mrtuvn commented May 25, 2023

Fix issue no touch event on mobile #37234

Fix issue no touch event on mobile #37234

Conversation

mrtuvn commented Mar 17, 2023 • edited Loading

Description (*)

Related Pull Requests

Fixed Issues (if relevant)

Manual testing scenarios (*)

Questions or comments

Contribution checklist (*)

m2-assistant bot commented Mar 17, 2023

mrtuvn commented Mar 17, 2023

magento-automated-testing bot commented Mar 17, 2023

mrtuvn commented Mar 18, 2023

magento-automated-testing bot commented Mar 18, 2023

mrtuvn commented Mar 18, 2023

magento-deployment-service bot commented Mar 18, 2023

magento-deployment-service bot commented Mar 18, 2023

ihor-sviziev left a comment

Choose a reason for hiding this comment

mrtuvn commented Apr 20, 2023

magento-automated-testing bot commented Apr 20, 2023

ihor-sviziev commented Apr 20, 2023

magento-automated-testing bot commented Apr 20, 2023

cptX commented Apr 21, 2023 • edited Loading

mrtuvn commented Apr 21, 2023 • edited Loading

cptX commented Apr 21, 2023 • edited Loading

mrtuvn commented Apr 21, 2023

magento-deployment-service bot commented Apr 21, 2023

magento-deployment-service bot commented Apr 21, 2023

mrtuvn commented Apr 21, 2023 • edited Loading

cptX commented Apr 22, 2023 • edited Loading

mrtuvn commented Apr 22, 2023 • edited Loading

mrtuvn May 10, 2023 • edited Loading

Choose a reason for hiding this comment

mrtuvn May 14, 2023

Choose a reason for hiding this comment

fredden May 14, 2023

Choose a reason for hiding this comment

mrtuvn May 15, 2023

Choose a reason for hiding this comment

mrtuvn commented May 12, 2023 • edited Loading

This comment was marked as off-topic.

ihor-sviziev commented May 25, 2023

mrtuvn commented May 25, 2023

mrtuvn commented Mar 17, 2023 •

edited

Loading

cptX commented Apr 21, 2023 •

edited

Loading

mrtuvn commented Apr 21, 2023 •

edited

Loading

cptX commented Apr 21, 2023 •

edited

Loading

mrtuvn commented Apr 21, 2023 •

edited

Loading

cptX commented Apr 22, 2023 •

edited

Loading

mrtuvn commented Apr 22, 2023 •

edited

Loading

mrtuvn May 10, 2023 •

edited

Loading

mrtuvn commented May 12, 2023 •

edited

Loading