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

Unnecessary dependency to jQuery might cause errors #213

Closed
uuf6429 opened this Issue Aug 6, 2018 · 2 comments

Comments

Projects
None yet
2 participants
@uuf6429
Copy link

uuf6429 commented Aug 6, 2018

In some places (Toolbar.php:122 and Toolbar.php:188), jQuery is used to add a classname (hover) to an element.

Expected behaviour

If the element is missing or for some reason jQuery is not loaded, this should degrade gracefully. In particular, I don't think it should expect or depend on jQuery.
I would expect an error similar to element not found or so.

Current behaviour

If for some reason the current page does not contain jQuery, the follow error is shown:

WebDriver\Exception\UnknownError: unknown error: jQuery is not defined

Possible solution

The code is not difficult to do in vanilla js. Since it's used in two places, maybe have a private method to do this.

    private function addHoverClass($id)
    {
        try {
            $this->getSession()->evaluateScript(
<<<"JS"
              const el = document.getElementById('$id');
              if (el) {
                const cl = el.className.split(' ');
                if (cl.indexOf('hover') === -1) {
                  cl.push('hover');
                  el.className = cl.join(' ');
                }
              }
JS
            );
        } catch (UnsupportedDriverActionException $e) {
            // This will fail for GoutteDriver but neither is it necessary.
        }
    }

Steps to reproduce (for bugs)

  1. Navigate to some page missing jQuery
  2. Call UserContext->iAmAnonymousUser() (via "And I log out" statement)

Context

Instead of seeing the actual error (eg; being logged out or ending up on a page without jQuery), this error is shown.

Environment

  • Behat 3, WordHat 2.0, PHP 7.1 (docker)
@paulgibbs

This comment has been minimized.

Copy link
Owner

paulgibbs commented Aug 14, 2018

Hi @uuf6429

Thanks, yes, you're right!

@paulgibbs paulgibbs added the bug label Aug 14, 2018

@paulgibbs paulgibbs added this to the Next Release milestone Aug 14, 2018

@paulgibbs paulgibbs added this to Next in WordHat roadmap. Aug 14, 2018

@paulgibbs paulgibbs modified the milestones: Next Release, 3.1 Aug 20, 2018

paulgibbs added a commit that referenced this issue Oct 6, 2018

Switch jQuery references with plain Javascript.
We should not assume that the WordPress site is reliant on jQuery.

Thanks to @uuf6429 for the great idea!

Fixes #213

@paulgibbs paulgibbs referenced this issue Oct 6, 2018

Closed

Switch jQuery references with plain Javascript. #221

3 of 9 tasks complete

paulgibbs added a commit that referenced this issue Oct 9, 2018

paulgibbs added a commit that referenced this issue Oct 9, 2018

paulgibbs added a commit that referenced this issue Nov 24, 2018

@paulgibbs paulgibbs referenced this issue Nov 24, 2018

Merged

Swap jQuery for plain Javascript. #225

0 of 9 tasks complete
@paulgibbs

This comment has been minimized.

Copy link
Owner

paulgibbs commented Nov 25, 2018

Swapped out!

@paulgibbs paulgibbs removed this from Next in WordHat roadmap. Dec 20, 2018

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