Problem/Motivation

When links in node previews are clicked, a modal is supposed to be presented to a user confirming that they want to navigate away from the preview. See #1440662: UX regression: Prevent links in node preview from being clicked for more details.

This behavior has apparently been broken since ef3674cc559358890e4cab01f1266503e2b16515, when jQuery was updated to 2.2.3 in April 2016!

Since we upgraded jQuery, any time any element in the node preview page is clicked, this error is thrown in the console:

Uncaught Error: Syntax error, unrecognized expression: [href^=#], #edit-backlink, #toolbar-administration a

This is happening because attribute selectors containing a hash have to quote the attribute value (ref https://github.com/jquery/jquery-migrate/issues/140). So [href^=#] is invalid, but [href^="#"] is OK.

Proposed resolution

Fix the bug by updating the selector, and write a Javascript test this time to prevent future regressions.

Remaining tasks

Write a failing test, then the simple fix.

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#4 2939885-4-test-only.patch1.89 KBsamuel.mortenson
#4 2939885-4.patch3.71 KBsamuel.mortenson
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

samuel.mortenson created an issue. See original summary.

samuel.mortenson’s picture

samuel.mortenson
he/him
Primary language English
CreditAttribution: samuel.mortenson at Acquia commented
Status: Needs review » Active
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Version: 8.4.x-dev » 8.5.x-dev
Priority: Normal » Critical

I think we actually treated this issue as a critical the last time around, so promoting accordingly. Lost user input is generally major per our definition at https://www.drupal.org/core/issue-priority#major-bug, but since this was "lost user input for the 80% usecase" it was serious enough to be considered a critical UX issue.

samuel.mortenson’s picture

samuel.mortenson
he/him
Primary language English
CreditAttribution: samuel.mortenson at Acquia commented
Status: Active » Needs review
FileSize
2939885-4.patch3.71 KB
2939885-4-test-only.patch1.89 KB

Attached a test-only and fix patch. I found three other bugs with the current file:

  1. The core/drupal.dialog library wasn't explicitly required
  2. The .content selector was used, which is specific to classy. There's no harm in targeting the entire document (context).
  3. Toolbar links were allowed to be clickable which doesn't make sense to me - we want to prevent all link-based navigation away from the preview. I removed that from the rule.

The last submitted patch, 4: 2939885-4.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 4: 2939885-4-test-only.patch, failed testing. View results

samuel.mortenson’s picture

samuel.mortenson
he/him
Primary language English
CreditAttribution: samuel.mortenson at Acquia commented
Status: Needs work » Needs review
timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood at Appnovation for Pfizer, Inc. commented
Status: Needs review » Reviewed & tested by the community

Looks good to me, nice to have a test for this now!

  • catch committed 3b304d2 on 8.6.x 
    Issue #2939885 by samuel.mortenson: UX regression (again): Prevent links...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed 3b304d2 and pushed to 8.6.x. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.