🇺🇸United States @bnjmnm

Draft is pushed and in progress
I temporarily changed the styles of some sdcs so I could distinguish their background from the preview container background

This already largely works, but I'm still refining the logic that determines how to scale the iframe contents so we can see the entire SDC without necessarily having to create a same-sized preview

Good feedback, all addressed. This is going in.

Are tests feasible for this? 🤔

Yes -

• create a test-only component that uses VH units for its height
• In an e2e test first drag the component into the layout
• Check the iframe height and confirm it isn't absurdly large due to it determining height based on a seemingly infinitely tall vh-height component

bnjmnm → created an issue.

In other words: what about field types with multiple props, such as ImageItem?

That would of course need to happen, and I'll make it more explicit in the code. It will be much easier to implement when the all-props component can be added as a page component in 📌 Create a component for testing form backend + frontend integration Active (or whatever issue is blocking that one).
I'm not sure if the solution will be a more clever parsing of the Drupal selector or if we figure out server side and provide that info in an additional attribute. I feel like what is here is worth adding now and addressing the different prop shapes in a separate issue - but if thats concerning it's NBD to do it all here and postpone this on #3463583.

bnjmnm → created an issue.

Looks good - this is in!

I'm getting an error on any props of type: integer

InvalidArgumentException: Field test-integer is unknown. in Drupal\Core\Entity\ContentEntityBase->getTranslatedField() (line 616 of  
                                         /Users/ben.mullins/Sites/drupal/core/lib/Drupal/Core/Entity/ContentEntityBase.php).      

Which happens after calling getWidget() in formTemporaryRemoveThisExclamationExclamationExclamation

Addressed "" is not a valid backing value and digging into additional errors that surfaced once that was addressed.

If this winds up providing all the necessary fields, then there should be at least an install hook added to sdc_test_all_propsso it is available as a Page Builder component as soon as the module is installed.

I attempted to add the "All Props" SDC as a Page Builder component and got
ValueError: "" is not a valid backing value for enum Drupal\experience_builder\SdcPropJsonSchemaType in Drupal\experience_builder\SdcPropJsonSchemaType::from()

If the error 👆 is addressed then I can review how this renders in the edit form and recommend if anything additional is needed.

