🇺🇸United States @bnjmnm

bnjmnm → made their first commit to this issue’s fork.

The cleanup looks good!

This is because the linkers are (per design) associated with a form input's , but in the datepicker's case the labeling is in the of the fieldset. There is already special casing for media, and it will likely need to generalize this for any widget using this fieldset approach.

bnjmnm → made their first commit to this issue’s fork.

Setting to NR, but aware the MR is not mergeable as-is. Per the steps in #12, some of what is there is to make manual testing easier.

Could use some eyes on this - it does prevent AJAX / layout PATCH requests from happening simultaneously, but still not 100% certain it addresses the underlying problems that might be caused by the simultaneous invocations.

In its current state, the 3521641-ajax-patch-traffic MR has some extra stuff specifically for manually testing.

1. enable the canvas_test_autocomplete module as autocomplete is unique in that it calls the layout PATCH request on blur
2. Go to the instance form for a component with a text field that isn't autocomplete
3. You'll see instructions on how to test both scenarios
   
4. It's easier to make the AJAX vs PATCH happen with some level of throttling
5. The console will report when AJAX/PATCH was delayed while another one was triggered
6. To help with test timing, the "Click to test AJAX vs PATCH" button text will be red while the patch request is in progress

I had thought the delaying of the AJAX while PATCH completely could cause issues as it could be a stale element, but so far I haven't noticed any problems. If a reviewer can surface anything that's great - I want to be sure this covers all the use cases.

bnjmnm → made their first commit to this issue’s fork.

TY @hooroomoo, the redirect now checks to see if there's a back-button path and redirects to that.

