Default image is not loaded after removing media

Created on 21 February 2025, 3 months ago

Overview

After removing a media from a field, the default image is not loaded anymore. Instead, the component continues to display the removed media.

Proposed resolution

User interface changes

🐛 Bug report
Status

Active

Version

0.0

Component

Page builder

Created by

🇫🇮Finland lauriii Finland

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @lauriii
  • 🇫🇮Finland lauriii Finland
  • 🇫🇮Finland lauriii Finland
  • 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

    We'll need to check if clicking the button triggers the appropriate Redux state changes.

  • 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
  • 🇺🇸United States bnjmnm Ann Arbor, MI

    There's some trickiness here (surprise!) because all of the current redux-updating is based on changes to input values, but when an item is removed via media library, this is reflected in the form/ui by removing the correspnding hidden <input /> that represents the selection.

    I have some ideas on how to deal with this...

  • 🇺🇸United States bnjmnm Ann Arbor, MI

    Current MR gets us closer to fixing it. Currently works best when the UI is loaded and the image prop already has media.

    It is still buggy when new media is added and then removed. I assume it's because the component is now removed-element aware, but there isn't yet a full equivalent for additions

  • 🇬🇧United Kingdom f.mazeikis Brighton
  • 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

    @f.mazeikis re-discovered this same bug. @longwave is going to update the media-library.cy.js e2e test.

    During pair (triple?) debugging we discovered that the client-side model never got updated. Our going theory is that the mediaSelection client-side transform added in 📌 Move clientside assumptions about prop form data shape into a series of prop specific transforms Active is not firing when removing an image.

  • First commit to issue fork.
  • Merge request !811Resolve #3508170 "Image in preview" → (Open) created by longwave
  • 🇬🇧United Kingdom longwave UK

    Rebased and added assertions to media-library.cy.js to demonstrate the issue. The canvas starts with a sample image inserted, which we assert correctly, then the Remove button is clicked, which (I believe) should fall back to the example image for the component - but this doesn't happen at the moment so the test fails.

  • Pipeline finished with Failed
    2 months ago
    Total: 2066s
    #454220
  • Pipeline finished with Failed
    2 months ago
    Total: 1048s
    #454388
  • Pipeline finished with Failed
    2 months ago
    Total: 1620s
    #454399
  • Pipeline finished with Failed
    2 months ago
    Total: 1844s
    #454454
  • 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
  • 🇬🇧United Kingdom longwave UK

    Removed the SDC changes, as per Wim we can use image-optional-with-example-and-additional-prop for testing here.

    There is something wrong with the changes to the frontend in this MR, as this now has the same result as 🐛 SDCs with optional images without examples cannot be placed Active when testing manually with image-optional-with-example-and-additional-prop - as soon as I select a valid media entity then the form errors out with a 400 error because the prop is missing. I've tried making some changes in the frontend code but I'm not sure what I'm doing or where it's going wrong so assigning to @bnjmnm for more investigation.

  • Pipeline finished with Failed
    about 2 months ago
    Total: 1744s
    #470468
  • 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10

    https://git.drupalcode.org/project/experience_builder/-/blame/0.x/ui/src... is the likely cause (disclaimer didn't check the MR)
    When we click remove there's no new elements in the ajax response and hence nothing to trigger the model update via this event

  • Pipeline finished with Failed
    about 2 months ago
    Total: 1490s
    #470619
  • Pipeline finished with Failed
    about 2 months ago
    Total: 182s
    #471544
  • Pipeline finished with Failed
    about 1 month ago
    #487305
  • Pipeline finished with Failed
    about 1 month ago
    Total: 320s
    #487324
  • Pipeline finished with Failed
    about 1 month ago
    Total: 669s
    #487347
  • Pipeline finished with Canceled
    about 1 month ago
    Total: 137s
    #487473
  • Pipeline finished with Canceled
    about 1 month ago
    Total: 152s
    #487475
  • Pipeline finished with Failed
    about 1 month ago
    Total: 590s
    #487479
  • Pipeline finished with Success
    about 1 month ago
    Total: 869s
    #487507
  • Assigned to bnjmnm
  • Status changed to Needs review about 1 month ago
  • 🇺🇸United States bnjmnm Ann Arbor, MI
  • 🇫🇮Finland lauriii Finland

    Tried to test this and it doesn't seem to be doing what's expected. The image persists but I'm also unable to make changes to other props after removing the image 🤔

  • 🇺🇸United States bnjmnm Ann Arbor, MI

    @lauriii Id like to try and reproduce your findings to narrow down what is going on, but it looks like the component you are testing with is called "Image with text", which is not one available in the experience builder codebase.

    The MR was tested with the various image-including components provided by the xb_test_sdc module, as well as the standard image component. I'd like to see what is different about the one in #18 and figure out which side things need to be addressed.

  • 🇺🇸United States bnjmnm Ann Arbor, MI

    The component used for the review in #18 is a custom code component. I think it's still worth reviewing with the included-with-experience-builder components the MR was tested with as it would be good to get feedback on the solution approach sooner than later.

    Once I have access to & can successfully import the custom component from #18 I can see why that is running into problems, but that probably should preclude getting eyes on the solution that has solved the reported symptom for the components that are available to anyone with the Experience Builder codebase.

  • 🇺🇸United States bnjmnm Ann Arbor, MI

    Also, with the current solution in the MR any removed image reverts to the example when one is available, otherwise it is removed from the preview. The IS suggests there might be interest in different logic depending on whether or not the image prop is required. That should be a feasible expansion of the current logic if needed.

  • 🇫🇮Finland lauriii Finland

    Tested this with the built in components and couldn't reproduce #18 🐛 Default image is not loaded after removing media Active with them. I attached config for the component that allows you to reproduce this.

    When remove is clicked on an optional image, it should not fallback to the example image.

    Keeping this needs review in case that the approach needs review.

  • 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
  • Pipeline finished with Failed
    28 days ago
    Total: 638s
    #489457
  • Pipeline finished with Failed
    27 days ago
    Total: 753s
    #489506
  • Pipeline finished with Failed
    27 days ago
    Total: 643s
    #489559
  • Pipeline finished with Failed
    27 days ago
    Total: 6925s
    #489566
  • Pipeline finished with Failed
    27 days ago
    #489724
  • Pipeline finished with Failed
    27 days ago
    #490412
  • Pipeline finished with Failed
    27 days ago
    #490544
  • Pipeline finished with Canceled
    27 days ago
    #490546
  • Pipeline finished with Running
    27 days ago
    #490558
  • Pipeline finished with Canceled
    27 days ago
    #490557
  • 🇺🇸United States bnjmnm Ann Arbor, MI

    Only test fails are a common random failer. Will click retry and it may be green by the time anyone sees it.

  • 🇺🇸United States bnjmnm Ann Arbor, MI

    But I found a bug when testing with a JS component that has both the image prop and a text prop. Here in this GIF, I removed the media image, but then the text prop content also disappears from the preview, but it still exists in the form. And then when you type in the text prop, it does return to the preview.

    I expanded the two e2e tests at the end of media-library.cy.js to include this scenario and they are passing. Perhaps the issue was resolved by a recent commit - many bugfixes recently.

  • Pipeline finished with Success
    25 days ago
    Total: 2319s
    #492518
  • 🇬🇧United Kingdom longwave UK

    I manually tested this and it works great but I'm a bit uneasy about the isRemovedProp() method as well - I think we should just be able to drop the values from resolved and source entirely and let the server side figure out that we need to go back to the default?

  • Pipeline finished with Canceled
    19 days ago
    Total: 555s
    #497407
  • Pipeline finished with Canceled
    19 days ago
    Total: 196s
    #497411
  • Pipeline finished with Failed
    19 days ago
    Total: 1706s
    #497412
  • 🇺🇸United States bnjmnm Ann Arbor, MI

    Good news
    It looks like changes in 0.x that happened since I came up with the solution ~3 months ago means there's way less backend hacky stuff needed.

    Bad news
    Getting the e2e tests to pass reliably - despite the IRL version working fine - is not going smoothly at all. I'll just set this to needs work until I get that slop more agreeable.

  • Pipeline finished with Success
    14 days ago
    Total: 654s
    #501068
  • Pipeline finished with Success
    13 days ago
    Total: 687s
    #501222
  • 🇺🇸United States bnjmnm Ann Arbor, MI
  • 🇫🇮Finland lauriii Finland
  • Pipeline finished with Success
    13 days ago
    Total: 8104s
    #501776
  • Pipeline finished with Success
    12 days ago
    Total: 782s
    #502915
  • 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

    Marked 🐛 Cannot remove media Active as a duplicate of this.

Production build 0.71.5 2024