(I also noticed sdc_test_all_props.info.ymlhas a dependency on sdc:sdc which shouldn't be needed now that it is in core)

Re #21

• Turns out it was old stuff that was still there but we didn't see due to it being black text/black BG.
• I pushed a fix for this.

• The contextual form seems to be loading an older version of the form - I'm guessing the MR moved some stuff around which would up preserving some code that has since been refactored
• Clicking a component to make it active/selected takes us to /xb/component/ which is great, but refreshing a page on that URL results in a 404. While I'm very OK with incremental progress, refreshing the UI page is something I do frequently while developing for it and I'd like it to at least redirect back to /ui

bnjmnm → created an issue.

Looks like this

It does look slightly different because the hero component intentionally had vh styling to confirm a specific fix for vh-in-iframe worked.

But! That fix only works on inline styles, which are no longer being used. An issue has been opened for that 🐛 VH units fix only works for inline styles Active
and in the meantime I prefer the less huge hero elements - easier to manually test things.

....

Also added an assertion in xb-general that confirms the SDC css loads in the iframe.

bnjmnm → created an issue.

The automated tests prompted me to run a manual A11y check on on the page

There is an AA contrast violation on
div:nth-child(1) > ._previewContainer_14ww3_31 > ._xbComponentToolbar_lmyju_17.rt-Box[data-state="closed"] > ._nameTag_1uhn3_1._selected_1uhn3_11

on

<div class="_nameTag_1uhn3_1 _selected_1uhn3_11">

(obviously these are hashed classnames but it is safe to assume they are part of the NameTag component introduced in this issue.

Your passion for coding standards in contrib modules is commendable, but we're fine with it as is. Thanks.

Wim Leers → credited bnjmnm → .

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

So much better

The MR I pushed has this working - maybe there's a slicker way but this is what I figured out. It will need tests either way but I'm needed elsewhere for a bit so leaving this unassigned in case someone wants to add those.

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

Neither point in #17 has been addressed.

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

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

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

I think it just needs @Wim Leers signoff on the MR and we're good.

Maybe we should include it as a warning and accept that we may have to allow the 1 line "skip this rule" comment when we need to use it?

Yep, if it is needed then that instance of it can be skipped and should be accompanied by a specifics-laden (as in something more substantial than "it the only way":upside_down_face: ) comment explaining why.

But does that mean you are +1 to the other 3 rules @jessebaker proposed?

Good to officially state! Yes I'm good with the other rules too.

no-unnecessary-waiting is fine, Cypress' retry-ability is built into every query+assertion which pretty much eliminates the need to explicitly add waits at all, even for elements - it's essentially built into the framework already. If the default retry-ability time is not sufficient for a specific query, it can be adjusted on a per-query basis.

Although we're waiting to take care of a few e2e things in gitlab, component/unit tests are working fine and these should get at least surface level testing

I approved - still needs approval from a data layer person.

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

Wim Leers → credited bnjmnm → .

For me at least, pasting the snippet in #7 takes me all the way to Cypress tests passing on a DDEV site. If ddev auth ssh hasn't been run recently that might be necessary to do the apt-get

It would be helpful for me to know where specifically that snippet fails on your machine, or if it fails at all.

bnjmnm → created an issue.

The alignment and spacing issues reported are not really bugs/problems - it is a vertical tab rendering exactly by design but it looks odd because there is only one tab.

I see the benefit in creating alternate styling for vertical tabs if only one tab is present, but the solution here is not going to work because

• It messes up the styling for instances when more than one tab is present
• Even in this single-tag situation reported in the issue summar, it only "fixes" it for the contents of that specific tab with the "create new revision" checkbox. Tabs can contain anything so setting it to the height of what happens to be in there on your site is not a solution that would benefit any use of tabs outside of those with content that fills the exact same amount of pixels.

Re #60

The question here is am I missing something is steps to reproduce?

Yes. The file where the error is happening needs to be loaded in order to reproduce the problem.

The error is occurring in form.js, but the form loaded by FormTestUrlForm does not include that file.

See core.libraries.yml for the library that includes this file and the libraries that depend on it.

The Repo linked in #10 looks like (but perhaps I'm wrong) something that sets up an entire instance, but the goal here is ti make an addon (probably based on the template) that can be applied to an existing ddev instance - basically the Cypress equivalent of the Nightwatch DDEV addon mentioned in the issue summary.

Unless the Github search isn't working right I'm also not seeing the Cypress system dependencies being installed, which are needed for Cypress to run at all. The Cypress documentation page on system dependencies mentioned in the issue summary provides more details, and the following line is how the dependencies are added in the GitlabCI runs:

apt-get -y update; ddev exec sudo apt-get -y install libgtk2.0-0 libgtk-3-0 libgbm-dev libnotify-dev libnss3 libxss1 libasound2 libxtst6 xauth xvfb

bnjmnm → created an issue.

MR I'm pushing up is built on top of 📌 Remove dependency on sdc_test Needs review as the changes there make it unnecessary to manually add components.

Left a comment on the MR about a change I had to make that isn't in-in-in scope but might work well here assuming it isn't something in the PHP that caused the issue. LMK

bnjmnm → created an issue.

I'm getting the same dang thing as #26 if I run npm run build

error TS6305: Output file '/Users/austin.powers/core/modules/contrib/experience_builder/ui/vite.config.d.ts' has not been built from source file

It will, however, work with the npm run drupaldev I added as part of this MR.

The current MR seems like quite a bit of height-calculating logic with several new mutable variables in play. I wonder if this can be done simpler.

The toolbar moves because of the margin reset in toolbar.js

if (toolbarBar) {
            toolbarBar.style.marginTop = '0';

// etc...

Perhaps a solution that doesn't apply the reset when a top off-canvas exists would take care of it?

if (toolbarBar) {
           if (<NO TOP OFF-CANVAS>) {
              toolbarBar.style.marginTop = '0';

}
// etc...

This would also be reasonably easy to add as a test inside \Drupal\Tests\system\FunctionalJavascript\OffCanvasTest, and the toolbar doesn't even have to be present - just an element with the id toolbar-bar that you can check the margin-top of.

Re #45
The way PHP renders the links and their attributes is fine. This was correctly reported as a JavaScript issue and that's where the solution will be.

The specific issue is JavaScript is not adequately finding those links when they use special characters. If the JS query string escapes special characters in the same way PHP does, then it will get a match and work properly.

Use the steps in #44 to make a link that will reproduce the problem as reported, and the solution is modifying the JS. In fact The solution will likely be modifying the same line as the prior patches, but instead of just removing special characters, it converts it to the encoding that allows it to match the expected link and add the "is active" class.

Testing / reproducing is fortunately pretty simple. The callback where the problem is occuring is triggered by, among other things, clicking on an anchor link inside a form. Get this link into a form <a href="#edit-contact/broken">This is an anchor link with a broken fragment</a> then click it, and the error will appear in the console.

And hopefully the issue summary can get updated soon as it is presents the issue as being specific to a file that has not been in Drupal for several years.

Can we use regex and string function like replace to get it more consitent with the PHP's trim function.

That should work. Since FunctionalJavascript Tests have access to both PHP and JS (via evaluateScript) you could quickly iterate through all sorts of potential strings and confirm the results are identical, and if there are use cases that won't pass the test provides a clean way for a l33t regexer to hop in and potentially solve it fully.

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

The following steps is from a new DDEV site to Cypress + DDDEV working at the same level Nightwatch Currently does (with no access to the test runner app)

mkdir xb-ddev-project && cd xb-ddev-project
ddev config --project-type=drupal --php-version=8.3 --docroot=web
ddev start
ddev composer create drupal/recommended-project:^11.x-dev
ddev config --update
ddev restart
ddev get ddev/ddev-selenium-standalone-chrome
ddev composer require drush/drush
git clone git@git.drupal.org:project/experience_builder.git web/modules/contrib/experience_builder
echo '$settings["extension_discovery_scan_tests"] = TRUE;' >> web/sites/default/settings.ddev.php
echo '$settings["extension_discovery_scan_tests"] = TRUE;' >> web/sites/default/settings.php
ddev restart
ddev npm --prefix /var/www/html/web/modules/contrib/experience_builder/ui install
ddev npm --prefix /var/www/html/web/modules/contrib/experience_builder/ui run build
cat > web/modules/contrib/experience_builder/ui/.env << EOF
BASE_URL='http://web'
DRUPAL_TEST_DB_URL=sqlite://web/sites/default/files/db.sqlite
VITE_SERVER_ORIGIN='http://localhost:5173'
EOF
ddev drush site:install --account-name=admin --account-pass=admin -y
ddev drush en -y experience_builder
ddev drush uli
ddev launch
ddev exec sudo apt-get -y update; ddev exec sudo apt-get -y install libgtk2.0-0 libgtk-3-0 libgbm-dev libnotify-dev libnss3 libxss1 libasound2 libxtst6 xauth xvfb


# Run tests using ddev exec... for now
ddev exec -d /var/www/html/web/modules/contrib/experience_builder/ui node_modules/.bin/cypress run --browser electron

This also makes running other core tests possible, too.

Things that still need to be done:

• This should ultimately be in an addon so it doesn't go away on poweroff, but this is a good step in the right direction
• We'd want to create comands so we don't have to use a long ddev exec
• We should get it so the Electron based test runner is available
• It would be great to get not-headless Chrome installed on the image so we don't have to configure the tests for the electron browser (or we could change the default test browser to electron)

There were some pipeline failures that I was looking into only to re-run the tests and they were fine. In each case it was because the project directory could not be found, something that would not be caused by changes in either of the merge requests.

While I'm partial to the @finnsky MR #78 because it only changes the JS, the npm run format command still runs on non-js files. This means someone wanting to use format will have files changed that they haven't even edited. We should either include all file types or limit the types changed by format

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

The navigation sidebar typically has icons:

it seems like the issue reporter and everyone working on this issue are getting a version of the sidebar that is providing the alt text instead of the icons. This is not something I'm able to reproduce, but considering multiple contributors are seeing the exact same thing, I have to assume there are fairly straightforward steps beyond those in the issue summary to and this should be documented. I'm also not sure how to get it where layout builder can be used on a navigation block as seen in the screenshots, so that would be good to include in the issue summary as well.

If the icons aren't showing up, there are already problems happening and making this look nicer isn't bad but probably not a major priority. We should ensure nothing in this MR conflicts with the correctly-loading Navigation bar.

See MR

The DDEV + Cypress addon mentioned in #2 looks like a good thing to reference, but I'm not sure it can be used as-is. It does not appearlike it is adding Cypress dependencies to the image Drupal runs on, but instead is running Cypress from a dedicated image. Because of the setup scripts (same PHP scripts as what Nightwatch uses) I'm pretty sure /em> Cypress needs to be running on the same "machine" as Drupal's PHP but happy to find out otherwise

Looks good, merged!

Left some comments on the MR that are largely refactoring suggestions that I spotted while reviewing. In the early stages I think that type of feedback would just slow things down, but since this is part of an effort to make the UI codebase more approachable it's probably time to start doing a bit more of that.

bnjmnm → created an issue.

Should the other toArray() changes in the MR be updated?

Yes. They should all be refactored, not just the instance specifically called out in the MR.

In the JS it looks like a good amount of additional logic is going into supporting aria-controls, but given that only the JAWS screenreader supports it, and a trusted A11y expert has deemed aria-controls to be poop. Since aria-controls support is so limited it is not likely causing any AT problems, but it may not justify the additional complexity.

I'm not sure a theme setting would work well here for a few reasons:

• This doesn't work as well as a site-level setting. It's something that should be available on a per-user level
• An AT-using user that wants to disable sticky should not need to have permissions to change theme settings
• There's a reduced likelihood of a user knowing such an option even exists as it is no longer in the same location as the UI element it impacts

I mentioned the hide/show weights toggle in #28 as a table-specific A11y setting that has existed in Drupal core for some time without much objection. Were the positioning of the sticky toggle more similar to hide/show weights I suspect the awkwardness would be mitigated.

Wim Leers → credited bnjmnm → .

+1 from me as well. Maybe this will change a bit over time but this is the foundation to do it from.

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

I tried reproducing this but i was not able to do that on 11.x .

Can you detail a bit more about how you attempted to reproduce the issue? The above is unfortunately not enough information to help move the issue forward.

If something can't be reproduced it can be one of two things

1. The reported problem is no longer an issue (or was something specific to their site)
2. The attempt to reproduce was missing a step necessary to reproduce the issue

Currently there's no way of knowing if the inability to reproduce was due to reason 1 or reason 2.

cypress-component-and-unit-tests-should-run branch is here to address the test setup

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

Updated IS with details of how the form is being generated.

There's usage in states.js that isn't being caught by eslint but should also be changed. Perhaps in other files as well - can't hurt to grep a bit to find out.

Happy to see this change!

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

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

Nice! Merged

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

Glad to see this spotted and addressed and working up from 1 seems like a good approach.

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

Good refinement. Merged.

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

pameeela → credited bnjmnm → .

pameeela → credited bnjmnm → .