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

Fix for #14593 #16431

Closed
wants to merge 1 commit into from
Closed

Fix for #14593 #16431

wants to merge 1 commit into from

Conversation

0m3r
Copy link
Contributor

@0m3r 0m3r commented Jun 27, 2018

Description

Preconditions:

Magento version 2.2.4 with luma theme

Step to reproduce:

  • Open chrome developer console
  • Add a product to the cart.
  • From the mini cart in the upper right click the trash icon.
  • A confirmation modal will open.
  • Press Esc button

Expected Result

  • Close the confirm

Actual result

  • js error

https://github.com/magento/magento2/blob/2.2-develop/app/code/Magento/Checkout/view/frontend/web/js/sidebar.js#L96

Uncaught TypeError: Cannot read property 'stopImmediatePropagation' of undefined

2018-06-21 15-50-01

Fixed Issues

  1. Press Esc Key on modal generate a jquery UI error #14593

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 on Travis CI are green)

@magento-engcom-team
Copy link
Contributor

Hi @0m3r. 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-engcom-team give me test instance - deploy test instance based on PR changes
  • @magento-engcom-team give me {$VERSION} instance - deploy vanilla Magento instance

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

@@ -93,7 +93,9 @@ define([

/** @inheritdoc */
always: function (e) {
e.stopImmediatePropagation();
if (e && typeof e.stopImmediatePropagation === 'function') {
e.stopImmediatePropagation();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why it happens to be undefined?

Copy link
Contributor Author

@0m3r 0m3r Jun 29, 2018

Choose a reason for hiding this comment

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

the event e is undefined

function escapeKey lost event param
https://github.com/magento/magento2/blob/2.2-develop/app/code/Magento/Ui/view/base/web/js/modal/modal.js#L108

but for closeModal function event is required param
https://github.com/magento/magento2/blob/2.2-develop/app/code/Magento/Ui/view/base/web/js/modal/confirm.js#L89

always (sidebar.js:95)
closeModal (confirm.js:97)                  

closeModal: function (event, result) {
            result = result || false;

            if (result) {
                this.options.actions.confirm(event);
            } else {
                this.options.actions.cancel(event);
            }
            this.options.actions.always(event);
            this.element.bind('confirmclosed', _.bind(this._remove, this));

            return this._super();
        }

(anonymous) (jquery-ui.js:402)
escapeKey (modal.js:111)                   

                escapeKey: function () {
                    if (this.options.isOpen && this.modal.find(document.activeElement).length ||
                        this.options.isOpen && this.modal[0] === document.activeElement) {
                        this.closeModal();
                    }
                }

keyEventSwitcher (modal.js:180)
...

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @0m3r, thanks for collaboration. I think that always method should always receive event object. The correct way to fix this issue is fixing the place where event object was lost

@VladimirZaets VladimirZaets self-assigned this Jun 29, 2018
@0m3r 0m3r closed this Jul 2, 2018
@0m3r 0m3r mentioned this pull request Jul 2, 2018
4 tasks
magento-engcom-team added a commit that referenced this pull request Jul 3, 2018
 - Merge Pull Request #16477 from 0m3r/magento2:2.2-develop
 - Merged commits:
   1. 8399f63
   2. a6a17d3
   3. 21f85b4
magento-engcom-team pushed a commit that referenced this pull request Jul 3, 2018
Accepted Public Pull Requests:
 - #16009: fix: change "My Dashboard" to "My Account", fixes #16007 (by @DanielRuf)
 - #16477: Fix for #14593 (second try #16431)  (by @0m3r)
 - #16458: Add missing showInStore attributes (by @aschrammel)
 - #16438: Credit memo email template file: fixing incorrect object type error (by @JosephMaxwell)
 - #15464: Fix "Confirmation request" email is sent on customer's newsletter unsubscribe action (by @nuzil)
 - #16386: Login with wishlist raise report after logout. (by @swnsma)
 - #16372: Wishlist update item issue (by @eduard13)
 - #16086: Fix false cache_lifetime usage in xml layouts (by @yuriyDne)


Fixed GitHub Issues:
 - #14593: Press Esc Key on modal generate a jquery UI error (reported by @apomili) has been fixed in #16477 by @0m3r in 2.2-develop branch
   Related commits:
     1. 8399f63
     2. a6a17d3
     3. 21f85b4

 - #15218: "Confirmation request" email is sent on customer's newsletter unsubscription (reported by @densen45) has been fixed in #15464 by @nuzil in 2.2-develop branch
   Related commits:
     1. d790198
     2. 1f66421
     3. f80eb03
     4. 3227832
     5. 69eb20f
magento-engcom-team added a commit that referenced this pull request Jul 11, 2018
 - Merge Pull Request #16623 from mageprince/magento2:2.3-develop-PR-port-16477
 - Merged commits:
   1. 0fdc0a8
   2. 07f7d83
   3. 160bc5d
magento-engcom-team pushed a commit that referenced this pull request Jul 11, 2018
Accepted Public Pull Requests:
 - #16668: Fix missing PHPDocs hinting for AdvancedPricingImportExport module (by @mageprince)
 - #16659: [Forwardport] dev:di:info duplicates plugin info (by @coderimus)
 - #16670: [Forwardport] Removed unused class from forms less file. (by @mageprince)
 - #16664: [Forwardport] 7399-clickableOverlay-less-fix - added pointer-events rule to .modal-� (by @mageprince)
 - #16642: [Forwardport] Variable as a method parameter might be overridden by the loop (by @lfluvisotto)
 - #16600: [2.3-develop][ForwardPort] Fixed backwards incompatible change to Transport variable event parameters (by @gwharton)
 - #16640: Trim email address in newsletter, forgot password, checkout login and email to a friend form (by @gelanivishal)
 - #16607: Add spelling correction: formatedPrice to formattedPrice (by @arnoudhgz)
 - #16611: [Forwardport] Removed extra code (by @gelanivishal)
 - #16638: [Forwardport] Update checkout translations (by @JeroenVanLeusden)
 - #16635: [Forwardport] Updated SynonymGroup.xml (by @sanganinamrata)
 - #16348: [Port 2.3] Captcha: Added integration tests for checking customer login attempts cleanup (by @rogyar)
 - #16623: [Forwardport] Fix for #14593 (second try #16431)  (by @mageprince)


Fixed GitHub Issues:
 - #7399: Modal UI: clickableOverlay option doesn't work (reported by @thdoan) has been fixed in #16664 by @mageprince in 2.3-develop branch
   Related commits:
     1. 76ae28f

 - #10210: Transport variable can not be altered in email_invoice_set_template_vars_before Event (reported by @diybook) has been fixed in #16600 by @gwharton in 2.3-develop branch
   Related commits:
     1. cdbd324

 - #6058: IE11 user login email validation fails if field has leading or trailing space (reported by @dnadle) has been fixed in #16640 by @gelanivishal in 2.3-develop branch
   Related commits:
     1. 6fd1eda
     2. f00c37a
     3. 1888b1e
     4. 0653f09

 - #14593: Press Esc Key on modal generate a jquery UI error (reported by @apomili) has been fixed in #16623 by @mageprince in 2.3-develop branch
   Related commits:
     1. 0fdc0a8
     2. 07f7d83
     3. 160bc5d
magento-engcom-team added a commit that referenced this pull request Aug 1, 2018
 - Merge Pull Request #17223 from gelanivishal/magento2:2.1-develop-PR-port-16477
 - Merged commits:
   1. b50caeb
   2. 540cdb5
   3. d937ca7
magento-engcom-team pushed a commit that referenced this pull request Aug 1, 2018
Accepted Public Pull Requests:
 - #17240: [Backport] Resolved : Mobile device style groups incorrect order (by @tiagosampaio)
 - #17223: [Backport] Fix for #14593 (second try #16431) (by @gelanivishal)
 - #17211: [Backport] Fixed ability to set field config from layout xml #11302 (by @mageprince)
 - #17212: [Backport] Magento sets ISO invalid language code (by @mageprince)
 - #17213: [Backport 2.1] Fix "pattern" UI Component validation (by @mageprince)


Fixed GitHub Issues:
 - #14476: Mobile device style groups incorrect order in _responsive.less (reported by @damiandawber) has been fixed in #17240 by @tiagosampaio in 2.1-develop branch
   Related commits:
     1. b957e32

 - #14593: Press Esc Key on modal generate a jquery UI error (reported by @apomili) has been fixed in #17223 by @gelanivishal in 2.1-develop branch
   Related commits:
     1. b50caeb
     2. 540cdb5
     3. d937ca7

 - #11540: Magento sets iso invalid language code in html header (reported by @SirElectro) has been fixed in #17212 by @mageprince in 2.1-develop branch
   Related commits:
     1. 6702cdc

 - #9919: Pattern Validation via UI Component Fails to Interpret String as RegEx Pattern (reported by @bap14) has been fixed in #17213 by @mageprince in 2.1-develop branch
   Related commits:
     1. 64bad6b
     2. 50f4051
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.

4 participants