This is because CSS scope() is still an experimental feature in Firefox that needs to be enabled. There was a decision that we should go ahead with scope() as its addition was inevitable, but it might not be standard until later this year (2025) according to the FF team. Given that XB might have users prior to scope() working with FF out-of-the-box, we might need to come up with a workaround.
@lauriii
wim leers → credited bnjmnm → .
The Pipeline fails are due to CI issues installing Cypress dependencies, nothing specific to this MR (the 2nd to most recent had been passing, and the only change in the most recent were stylelint related).
Gonna merge this in now since @hooroomoo already reviewed.
I'm not able to reproduce what was reported in #46 with the "All props" component from the sdc_test_all_props module. This component has multiple booleans (one default true, one default false), though not as many as the one in the provided video
There are also and the e2e tests using the "All props" component that would have failed had the symptoms in #46 been occurring.
Although this does not appear reproducible with the components available within the experience builder module, we should give this a try with the component used in #46 to see what we can surface. Could @heyyo direct us to where we could find that component?
After debugging a bit with the ref-stored transforms I'm more glad it's in there and it might spare us some future headaches.
I think the back and front end scrutiny has been duly applied, and the changes are good ones, so this shall be merged.
I'd incorrectly expected this to be addressed by the de-radixification but it finally occurred to me - this is specifically for a boolean widget, not a checkbox element. The values are handled slightly differently so I wired that up and it should work fine now.
@jessebaker reviewed my parts, I reviewed his and @hooroomoo did a pass as well so this is gonna go in.
I could only reproduce the symptoms as reported once, but I approved the code as it does address the possible cancelling of a callback we didn't actually want cancelled. However, because the issue is difficult for me (and @jessebaker) to consistently reproduce, I think it would be best if @neha_bawankar (the reporter) tried this out before this gets merged. That's just to ensure we don't need to re-open this - it doesn't appear to be the sort of change that would introduce a problem.
(I found one bug that is also present in 0.x - if you have a media widget on the page data side bar, pick some images and then switch to a different sidebar and come back the selected images are no longer shown).
Good catch. I created 🐛 Page data form values-in-progress not retained for Media Library (and perhaps other) fields Active for this.
bnjmnm → created an issue.
This was filed before form elements were refactored to use native instead of Radix and I believe this is no longer an issue, and I was unable to reproduce the problem as reported.
Looks awesome in Chrome! But in Firefox (my default), the styling gets a little messed up
🤔 I checked this out in Firefox and spotted one thing in the screenshot more (the media titles were wrapping instead of elipsising...) but I'm not entirely sure that would explain how herky jerky that screenshot was... This is how it looks for me now in FF
If you're still having problems, would you be OK looking in the FF inspector and seeing if there are any styles (or lack of) that seem to be making the most nuisance?
Yes, I think we need to revisit this. I'm actually surprised we've managed to get this far with XB with this existing order of operations. It seems fundamentally incorrect. We're using AJV to validate, which means our validation is based on the json schema, which means the thing we need to validate is the prop value, but we don't have a prop value until we run the transforms. Prior to running the transforms, we don't have a prop value, we only have a form input value, and form input values are allowed to be anything so long as the transforms turn them into a valid prop value.
The transformed versions are getting validated, but they're still valid. After digging into the implementations, the format requirements are more relaxed than I'd assumed.
If it is validating entity:node/2
- ✅format: uri passes
/^(?:[a-z][a-z0-9+\-.]*:)(?:\/?\/)?[^\s]*$/i(the colon indicates a scheme and the slash indicates a path) - ✅format: uri-reference passes
/^(?:(?:[a-z][a-z0-9+\-.]*:)?\/?\/)?(?:[^\\\s#][^\s#]*)?(?:#[^\\\s]*)?$/i(uri-reference is even more forgiving)
This could be made more robust with the use of pattern: but that alone would result in the scenario mentioned by @effulgentsia where the AJV validation does fail
I'm good with this approach, I think it's more accurate to see this as a stale @todo vs functionality that needs to be changed.
wim leers → credited bnjmnm → .
wim leers → credited bnjmnm → .
Updated issue summary to reflect that is this not referring to a Dropdown list (it is not a list of select menu options) nor is the issue necessarily related to scrollability (there are several ways this can be addressed)
I'm also postponing this as it might be redundant in light of
📌
Style Autocomplete suggestions
Active
- based on what was reported, this should only require setting the autocomplete collision logic Drupal.autocomplete.options.position = {collision: 'flip'}; which I've already done in that issue's MR. I'll keep this issue postponed instead of closed in case #3529623 has trouble getting in, in which case we can do a targeted MR here with the Drupal.autocomplete.options.position = {collision: 'flip'}; that allows the suggestions to show up regardless of the input's proximity to the bottom of the viewport.
Further, I can't imagine not showing a hidden dependency, that would be a huge point of confusion.
I'm inclined to agree. This issue was filed because
- there were comments in core indicating hidden modules exclusion from "depends on" was the intended functionality, but the code was failing to achieve that.
- Giving the above finding it's own issue make it possible to unblock progress on a the more important issue where the above happened to be found
I was leaning towards keeping things as-is even when this issue was initially filed, and considering 5 additional years have since passed since I'm not sure there's anything that needs changing aside from possibly pruning the @todos
Ideally, the component should not fail to render when a required field is left blank.
I disagree, if a component has an invalid prop, it shouldn't be rendering. That said, the. component should do a better job informing the user why it isn't rendering, which we will address here:
📌 Invalid prop errors should be detailed in Component preview Active
wim leers → credited bnjmnm → .
Re #11 There were styles for this but it looks like there were some conflicts with focus-visible vs visible but this should be good now.
FB is addressed as far as I can tell so I'm merging to get _none working properly again. If there's lingering bits happy to address in a followup.
avpaderno → credited bnjmnm → .
smustgrave → credited bnjmnm → .
griffynh → credited bnjmnm → .
The two PATCH requests to component-instance is due to the endpoint set to forceRefresh: true, which was added to fix the issue of vanilla Drupal behaviors not re-running if a component form is revisited. (the most common symptom was leaving then re-loading a component with a media library widget would result in the widget JS not working)
Setting forceRefresh: true provides a redux-cached version of the form until the new version is available, then the updated data (including selectors necessary for Drupal Ajax to work) replaces it.. Apparently this setting also makes additional, unnecessary requests. In the MR I updated the criteria to force a refresh if
- The query string changed (which is very expected)
- The last query string came from a different component.
Some manual testing is needed to ensure there aren't scenarios where a re-opening of a props form doesn't force a refetch.
The mr 3526866-pretty-major-regression updated some CSS and here's a comparison of the MR to HEAD.
Alright lets see if this eases some e2e angst.
Now that I can run these tests locally (my site was not in a /web folder but with the recent change it works great), I spotted a few additional things that I imagine are quite easy to address.
Needs signoff that @hooroomoo can grant + some of the test improvements need another set of eyes as they were added after the RTBC
Added a handle-none-option MR with test that should address #15
The final form element is de-Radix'd, which should make things easier & more stable 😎
The MR in 📌 Create a test module that adds color, machine name, and dropbutton elements to the props form Active includes a test module with color component which can be cherry picked into this.
We should first address 🐛 Autosave publish process does not acknowledge pathauto deactivation Postponed before proceeding here. Once that is fixed, this issue might be completely resolved, too.
Argh - this must have been due to changes that occured during the longer-than-typical review cycle. Filed a new bug for what was found 🐛 SDC Required image can't be removed without error Active . If there's interest in keeping this open until that is fixed NP, but it will be easier for me to get it addressed with a fresh issue scoped to those specific symptoms.
bnjmnm → created an issue. See original summary → .
Clearly a problem is occurring in #7 but it isn't one I'm running into (see this video) + this and the e2e test mentioned in #5 demonstrate that "Handl[ing] media image fields on page data form" currently works.
The error in #6 looks like it's coming from OpenAPI validation, which should obviously be addressed. If anyone currently experiencing it can either update this issue summary or create a new issue targeting that specific bug
No longer relevant as this was addressed by other work. There is confirmation of this in the test 'Can open the media library widget in an xb_page props form' in media-library.cy.js (this test does more than just open, it adds / removes etc.)
Addressed elsewhere. See field_xbt_language.cy.js
Addressed in other issue
This was addressed a while back. The reported error is from Radix due to not meeting its option requirements. Tested manually to make sure this isn't a problem with metatag and it is fine.
In the past week, we also stopped using Radix for our select elements which should make it even less of a concern.
Overview
- Add text-based test cases to xb_test_state_api module.
- As part of getting those tests to pass Make the text components use native elements instead of radix
- Even if the text inputs can't look identical, it should look pretty close. More formal designs will be provided in the future so there's no need to be pixel perfect if it prevents the functionality from happening.
Proposed resolution
Manual Test
Enable xb_test_state_api module and try the inputs added in this MR
Media Library and many aspects of JS/AJAX iwork and there's considerable e2e test coverage confirming this.
It's quite possible there are use cases that have not been addressed yet, but there's definitely not a blanket omission of AJAX/JS integration. If specific problems arise, they are best addressed as targeted issues with tight scope so the specific symptom can be addressed
Thanks for all the reviews etc on this lil' dragger.
Ty @hooroomoo for the review
I have a proof of concept from last year that successfully does this, and the CK team + several committers were in support of the approach. The biggest part will likely be finalizing ✨ Add an API for importmaps Active , after which most of the CK changes are not too intense.
I also created a doc that details the approach and provides steps for several contrib modules on how to approach it
Re #10its not yet possible to get selects to wrap on long options in FF, but it looks much better now. The Chevron being there makes it more apparent it's a deliberate truncation.
When FF supports the select style pseudos, this will look even better
Thx for reviews everyone
avpaderno → credited bnjmnm → .
Took care of the merge conflict and it looks like all other reviews were good with the changes so I'll go ahead and commit.
Radix / 0.x
Native / this MR
Despite adding a bunch of additional testing the end result is fewer LOC! +341 −345
Thanks for the reviews @hooroomoo & @balintbrews
Between this and some other recent changes, these can even be unskipped. Nice work @penyaskito
The is always unchecked part is addressed by
📌
Confirm Semi-coupled form elements can work with State API visibility
Active
, the other part will likely be fixed with
🐛
Autosave publish process does not acknowledge pathauto deactivation
Postponed
as mentioned in #4
I had this postponed on the State API checkboxes issue becausei I can’t manually reproduce the problem without the State API working with that widget. Based on the above it sounds like this or something else representative of the situation can be reproduce me - could the issue summary be updated with these steps?
It's possible I duplicated this issue with 🐛 Autosave publish process does not acknowledge pathauto deactivation Postponed , but not fully sure as the IS here specifies the issue is due to pathauto creating an alias when a pattern isn't defined for the entity type, while #3526130 is specifically about the publish process not acknowledging the "Generate automatic URL alias" being unchecked.
If they are in fact the same issue reported differently please note the issue summary in #3526130 already provides a 3 line solution that works, but notes there's probably a cleaner way to implement it (perhaps in a hook that specifies module: 'pathauto'