Steps to reproduce:

  1. Go to a Landing Page
  2. Click "Customize this page"
  3. Click the plus button in one of the regions
  4. Click "Add text"
  5. Set the title to: Here's a title
  6. Wait for the preview to reload, you'll see the title appear correctly
  7. Click "Make title a link"
  8. Wait for the preview to reload
  9. You'll now see it incorrectly includes an HTML entity: Here#s a title

This only happens in the preview - when rendering the FPP on the page, the title looks fine.

CommentFileSizeAuthor
#12 panopoly_test-double-encoded-fpp-link-2483295-12.patch1.14 KBdsnopek
#11 panopoly_magic-double-encode-fpp-link-2483295-11.patch574 bytesdsnopek
#8 panopoly_test-title_in_fpp_is_double-2483295-8.patch1.1 KBmglaman
#5 title_in_fpp_is_double-2483295-5.patch1012 byteshumansky
#4 title_in_fpp_is_double-2483295-4.patch678 byteshumansky
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
CreditAttribution: dsnopek commented
Issue summary: View changes

Blergh. Trying to get the entity rendered right in the summary.

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
CreditAttribution: dsnopek commented
Issue tags: +Needs tests, +sprint

Since this is Panopoly Magic, we'll need a test that does the steps above.

humansky’s picture

humansky CreditAttribution: humansky as a volunteer commented

Here's the behat test that will replicate the issue:

  @api @javascript @panopoly_widgets
  Scenario: Make text widget title a link
    Given I am logged in as a user with the "administrator" role
    And Panopoly magic live previews are automatic
    And I am viewing a landing page
    When I customize this page with the Panels IPE
    And I click "Add new pane"
    And I click "Add text" in the "CTools modal" region
    Then I should see "Configure new Add text"
    When I fill in the following:
      | Title   | Here's a title          |
    And I check the box "Make title a link"
    And I wait for AJAX to finish
    Then I should see "Here's a title"

Should this scenario be with the text_widget.feature test? Perhaps wee should consolidate all the related tests together?

humansky’s picture

humansky CreditAttribution: humansky as a volunteer commented
FileSize
title_in_fpp_is_double-2483295-4.patch678 bytes

Adding patch to fix the quote encoding issue with live preview

humansky’s picture

humansky CreditAttribution: humansky as a volunteer commented
Status: Active » Needs review
FileSize
title_in_fpp_is_double-2483295-5.patch1012 bytes

Adding behat test for quotes in title. Eventually we can combine the scenarios into one, but for now, I created a separate scenario.

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
CreditAttribution: dsnopek commented
Status: Needs review » Needs work

Thanks for working on this! :-)

Should this scenario be with the text_widget.feature test? Perhaps wee should consolidate all the related tests together?

Actually, this shouldn't be in mixed in the text_widget.feature, because that file is meant to be testing only the Text widget.

The functionality that's broken here actually comes from panopoly_magic (I think) and using the Text widget here is just a means to an end. Ideally, actually, panopoly_test would declare it's own simple FPP and we'd use that so that the Text widget (and panopoly_widgets in general) didn't need to be involved - there's an issue for that already:

#2433037: Decouple panopoly_magic tests from panopoly_widgets

Eventually we can combine the scenarios into one, but for now, I created a separate scenario.

Being seperate scenarios is correct!

Marking as "Needs work" to at least move it to somewhere panopoly_magic specific (like maybe livepreview.feature?).

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
CreditAttribution: dsnopek commented

I forgot to mention that I ran the test and it worked as it should, so the actual content of it is good!

mglaman’s picture

mglaman
Primary language English
Location WI, USA
CreditAttribution: mglaman at Gaggle.net, Inc. commented
Status: Needs work » Needs review
FileSize
panopoly_test-title_in_fpp_is_double-2483295-8.patch1.1 KB

Reroll of patch to move it to livepreview.feature as #6.

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
CreditAttribution: dsnopek commented

@mglaman: Thanks! Did you test that it still works with Behat 3? I don't see any problematic steps, and it'll need to be tested anyway when we try have a fix, but just wondering if you actually ran it.

cboyden’s picture

cboyden CreditAttribution: cboyden commented
Title: Title in FPP is double encoded when making a link (preview only) » Title in FPP is double encoded when making a link
Status: Needs review » Needs work

I am seeing this problem with ampersands and right/left angle brackets on the rendered page as well. It's not limited to the live preview.

The patch in #4 still applies, with an offset, but it doesn't fix ampersands or right/left angle brackets.

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
CreditAttribution: dsnopek commented
FileSize
panopoly_magic-double-encode-fpp-link-2483295-11.patch574 bytes

Still not sure where the second round of encoding is coming from for links, but this should be filter_xss_admin() rather than check_plain() or htmlentities() in order to match the output when rendering the FPP.

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
CreditAttribution: dsnopek commented
FileSize
panopoly_test-double-encoded-fpp-link-2483295-12.patch1.14 KB

Here's a new version of the test patch which includes an ampersand in it per #10. This should fail until we commit #2717509: Update to Fieldable Panels Panes 1.10 for SA-CONTRIB-2016-025.

EDIT: Here's a Travis build: https://travis-ci.org/panopoly/panopoly/builds/127615600

  • dsnopek committed 3ea3207 on 7.x-1.x 
    Update Panopoly Magic and Test for Issue #2483295 by dsnopek, humansky,...
dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
CreditAttribution: dsnopek commented
Status: Needs work » Fixed

Committed to expedite release! I expect this will pass tests:

https://travis-ci.org/panopoly/panopoly/builds/127874683

But if it doesn't we'll revert and fix.

Status: Fixed » Closed (fixed)

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