If we're running into an infinite recursion issue, I recommend first finding where the recursion is occurring. This
We already know that there is an infinite recursion problem when #type is set in the render array.
This is most likely due to a condition that executes code only when #type is present. The two most likely ways PHP would check for the presence of #type are isset($foo['#type']) or !empty($foo['#type'])
We can use a regular expression to find places where these conditions might be set
[^\!]isset\(.*\['#type'\]\) Will look for an "isset" call without a preceding exclamation point, followed by an array path that ends #type. The regex could be even more precise, but just this narrows things down to only 9 places this could be happening.
Most of the results can be ruled out because they are in modules/themes that don't need to be running for this error to occur, they are in tests, or it belongs to a class for a feature that is unrelated to the issue.
Expand the results and you can rule out additional possibilities:
This leaves only 5 possible places, making it pretty easy to check each with an xdebug breakpoint, and see which condition is getting hit infinite times.
If I place the breakpoint in the Renderer.php condition, it appears to loop infinitely when it hits an element with a form_element_label #theme.
Without even having to know that much about the render system, just a little bit of PHP / regex / xdebug we can see the recursion is happening in this block in Renderer.php
// If the default values for this element have not been loaded yet, populate
// them.
if (isset($elements['#type']) && empty($elements['#defaults_loaded'])) {
$elements += $this->elementInfo->getInfo($elements['#type']);
}
Is this best accomplished by changing the condition, or by changing what this->elementInfo->getInfo($elements['#type']); returns, or something entirely different?
Perhaps the solution can be more targeted and the sticky recalculated on the details toggle event instead of the current approach that uses the more-often-occuring scroll event.
It's also worth pointing out that this will likely no longer be an issue once this issue lands: 📌 Make drupal.tableheader only use CSS for sticky table headers Needs review
However, that would only benefit 11.x, and it would be nice to get this bug addressed for Drupal 10 so this could still be worth doing.
There's an error occurring in the logs as well, the "Content Type: example" recipe does not have a 'name' property, so logo: $this->getRecipeLogo($path, $recipe['name']) in the plugin is hitting an error with that recipe.
We need some tests for this - among other things we need to be able to prevent changes that reintroduce "this is a module" assumptions. The issue that preceded this, 🐛 "View Commands" button assumes project is a DO module (and that a project has commands) Fixed , probably should have had tests but we chose to wait until this issue so the tests were based on an actual plugin (faster to write and more representative of IRL scenarios!)
What @smustgrave spotted in #8 should probably be addressed - there is a momentary flash of the no-javascript buttons before they are processed into the familiar Views UI. These should probably just be hidden and exposed with a no-js stylesheet. Claro is already adding one here so you can build from that.
The gif in the original issue summary looks like there are additional JavaScript errors as that gif shows clicking the "Add" button reloads the page. If JS is executing properly, that should not happen as the click event is prevented. If there is a JS error, however, the event handler that does this might not be getting added
I am a Ckeditor 5 maintainer and I reviewed this doc and the necessary changes were made so this status is no longer "Needs Review"
Add the "5" to Ckeditor in a few spots. Reformat to convey ideas with less overall text.
Left a comment in the MR as to why we need to find something different from the current solution.
The issue is due to this style that is added to sticky table headers:
.position-sticky thead {
position: sticky;
z-index: 500;
top: var(--drupal-displace-offset-top, 0);
}This high Z-index is only necessary when the header is actually "stuck", but the Z-index is applied in either state. Unfortunately there is no way to target "stuck" elements with CSS only, so this leaves us with two approaches:
- Use Javascript to determine the coordinates of the table header and conditionally provide a z-index boosting class only when stuck at the top (and accounting for offsets such as toolbar)
- Update the Z-index of the dropdown, but target something specific to autocomplete vs. the current approach of targeting
ui-frontIf an autocomplete dropdown is open, it's reasonably safe to assume it should be above other elements. An autocomplete-specific z-index boost (vsui-front) is easier to test and less at risk of causing unwanted side effects
Yay! That one little icon added a surprising amount of complexity and I'm glad to see it move to a nice farm up north.
When I filed this issue in 2020...
- Claro was an experimental theme. It has since become Drupal's admin theme
- I was not a frontend framework manager. I have since become a frontend framework manager
Given that it is 2+ years since Claro graduated from experimental status, I'm not sure the benefits of of moving the assets + references outweigh the potential disruption. Testing this manually requires time and it is a lengthy process without tests to confirm things are OK. Something unwanted could slip through, and custom modules/themes may be referencing some of these assets. Had this happened while Claro was still experimental I'd be all for the change. While it should have occurred when Claro was added to core on October 4, 2019, that doesn't necessarily mean it should happen now. I'm setting this to Closed (outdated), but I don't intend this to be a stern "final word". If anyone here believes the benefits of doing this outweigh the risks then document that and re-open.
Alternatively, get some quick issue credit and open an issue that simply removes this from the readme:
Icons in this folder are copied from Drupal core. This folder with its content
should be removed before moving Claro to Drupal coreWe are making changes in claro templates only.Do we need to do the same of default theme and then expand tests or shall we create a follow-up issue for default theme?
\Drupal\form_test\Form\FormTestDetailsForm already tests the default theme. it is simplest to add a few lines to that test to run the test with the default theme and then with Claro. Use a @dataProvider
Set up your dataProvider and put this at the beginning of the test to set the theme when $theme isn't false, and that does everything needed without having to add a new test class that is 99% the same as an existing one.
if ($theme) {
$this->config('system.theme')
->set('default', $theme)
->save();
}
Re #13 enabling the Inline Form Errors module should get this to show up I just updated the issue summary to include this in steps to reproduce.
bnjmnm → created an issue.
Did some grepping on 11.x and the only Classy references are still-relevant ones. Closing as outdated.
If steps to reproduce are provided that don't require the presence of the statistics module, this can probably be looked at, but with the information as provided there's nothing to suggest one way or the other that this is something addressable by a change to drupal Core.
The findings in #6 show that does not appear to be happening. The screenshots from the bug report indicate that a custom theme is in use, which could very well be the cause.
I am going to close this as not reproducible. If the issue creator or anyone else can reproduce with a core theme, by all means reopen.
his is because $element contains a reference to $variables['label'], and modifying $variables['label'] inside the function is triggering the function to be called again, leading to an infinite recursion.
This was spotted in #2 as well. The next step should be figuring out why adding #type is causing this recursion.
What is it about this additional array property causing this recursive loop? Until we know that, we won't know if the solution in #2 is the right approach. Knowing this might reveal a way to get #type in without the need to rename - it may also reveal an underlying bug or optimization opportunity.
You don't need to fully know the render system to know how this works. The process of investigating these types of bugs is what leads to the fuller understanding. I recommend looking for logic that checks for either being present or not empty. These regexes can help you find those uses:
- isset\(.*\['#type'\]\)
- !empty\(.*\['#type'\]\)
. There will still be several results, but you can rule out any uses found inside a module, theme or test since the recursion happens on a minimal install. Look at what the remaining uses do and put breakpoints in places you believe the recursion might be happening. There's a good chance you'll find the cause, but if you don't you can still document which regex-matching areas you checked and what your findings were - that information is still a helpful step forward.
This issue is specific to depending on libraries that were deprecated in Drupal 9, and fully removed in Drupal 10. Drupal 9 is EOL so this will not be addressed. It is unfortunate there wasn't a solution provided during Drupal 9's lifespan, but hopefully the modules impacted by this were able to mitigate the issue without too much hassle.
The media dialog appears to be making good use of the available vertical space
If you have steps on how to reproduce on a fresh install (an Umami install might be easiest since Media Library is already set up), put that in the issue summary and reopen
Btw I noticed the workaround of mentioned in the issue summary sets the dialog content to a fixed height of 700px. This means on taller screens you are unable to take advantage of the available height.
.media-library-widget-modal .ui-dialog-content {
height: 700px !important;
}You may find the vh unit more helpful as it is a percentage of the available height, not a fixed pixel value.
When this issue was filed in 2016 the <details> element was not supported by all browsers, making a polyfill necessary. Drupal's polyfill implementation was likely failing in this case.
As of 2020, all modern browsers natively support <details> and the polyfill has since been removed. It is unlikely this is still an issue, but even if it were it would no longer be something addressable within Drupal as this functionality is now fully native.
Unable to reproduce.
If the person reporting this is still experiencing the issue 6 years later by all means reopen with additional information about the environment this issue is happening in.
This was a nice idea when the issue was filed roughly two years before the release of Drupal 8. Now that Drupal 8+ has existed for over 8 years, such a change could be quite disruptive. While it's true the renaming would result in a small DX improvement, I'm not sure if at this point in Drupal's lifecycle it is enough of an improvement to justify forcing sites to rename, making existing documentation inaccurate, and potentially forcing some contrib to maintain multiple versions when they otherwise wouldn't have to.
Based on a near-decade of no activity it seems like this isn't something in high demand so I'm changing the status and it can reopen if there's a rationale that justifies the tradeoff (and there might be and I'm not considering it!).
Hi @bendqh1
Stark is not a starter theme at all, it is very intentionally the most minimal a theme can be while still being a theme.
Starter themes are created using the appropriately-named Starterkit Theme → and is effectively the replacement for Classy.
This was filed ~10 months before Drupal 8 was released, any time during those 10 months would have been a fine time to take care of this.
Drupal 8+ has now been around for 8.5 years, and removing the classes now would be incredibly disruptive and cause many regressions. The intentions here were good, but any benefits they'd have yeilded are trival compared to the problems that would be introduced. Closing so nobody winds up devoting any more time to something that I (Frontend Framework Manager) would not approve.
This was filed ~10 months before Drupal 8 was released, any time during those 10 months would have been a fine time to take care of this.
Drupal 8+ has now been around for 8.5 years, and removing the classes now would be incredibly disruptive and cause many regressions. The intentions here were good, but any benefits they'd have yeilded are trival compared to the problems that would be introduced. Closing so nobody winds up devoting any more time to something that I (Frontend Framework Manager) would not approve.
This was filed ~10 months before Drupal 8 was released, any time during those 10 months would have been a fine time to take care of this.
Drupal 8+ has now been around for 8.5 years, and removing the classes now would be incredibly disruptive and cause many regressions. The intentions here were good, but any benefits they'd have yeilded are trival compared to the problems that would be introduced. Closing so nobody winds up devoting any more time to something that I (Frontend Framework Manager) would not approve.
This needs tests
It would be helpful to get the issue summary updated to provide a scenario that doesn't use paragraphs hooks as the example so this is clearly a core issue. Even better - as part of creating the tests, make a test -only theme that reproduces the reported problem, yet works properly once the fix in #8 is present. If we have a test that fails without #8 and passes with it, I'm happy to commit.
It's probably worth checking a bit to see if the ksort is sufficient. In all cases does the specificity order match php sort order?
(and tbh even if it doesn't match 1:1, as long as it doesn't break anything, the ksort fix should happen and the edge cases figured out later).
This may have been worthwhile when this issue was filed and Drupal 8 had not yet been released, but in the 9 years since this was filed, Drupal sites have grown quite accustomed to blocks having IDs and it would be very disruptive to existing sites to make this change now - potentially millions of CSS/JS selectors would cry out in terror and suddenly be silenced.
There is also no current standard that would get mad about blocks having IDs, so lets send this one to a nice farm up north.
Although this issue was filed when Force Awakens was #1 at the box office. I confirmed this is still an issue in mid-2024. As the long lifespan of this issue suggests, it's probably not an urgent matter but it would me nice for label preprocess to know the type of element they belong to.
Comment #2 seems like the right place to get this attribute added. If there are still memory issues like the ones reported 8 years ago, perhaps some xdebugging can sniff out the culprit.
As mentioned above, this will of course need some tests. I'm guessing there is an existing test that can get a few lines added so it's not necessary to write a whole new dang test to see if #type is there.
Going through the JS it all looks fine for the Navigation module but I'm a little unsure about it becoming an API available to other extensions. Instead of dealing with the buzzkill of last minute JS change requests, we can address my concerns about this being marked beta experimental by just making the libraries internal 📌 Make libraries with JS internal, at least for beta Active
In the Stable theme, we're using space for indentation in core/themes/stable9/templates/admin/indentation.html.twig. We have different behaviors within the core themes. Can we make them similar for both? We can make changes to Claro or Stable.
This is unlikely
The Stable theme, as the name suggests, exists to provide a stable set of markup and styling that will not make any changes that could potentially result in regressions (sometimes referred to as the "stable fence"). Updating Stable to work like Claro would be counter to this.
Claro is arguably a better implementation and would not want to roll it back to a pre-2019 implementation for the sake of making the approach similar in two themes - particularly since Stable's purpose is to provide a minimal frozen-in-amber theme intended for use as a base theme, while Claro is built to be an admin theme that accommodates innovations across updates.
Simlar to my stance in #25, if there's a non-theoretical real-world use case that this might address, making the change is a conversation worth having. If the desire is to have consistent approaches across themes without that real-world use case, it would not get FEFM signoff from me.
@alexpott is correct, this is a very specialized use case. It defaults to being a blank square, and there are several hidden svgs that use tabbledrag specific functionality to become visible on mousedown to display lines that visually connect related drag items. (so very coupled to tabledrag JS)
Keeping it inline makes it easier to maintain and allows for improvements to the Tabledrag UI without having to worry about BC with other uses of the image. I don't think it is worth moving it to a separate image unless there are specific use cases to justify it. The reported use cases seem more hypothetical.
If there is something more specific that seems best addressed by un-inlining the SVG, comment or update the IS with it an reopen, but as reported this seems like something best left alone.
This is great!
Committed to 11.x
Will also commit to 10.3.x, but would first like to run tests on that combo as I've encountered a few JS patches that had unexpected backport surprises
Also need to figure out how to avoid duplicated IDs when same block is placed more than once clean_unique_id twig filter does not work well for individual AJAX calls.
On 1.x it is already appending --<NUMBER> to avoid the duplicates if > 1 of the same Navigation Block is placed.
I'm specifically looking at the ids that the MR modifies in templates/navigation-menu.html.twig, and those seem to be fine. If there are specific steps that lead to the duplicate ID issue I'm interested in knowing what they are.
It looks like the <article> use extends beyond Claro - it's in additional places such as the core module template and Stable 9. This could be disruptive to existing sites using <article> as a selector for styling/JS/tests so it really shouldn't happen in 10 outside of Starterkit. Thi change could be introduced in 11, though!
The adverse effects of this excess <article> use can be somewhat mitigated by labeling these instances using aria-label or a similar approach. That way, assistive tech would see something more descriptive than just Article
I'm going to keep the "Needs subsystem maintainer review" around as I suspect an additional review will be needed, but that's my review for this stage of the process ✔️
#4 isn't quite right. It seems like at the moment this is just a controller providing a div that a React application attaches itself to - the goal here is to use the actual theme engine this project provides.
In a module we want to
- use the JSX theme engine to render some react templates using the same approach used in
umami_jsx. The goal is to demonstrate that a module can take advantage of the JSX theme engine without the entire theme having to be built on it - In the
umami_jsximplementation, if there is a JSX version of a template, it is ALWAYS used instead of the Twig version. In the module created in this issue, we want to demonstrate that the JSX version can be used conditionally. For example, have it so the site defaults to rendering everything using twig templates, but will switch to the JSX versions on node routes. - An even more ideal scenario than the node route one above would be scoping the JSX theme engine rendering to a single block. That might be complex enough to merit a separate issue, but feel free to do here if it seems feasible. That would demonstrate that a module developer can inject JSX Theme Engine rendered applications via a block, without forcing the rest of the page to be JSX-theme-engined - this could be very useful to contrib devs.
bnjmnm → created an issue.
bnjmnm → created an issue.
I might be missing something obvious, but I'm not sure how to manually test this (so far everything has been based on just looking at the code). Can someone document some basic steps to do this? It's possible something else on my local is making this unavailable - running dev or build in web/themes/contrib/jsx/modules/jsx_devel/codeMirror results in an error, but perhaps that's not even what I'm supposed to be doing. Excited to see what has been built!
We are not able to read using fs so we have used fetch to do so. In the Stand alone react app we are able to read a file, within the react app. what should be the next step on this?
Create a Drupal controller that returns the contents of the file requested (probably have the path as a query arg?), and return it as JSON. That controller implementation doesn't have to have major access restructions for the time being, but it should only allow loading of files with the extensions related to Twig and JSX templates.
Re #5 this will ultimately need to have the ability to load and save files. For the time being, this might be easiest to accomplish by having the Codemirror integration run as a standalone Vite application so fs can be used. Eventually it would be nice to not have a separate app running on a different port - perhaps the file edits/saves can happen via server side endpoints. That could happen here but it could also happen in a followup.
The Migrate CRA to Vite has some good info, but probably shouldn't be followed too closely. Reasons include
- The Umami demo uses esbuild, not Create React App
- We're not serving anything via JS, this is strictly building. Things like running
devshould still update when files change while using non-minified code, but Drupal is already doing the serving - no "application running on port x" needed
When I bring down the current branch I see a console error with bindAjaxLinks in the trace, and bindAjaxLinks is what alters link behavior to open a modal instead of the default behavior of going to another page - addressing that error will likely address issues where clicking is failing to execute the expected AJAX task
Lets table the id approach for now. There's yet another approach that is worth trying out:
You'll first want to refactor every call to Drupal.attachBehaviors() in ajax.modified.js. If the first arg is a child of (or is) [data-off-canvas-main-canvas]</code, that arg should be <a href="https://developer.mozilla.org/en-US/docs/Web/HTML/Element/template">wrapped in a <code><template> tag. Template contents are not directly queryable, so we don't have to worry about duplicate ids or behaviors data-onceing an element that is not meant to be part of the rendered page.
Inside Drupal.behaviors.jsxAjaxProcess.attach() check to see if context is a template tag. Exit early if it isn't (you may want to keep the duplicate id prevention, but everything else can exit early.
It even may be possible to just send the <template>wrapped context without all the additional checks - so if it's a template go
Drupal.JSXAdditional(Drupal.JSXHyperscriptify(context), context);. If that results in nothing appearing you may need to send context.contentinstead.
By wrapping the code in <template>, it is shielded by DOM queries. Once it is scriptified, though, it can be sent to behaviors and AJAXed properly.
How to exactly get this approach to work might require some trial and error. It'll be easier to prgress the more you familiarize yourself with how template works on MDN and the docs at the beginning of demo/themes/umami_jsx/app/local_packages/hyperscriptify/index.js
Based on a Zoom call, it looks like implementing the changes that allow this submit button to appear also result in AJAX-enabled dialog buttons not being AJAXified. We were able to narrow it down to this:
When a form is inside a dialog, any buttons in actions are moved from the <form> to the dialog's buttonpane
If any of those buttons have Drupal-AJAX functionality, it is implemented in Drupal.behaviors.AJAX via the loadAjaxBehavior function
function loadAjaxBehavior(base) {
const elementSettings = settings.ajax[base];
if (typeof elementSettings.selector === 'undefined') {
elementSettings.selector = `#${base}`;
}
// Use jQuery selector instead of a native selector for
// backwards compatibility.
// Add timeout to provide time for scriptified elements.
setTimeout(() => {
once('drupal-ajax', $(elementSettings.selector)).forEach((el) => {
elementSettings.element = el;
elementSettings.base = base;
Drupal.ajax(elementSettings);
});
}, 50);
}
As the code above demonstrates, the element that should get AJAX functionality is found via elementSettings.selector. In the case of these buttons, elementSettings.selector is finding two matches - one is the visible element that is part of the page, and the other is inside a <drupal-fragment> as the not-visible markup that hyperscriptify() then renders into something interactable. In the case of these AJAX dialog forms (and probably elsewhere) the selector is finding the version inside <drupal-fragment> first, and applying the AJAX functionality to an element that is not part of the rendered page.
This also means that duplicate ids can exist on the page in general, existing in the hyperscriptify-informing markup as well as the final rendered page.
The best way to address this isn't immediately clear, but here are some approaches that come to mind:
- Update queries to ignore elements inside drupal-fragment and other custom elements.
- Alter the IDs of elements inside drupal-fragment and other custom elements, and change them back to the expected ids when they are hyperscriptified (this could fix the duplicate id problem too)
- Update the prevent-duplicate-ids script to prioritize changing ids inside
<drupal-fragment>etc when two instances are found and only one is for actual eyeballs. - Find a way to ensure that the contents inside
<drupal-fragment>etc are removed from the DOM before their visible-to-people hyperscriptified versions are sent throughattachBehaviors()
Left feedback on MR
Two spots I recommend looking at:
Our version of the dialog.ajax functionality
This is 98% the same as Drupal core's dialog.ajax.js, and all the changes thus far are specific to dialog + buttons functionality. Specifically, to prevent duplicate buttons.
You'll want to see how prepareDialogButtons($dialog) works, and when / where it is called in order to determine why some buttons are failing and others work. This is the method that takes buttons from the form-in-a-dialog and moves them to the dialog's buttonpane. Setting some debug points to identify what is different about preview should help you better understand what is happening and where the point of failure is.
In the added AJAX logic
Currently the nested components do not get pushed through behaviors.. I'm kind of surprised as much works as it does without this being the case. It's possible there could be problems if the buttons are in a context that doesn't have a component-element as the top parent. This is all new territory so I can't guarantee this is where to look, but it's definitely worth investigating.
Note the @todo
[...context.querySelectorAll(`${componentName}:not([data-drupal-scriptified])`)].forEach(component => {
if (!component.hasAttribute('data-drupal-scriptified')) {
const container = Drupal.JSXAdditional(Drupal.JSXHyperscriptify(component),component);
component.setAttribute('data-drupal-scriptified', true)
component.hidden = true;
// @todo: it is probably necessary to have a Drupal.attachBehaviors(container, {...settings, doNotReinvoke: true});
// here so it isn't only happening when the top-level parent of context is a JSX component.
// It isn't here currently as it was causing duplicate renders. It is possible this is no longer
// a problem due to fixes made elsewhere.
}
})
FEFM here:
While I'm not against making these kind of optimizations, something like this could result in regressions on existing sites that have styles/tests/etc that expect <article>. Sites this dependent on specific markup such as this should probably be extending a Stable theme, but I'd still like to be careful that these changes to fundamental templates provide more benefit than risk.
Based on the context in MDN page linked in the IS + reviewing some W3 specs, it seems pretty reasonable to interpret the "should" at face value as opposed to it being a "must". I also see that the default scenario is one where a header appears, and that that default scenario is likely to be true by a very high percentage. Based on the MDN recommendation alone, I'm not sure the regression risk is justified. However, I'm also open to changing my mind - my current reluctance is based only on the info currently available to me. If there's further evidence that this is objectively an accessibility bug, or that there are clear benefits that warrant the regression risk, I'm very much open to hearing that.
Re the media query comments #56
Positioning comments in each individual weird CSS is clearly outside of this task.
I think it belongs in the scope of this issue as the formatting changes implemented by this issue changes the position of these comments. I think having them on the same line or above a media query is similarly useful, having them underneath the MQ declaration makes them less so. If there are any specific ones that are particularly difficult to reposition, removing is also OK.
So for > me < to provide FEFM signoff, I want to see those media query PX comments positioned above. That said, if another FEFM is fine with it I will not protest that decision. I'm not comfortable approving as-is, but if one of my peers is that's fine by me.
There are something optimization opportunities here and there, but nothing worth delaying this excellent improvement any further. This queue could use some less intimidating followup issues anyway.
Something to try:
In the top-level component on the first render (I believe this is the DrupalContextApp component) add a componentDidMount which will be invoked as soon as that first render completes. Pop an event dispatch in there to let the world know things are rendered and awaiting further instruction
componentDidMount = () => {
window.dispatchEvent(new Event('first-render'));
}Then in an override of drupal.init.js, switch the domReady to something that responds to the event dispatched on first render
// NOT THIS 👇 , IT DOES NOT KNOW HOW TO HANDLE REACT.
// domReady(() => {
// Drupal.attachBehaviors(document, drupalSettings);
// });
// BUT THIS 👇is effectively reactReady instead of
// domReady()
window.addEventListener('first-render', () => {
Drupal.attachBehaviors(document, drupalSettings);
})I believe this (or something like this) will address what was specifically mentioned in #12 - the need to introduce a huge timeout to Drupal AJAX attach. That delay was good troubleshooting because it demonstrated that the behavior was executing too soon, and I believe the specific too-soon is that it was happening before the React render. With the approach mentioned above this should postpone that initial behavior invocation until the dom is Reactified.
Switching to NW to address the (admittedly superficial-ish) feedback on the MR. I really like how this is approached - I've witnessed several attempts over the past few years to implement something like this and this approach not only solves the problem, but it does it pretty elegantly. The change record covers the necessary info, too, so I'm removing that tag
Looks like I have a +1 from @chrisfromredfin to just remove the icon, so I feel comfortable updating the requirements to fix the reported issue with the Security Icon by removing the darn icon.
I update the issue summary to reflect this. Have at it @Vighneshh or whoever else!
Nice investigating!
This is new territory so the following suggestion might not directly be applicable to whatever the eventual solution is, but even in the worst-case it will familiarize you with concepts that will benefit your work on the JSX Theme Engine overall.
I suspect this is roughly what is occurring:
- On initial page load AND anytime the DOM is modified via ajax,
Drupal.attachBehaviorsis called, which executes everyDrupal.behaviorand the context is either the whole page (on initial load) or the part of the DOM altered by the AJAX call. - Behaviors that are triggered on initial page load happen as soon as the markup is rendered... but BEFORE the JSX is rendered.
- Some behaviors use
once()or other means of not re-running the same logic based on adding attributes to an element in the DOM. However, if these once-only operations are provided on the pre-JSX-rendered version of these elements, they may wind up rendered on the page with attributes that indicate they've received the proper JavaScripting... but they are not the same elements. - I can think of two general ways to getting around this:
- When the JSX theme engine is in use, delay the initial dom-ready call to
Drupal.attachBehaviorsand have it run once the initial JSX render occurs. Perhaps an event can be triggered when the JSX initial render happens, and THEN the initialDrupal.attachBehaviorscalls happen. This is probably the best approach as it will likely fix many other issues we're not aware of... but it's also a large change with potential side effects so the other option to consider is... - During the JSX render process, strip out any attributes that make the Javascripting happen only once, as these are new elements in the DOM that will need to be processed by behaviors.
- When the JSX theme engine is in use, delay the initial dom-ready call to
That's a whole lotta info above, so think of this more as something to guide your exploration into this issue vs something that has to fully click as soon as you read it 🙂. We're messing with some fundamental parts of Drupal so it'll be weird but hopefully interesting at the same time.
blcok, which solves this issue, but fails tests. As well as when tried with the Ajax test module, some of the functionality does not work.
example: when we open the Link 5 (form) on ajax-test/dialog page-- and then click on the Hello world button we expect a Hello world status message, but it does not work.
Sounds like it's time to work on a solution that both fixes the problem reported here and allows scenarios such as clicking Link 5 to work ajax-test/dialog. You're already more than halfway there because you know it has something to do with removing this block of code
if (childNode.hasAttribute('data-drupal-scriptified')) {
break;
}Forgot to switch status
The changes create a condition that is always false - that will need updating which may just mean removing the condition and the effect entirely, see MR comment for more.
I tried enabling JSX debug and it crashed due to portal roots not being found, which I suspect is due to several portal-root-null-(SOMETHIGN) ids. Note that there's a duplicate-id-preventing script that is appending millesecond timestamps to any portal-root-null id found after the first. They probably all start as just portal-root-null
- Lets give this a nice z-index so stuff like contextual links don't leak through
- Can this be made collapsible? This takes up more space than the vanilla version, which is overall a good thing as it's significantly nicer looking and easier to use. However, it also consumes a pretty significant amount of real estate with some screens, so it would be nice to have the option of collapsing it to a lil' bar when needed.
This is merged. Ajax Support is not FULLY supported now, but this is a major step forward, and the additional steps are best tackled in followup issues.
Core Ajax tests have been ported over, and all but 1-2 pass locally, but these tests fail far more often on GitlabCI. The tests are still present, but many have markTestSkipped() and in followup issues we can look into ways to properly wait on elements without fragile generic wait() use.
One test that was not working locally was JsxAjaxInGroupTest. That one relies on the replaceWith AJAX command, and that does not appear to be working ATM. Will be addressed in a followup..
Most, but not all, Ajax functionality is working. For example, the language selector and other Bigpipe generated elements are now properly appearing. Another notable "now works" is Layout Builder "Add Block" and "Add Section" now open the expected dialogs. The contents of these dialogs are not always complete, however - most apparent is the submit buttons aren't showing up in the off-canvas.
Split into controller and form. So we have no forms without form elements.
Nice work @omkar.podey. While this won't be fully accessible until we land ✨ Use modals in field creation and field edit flow Needs work , the changes that are best done within this issue's scope are addressed so I'll remove the tag. This will still require a broader code review but unless there are major changes the A11y is all set.
The accessibility of the grouping seems to work, but having it in a fieldset is introducing some style problems due to there being fairly opinionated fieldset styles that aren't ideal for this use case (particularly in Claro). Suggestion in MR.
There's already similar image-overlap-protection for other use cases in wide-content.pcss.css, see that here. The similar code being added here should go in there, perhaps as an additional selector to a style that is already there.
Merged. This is very nice. We can fine tune etc in other issues, this big buddy deserves to get in and inspire work in other issues.
- There was a suggestion for "When the Menu selection changes, move the focus to the Parent link select list." - this isn't something I'd approve in my role as accessibility maintainer. The expectation is focus would remain on the control being manipulated. Deviating from this expectation would likely cause more confusion than it would offer any benefits.
- The now-two select elements should be semantically grouped, you can reference the related elements section of these W3 docs.
+1 to the points brought up in #10.
Not using px prevents a11y breaks. I confirmed the current MR still allows resizing fonts via browser settings and custon stylesheets. These were the two accessibility risks that came to mind and neither appear to be a problem.
I also agree that issues such as the one in Bootstrap are the responsibility of the theme, but I highly recommend offering an MR to popular themes that resize the nav text to unusable sizes. Why require several theme maintainers to figure out a problem that several people in this issue alone would already know how to address.
Re #10
I haven't looked at this for a bit, but I recall running into multiple issues when there were DOM changes to the form. This could be mitigated by adding the progress throbber outside of the form, perhaps something like a bar at the top or bottom of the viewport. If we went that route, it may be simplest to not bother with progress messages in this issue's scope as I could see that opening a design rabbit hole that would probably be worthwhile, but not worth blocking some incremental progress.
It is very possible some of the MR feedback is for my own code from the first JSX demo that has been ported to the next demo.