The component in question also fails to render outside of Canvas because it is in fact malformed (@mherchel suspected this might be the case in #2)

{{ image }} should instead be

  <img src="{{ image.src }}" alt="{{ image.alt }}" width="{{ image.width }}" height="{{ image.height }}" />

But as @mherchel stated the error itself is not really the point - of course a malformed component will error! The issue is that one malformed component is making the entire component list unavailable.

The approach in the MR I just pushed catches malformed components on a per-component basis. When caught, the broken property is set to TRUE. And it will be listed, but disabled, in the components list. Not sure if this is the way to go, but there's still a preview on hover, but in this case is displays the rendering error.

bnjmnm → made their first commit to this issue’s fork.

The UiShell is a good addition - centralizes some things worth centralizing.

Left a few small comments on the MR, nothing major.

bnjmnm → made their first commit to this issue’s fork.

The scenario shown in the issue summary gif was one I couldn't reproduce as there wasn't a delete option for the components in Library.

I was able to reproduce the scenario in #2 and pushed an MR that fixes it.

Go ahead an set back to Active if there are steps to get this happening in Library. There were other recent changes with deletion so its also possible it's not available in that menu anymore.

bnjmnm → made their first commit to this issue’s fork.

Full support for removing optional images this was added earlier this year, and there's an e2e test in media-library.cy.js that should have prevented this, it's even called
'Can remove an optional code component image with example and there is no image in the preview'

Unfortunately, there's a delay before the default image returns, so the assertion that confirms it's not there fires before the unwanted reappearance.

With the help of git bisect I confirmed the problem was introduced in late July as part of ✨ Add a Video prop type to the Code Component editor Active .

Ensure the radix select widget is entirely visible inside the viewport. Or just use native selects?
Canvas has been using native selects since 📌 Get Semi-coupled Select form elements to work with the State API Postponed ... but I was also able to reproduce what you reported.

It looks like this is happening because of some styling added in Select.module.css as part the issue where the select component was refactored to use a native select.

This little bit here was part of an attempt to preserve the radix visuals as much as possible.

  &::picker(select) {
    top: -40px;
    left: -10px;

That offset results in the select options appearing in roughly the same position as they would with Radix, but because it's just a basic offset, it can unfortunately push the options above the viewport. My leaning is to remove this and let select be select - it's a pretty quick MR to provide & hopefully either that will be approved or it will inspire an alternate solution that still solves the problem.

bnjmnm → made their first commit to this issue’s fork.

FB addressed and reasonably sure the PR test fails are unrelated but will keep re-running.

bnjmnm → created an issue.

Although I may not have everything fully figured out in 🐛 Problem if twocomponents are added in VERY quick succession. Active @jessebaker agreed enough has been confirmed that we can safely say the problem is unrelated to the transforms-specific problems reported in this issue

This issue was filed due to media-video-prop.cy.js failing without adding what should technically be an unnecessary wait

I dug into this and found the problem. In the test:

1. Two Video components are added in quick succession
2. The first-added Video is selected by clicking it in the layout
3. Some values are filled out in the Video props form, including setting the width to 190
4. The test fails because the layout does not update so video 1 has a width of 190

So what is happening?

If we screenshot the test right before we check for that 190, you'll see that Video 2 has the width of 190

Why? Because even though Video 1 is clicked in step 2, its form hasn't loaded yet. The available form still belongs to Video 2 so that is the component being edited despite the test looking for changes in Video 1.

Per @jessebaker: he can still reproduce #9.

Doesn't seem likely he attempted to reproduce #9 - it looks he checked if this MR would fix an e2e test issue that also happened to deal with video components. I (and others) suggested this issue might fix it.. but ultimately what he's running into is not related to what is reported here. I reproduced the test failures in @jessebaker 's commit and the transforms were intact the entire time

I went ahead and fixed that test failure anyway because I'd hate to further slow down this issue landing based on differing understandings of the issue's scope. Plus, fixing it was pretty easy.

The solution for the failing test is not transforms related, but is evidence of something not working 100% correctly, so I created 🐛 Problem if twocomponents are added in VERY quick succession. Active to track that.

bnjmnm → created an issue.

bnjmnm → made their first commit to this issue’s fork.

When component cannot be deleted, the delete link should appear disabled. When the link is being hovered, it should display a tooltip:

Can't delete components that are being used..

When user gets to the dialog we should always display following text:

You are about to permanently delete the [Component Name] component.

We should also display this if there are past revisions where the component is being used:

This will break [X] past versions. Reverting to past versions that rely on this component will appear broken.

Left a bit of FB on the MR.

I also did some additional digging just to make sure there wasn't a straightforward way to do these updates without the deslect-reselect step. There's nothing to suggest this is impossible, but it might be a whole-sprint+ to accomplish, so I think this is very much worth bringing in until there's bandwidth for the deluxe version.

There is some logic that removing media items uses that was implemented before array component props were possible.... When the removal occurs it essentially resets the entire prop value. This was fine for single cardinality media... but obviously not good for multi. I know where it's happening and it shouldn't be too difficult to expand the functionality to support multi.

I mentioned this in a todo on 🐛 Editing support for type: array Active , but it's not clear if it will be part of the final scope, so lets keep this issue open to ensure visibility.

From what I can tell the circumstances in #6 that switched this back to NW is a known problem - it was pointed out to me on Sept 16.
However, I could not find anything in the queue representing that, though, so at the risk of creating a dupe I filed 🐛 Can't link field to non-required prop if it's empty on the entity. Active

bnjmnm → created an issue.

This MR doesn't fix it for me, unfortunately:

It looks like that's happening when you link the Revision Log Message. That was something I was told last week is a known issue. It happens when the revision log (and presumably other fields) are empty and the prop is optional. I assumed it was a filed issue, but perhaps it's not?

I get the same symptoms unless I populate that message.

Additional info

When the message is empty, the evaluation in \Drupal\canvas\Plugin\Canvas\ComponentSource\GeneratedFieldExplicitInputUxComponentSourceBase::getClientSideInfo
returns NULL.

If $evaluated is null and the prop isn't required, that prop is omitted from the return value of getClientSideInfo

Which means when we check for source in buildComponentInstanceForm, there is no $inputValues[$sdc_prop_name] and we don't get the $source that would confirm this is dynamic.
$source = $this->uncollapse($inputValues[$sdc_prop_name] ?? NULL, $sdc_prop_name);

bnjmnm → made their first commit to this issue’s fork.

bnjmnm → changed the visibility of the branch 3548395-link-ml to hidden.

bnjmnm → changed the visibility of the branch 3548395-linkedpropsource-render-element to hidden.

Look at the chains

Media Library doesn't use a label element, it uses a fieldset legend.

Once that is accounted for, it works.

bnjmnm → made their first commit to this issue’s fork.

I have a separate MR up 3535024-fix-with-window that builds on 3535024-investigate-why-the

It includes updating the global window object, which I know is sometimes side-eyed, hence the separate MR.

The underlying issue appears to be when the form is rebuilt via AJAX. InputBehaviorsComponentPropsForm does not initially have access to the TransformsContext when it's generated via an AJAX rebuild, so I'm persisting the transforms globally.

Perhaps with that explanation there's something we can leverage between window and context, but there is a fix available if needed

Manual reviews had been running into problems where a prop is linked, but changing the value of a different prop results in it being unlinked.

I believe this is now fixed, but there's an undesirable UI side effect - we need to temporarily deselect the form when a prop is linked. This is something that was necessary for the AI Wizard as well.

Visually, it was fine when the page data tab was present to maintain the width of the right sidebar. With templates, the sidebar vanishes momentarily, which isn't ideal. There are many ways to improve this, but it's worth deciding if that has to be done here, or if it can be moved to a followup so we can get the basic linking functionality into HEAD sooner.

Although it does not result in any errors, I can follow the steps to reproduce and have it so the transforms are unavailable. I noticed when that is the case, the transforms are an empty object, instead of the default FALSE this MR sets it to - so I think it may be getting explicitly replaced somewhere.

3540578-adding-components-to branch has the basic functionality working.
Some things I still know are needed:

• Add functionality for dragging out of a folder but not into a new one
• This isn't styled at all - the hovered folder has an intentionally-temporary ugly red outline rn
• Possibly some kind of throbber after the drag completes as the move is not instantaneous.

And undoubtedly more, but the essentials are in place.

bnjmnm → changed the visibility of the branch 3540578-adding-components-to to hidden.

bnjmnm → changed the visibility of the branch 3540578-dnd-folders to hidden.

bnjmnm → made their first commit to this issue’s fork.

Ben, did you really mean to change #3540577-17: Displaying and creating folders to #3540577: Displaying and creating folders?

I meant to change a broken link to that issue, but looks like I changed the working one instead.

I approved the MR based on what is there. It seems like common sense changes:

• represent the lack of transforms with an unambiguous FALSE instead of a truthy empty object,
• Don't render the form until we know transforms are available.

I'm not quite prepared to merge as I can't be certain this meets the issue summary criteria, nor can I reproduce the symptoms. However, this MR does address something that could result in the problem reported, but it's not clear if it is the cause.

IMO there would be no problem merging it, but we might need some attempts to reproduce the bug with/without MR before the BE workaround could be removed.

I pushed 46cb5b35 to fix sparkline, but it fixed the image gallery as well

Still needs tests so it doesn't regress. Probably a good idea to really look at my commit fix too. It certainly works for the components that were previously broken, but there may be additional use cases.

bnjmnm → made their first commit to this issue’s fork.

That sounds good to me. I was part of the efforts to transition Bartik out of core, but do not actually use it (which is also true of at least one of the other maintainers). It will be good for the project to have a maintainer who actively uses it. I'll go ahead and add Liam - I've seen him on D.O. enough to know he's legit 🙂.

bnjmnm → made their first commit to this issue’s fork.

I also noted that if I tried to the link the "revision log" to the sub heading this did not work. I think it might be because the revision log was empty. If I updated the revision log so it was not empty it worked

In \Drupal\canvas\Plugin\Canvas\ComponentSource\GeneratedFieldExplicitInputUxComponentSourceBase::clientModelToInput
There's a bit that reconciles the host entity values
$evaluated = $source->evaluate($host_entity, $is_required_prop);

If the host entity value is empty, this returns NULL, and it results in it not being added to the model that is ised to inform \Drupal\canvas\Plugin\Canvas\ComponentSource\GeneratedFieldExplicitInputUxComponentSourceBase::buildComponentInstanceForm that the prop is dynamic.

With a little more xdebugging I can probably figure it out, but someone who spends more time in there might be able to do it quicker.

There is a FE bug right now where if u

add a heading component
link the 'text' prop to a field,
change the 'style' prop to secondary

Then it reverts the 'text' prop to its original static value.

After some debugging I found that after doing step #2 of linking a prop to a field, the selectedModel.source looks to be correct but the selectedModel.resolved value is wrong and is the static value. So I think because the resolved value is wrong, the FE sends a request with the wrong values to the BE and gets the wrong model back.

But also I'm not familiar with how source and resolved values are handled in the backend so hoping someone with more knowledge there knows how to fix this.

Fixed this - it required the layoutModel AND the formState stores be updated when a prop is linked

bnjmnm → made their first commit to this issue’s fork.

Setting to NR - the playwright tests have yet to fully pass, but each run has a different combination of specs+browsers failing. In the many times I've re-run Playwright, every test with every browser has passed multiple times, so this appears to be random and unrelated to the changes in the MR. I'll keep re-running in hopes of it hitting that confidence-inspiring green.

I'm setting to NR now that 1.x has the component form + template.

Did not have much of an opportunity to manually test things now that 1.x is merged - lost a few hrs trying to narrow down Playwright flakiness but it's different spec+browser combos failing on each run so I'm guessing it's bad timing more than actual regressions.

This is covered in 📌 Manage Library / Code Component UI refinements Active which was NR in experience builder but needs a bit of attention now that it is Canvas.

setting as postponed instead of duplicate in case there's a greater affinity for this issue

bnjmnm → changed the visibility of the branch 3543804-manage-library- to hidden.

This is covered as part of 📌 Have "Add Folder" form use @/components/Dialog Active which had a working MR with experience_builder and should have a canvas version shortly. Will set this to postponed instead of duplicated so this remains invisible if it's decided to address that without the Dialog support.

This is the current state. Linking works. Need to confirm if unlinking should be part of this too, and there's undoubtedly other things to fine tune

bnjmnm → changed the visibility of the branch 3541037-allow-linking-a to hidden.

bnjmnm → made their first commit to this issue’s fork.

This looks fine and now I better understand why we didn't bother with some of the helpers I suggested in the routing issue that preceded this

bnjmnm → made their first commit to this issue’s fork.

bnjmnm → changed the visibility of the branch 3538562-provide-salient-warning to hidden.

MR 1 is committed, MR 2 is in progress, the add template dialog works but needs some loading spinners for the view modes request, and the form isn't quite what I'd expect if I choose a content type that already has a template assigned to the "full" view mode.

It should be jump-onable by folks in other timezones - it all works and the TypeScript is sorted out, but needs some refining. The dialog props were set up knowing it will evenually be opened by a bundle's contextual menus, too.

bnjmnm → made their first commit to this issue’s fork.

bnjmnm → made their first commit to this issue’s fork.

Followups created as needed:

• Already addressed in this issue
• Appearance of empty folders need to be adjusted, see #note_575777.
• Components and patterns should not be draggable to the canvas from Manage library, see #note_575781
• ✨ Broaden Manage Library -> Code Tab scope Active Covers "Display both internal and exposed code components in Manage library under the Code tab. See #16". I know it was stated above that this was already addressed and discussed in #20 but I'm not seeing exposed components in the Code tab, just internal - if I missed something here sorry about the noise
• 📌 Folder style updates Postponed Covers "Appearance of hovered folders need to be adjusted, see #note_575778."
• 📌 Have "Add Folder" form use @/components/Dialog Active Covers "Adding new folders should use @/components/Dialog and handle errors, see #note_575780."
• 📌 Manage Library / Code Component UI refinements Active Covers
  • When a new code component is created from Library, the Code tab of Manage library should be opened.
  • When a code component is clicked under Manage Library, and the current route is not the code editor, the Code tab should remain selected.

bnjmnm → created an issue.

bnjmnm → created an issue.

bnjmnm → created an issue.

bnjmnm → created an issue.

media-library-component-instance has a fix snuck into an unrelated not-yet-merged MR, but moving iterations inside the testMediaLibraryInComponentInstanceForm should take care of it. (has worked thus far at least)

There's a 2nd media library test that should get a similar change.

global-regions-interact.cy.js is the renderer crashing. This was a bigger problem with more tests before we parallelized, and this seems to be the only one continuing to suffer. It certainly can't hurt to make this the next playwright port, but a quick way that might reduce some pain here is to split it into it's own test group as it seemed like the renderer crashes were related to the number of tests in a run - but I'm not certain about this so it's only something to try if it is fast.

bnjmnm → created an issue.

Failing test is just a flaky Playwright

penyaskito → credited bnjmnm → .

bnjmnm → created an issue.

bnjmnm → made their first commit to this issue’s fork.

Setting to NR. e2e tests, particularly Playwright at the moment, are a bit flaky, so don't let fails there get in the way of a review.. there's plenty that needs looking at.

Library

Manage Library

wim leers → credited bnjmnm → .

The 3530795-validation-render branch has an approach that looks like this:

The screenshot was taken before we added support to gracefully handle empty required fields - mentally replace that message with any of the many possible render-crashing errors that can't be handled so smoothly. The design is just rough, the main things to note is the unrenderable component has a clear message in its place, and the guilty prop input is correctly highlighted as invalid.