πŸ‡ΊπŸ‡ΈUnited States @bnjmnm

Ann Arbor, MI
Account created on 3 November 2012, over 11 years ago
  • Staff Software Engineer at AcquiaΒ  …
#

Merge Requests

More

Recent comments

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

Happy to see this change!

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

Good refinement. Merged.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

The patch solution is a bit more complex than it needs to be. Looping through the results called by get() should be sufficient as it will simply not loop through anything if there are no results

      .siblings(':hidden.vertical-tabs__active-tab')[0].value =
        this.details.attr('id');

can be replaced with
.siblings(':hidden.vertical-tabs__active-tab').get().forEach((hidden) => hidden.value = this.details.attr('id');)

The issue summary currently suggests a solution that 1) would not work as jQuery does not have a .value property 2) suggests reverting back to jquery value settings, which our linter would not allow.

Update the suggestion to something that reflects what is actually happening.

As a committer / FEFM I'm fine with no tests on this as long as the scope of changes is just ensuring that value is not called on a result that might be empty.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

This isn't really a a bug that requires fixing as is only happening when there is customization taking place, and it is something that can be mitigated with a little bit of additional customization.

Create a library that depends on drupal.dialog.off_canvas then include a js file that overrides Drupal.offCanvas.bodyPadding and Drupal.offCanvas.resetPadding

something like this would work:


const oldBodyPadding = Drupal.offCanvas.bodyPadding;
Drupal.offCanvas.bodyPadding = (event) => {
  if (!Drupal.offCanvas.$mainCanvasWrapper.length) {
     return
  };
  oldBodyPadding(event)

}

If this were impossible or noticeably difficult to address then categorizing as a bug would be appropriate, but in this case it is an issue that only occurs with a specific customization and can be fixed with not that much additional customization.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

I have selected demo_umami profile because we get the ready made list of contents to check the sticky feature? Should I remove this and add content manually?

Yep, you'll need to add some dummy content, the need for nodes to fill the table isn't justification to use a whole profile. Creating what you need can be done in a handful of lines with a loop that calls $this->drupalCreateNode() enough times to fill the view.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

This is consuming quite a bit of space for a control that most users will be indifferent to.

The location "show row weights" button, while not perfect (as mentioned in #7) is probably a better option as it is less greedy while still being visible.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

I have been happy with Cypress component testing and, similar to Cypress e2e, - my students have been able to be productive in it far more quickly than other component testing options, and were even able to independently figure out features like mocking and spies. The Cypress application works for component testing like it does for e2e which makes debugging significantly easier too. We've also been pleased with cypress-react-selector which lets us make querySelector() esque queries but using component types.

I'd like to use Cypress for as much as possible due to its approachability / ease of use, but I'll also mention that most of my Cypress component testing has been for fairly simple applications so it's possible there are advanced use cases that make it an unwise option.

I think there's a huge benefit in having a larger number of contributors able to write & understand tests even if it's at the expense of some technical benefits, but I'm also willing to accept when the tradeoffs no longer justify such a choice, so if there are specific concerns regarding Cypress + component that might not justify the ease-of-use benefits please share as I'm not a line-in-the-sand drawer.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

However, in this, we are sorting the template array in ascending order by key names.
Will this order will impact any rendering for example region templates applied after block, fields, and nodes?

I think this is a reasonable concern and I was hoping the tests added would make this more evident. Sounds like you noticed the same thing and didn't need tests to see it πŸ‘. The current solution clearly fixes it for some use cases, but alphabetical order is not necessarily the order of template priority. For example, if we take this logically ordered list of suggestions from a real site:

<!-- FILE NAME SUGGESTIONS:
   * field--node--body--article.html.twig
   * field--node--body.html.twig
   * field--node--article.html.twig
   * field--body.html.twig
   x field--text-with-summary.html.twig
   * field.html.twig
-->

Sorting it alphabetically changes it to an order we don't want

Array
(
    [0] => field--body.html.twig
    [1] => field--node--article.html.twig
    [2] => field--node--body--article.html.twig
    [3] => field--node--body.html.twig
    [4] => field--text-with-summary.html.twig
    [5] => field.html.twig
)

It is probably better if the solution focuses on the logic in \Drupal\Core\Theme\Registry::completeSuggestion, and make it so the logic is consistent regardless of the array order as neither filesystem order or alphabetical sorting reflect the suggestion priority.

But since this will need tests to land anyway, those can exist before looking into how to solve this in \Drupal\Core\Theme\Registry::completeSuggestion.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

Can this get a test-only branch or patch to confirm it fails without the fix? Be sure to customize drupalci β†’ so it runs only this newly added test and not the full 30 minutes of Drupal core tests.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

See MR

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

@yash.rode if there are two files with identical names then that is what would happen. Drupal doesn't support identically named templates in a theme so that doesn't have to be tested.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

This has been merged. The /ui directory has this initial implementation of UI code which largely uses mocks. Integration with actual Drupal TK

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

This all looks fine and I think it's good to get 11x testing in ASAP as that may wind up being the version where this debuts and it would be unfortunate to have to do a bunch of catchup because this was written for a version that is out of date by the time D.O. is ready for Project Browser.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

This problem can be reproduced regardless of filesystem, and tests are important not just to ensure this does not cause problems, but to ensure future changes do not un-fix whatever is addressed here.

While it is true that the order of readdir() results returned by a single call can differ depending on the filesystem the \Drupal\Core\File\FileSystem::doScanDirectory
method calls readdir() recursively per directory. The recursive calls to

 do occur in a predictable order (parent before child) and returns an array with the subdirectory results appearing before the parent results 
 <code>return array_merge(array_merge(...$files_in_sub_dirs), $files_in_this_directory); 

If you have a template file you consistently want to appear before the others in the returned array, put it in a subdirectory of a directory including templates it should appear before.

In other words, this can be tested by adding a custom module or theme that provides templates with a directory structure where the template that should appear first is in a subdirectory of the directory with the templates it should appear after.

it may also be worth seeing if this is better to address in the logic of completeSuggestion since higher-priority theme suggestions do not necessarily correlate with alphabetical order.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

Patch testing will be deactivated in 32 days β†’ so it's best to do all of this in Gitlab.

  1. +++ b/core/core.libraries.yml
    @@ -680,6 +680,14 @@ drupal.tabledrag.ajax:
    +      - core/jquery
    +      - core/drupal
    +      - core/drupalSettings
    +      - core/once
    +      - core/drupal.displace
    

    Currently none of these dependencies are used, but core/drupal probably should be used so Drupal behaviors are used instead of DomContentLoaded

  2. +++ b/core/misc/tableheader.js
    @@ -0,0 +1,32 @@
    +              Sticky Header
    

    Not translated

  3. +++ b/core/misc/tableheader.js
    @@ -0,0 +1,32 @@
    +document.addEventListener('DOMContentLoaded', function () {
    

    Was this a GPT suggestion? GPT loves DOMContent loaded πŸ™‚. Drupal behaviors would be used for something like this

  4. +++ b/core/misc/tableheader.js
    @@ -0,0 +1,32 @@
    +  const stickyHeaderElement = document.getElementsByClassName('views-table')[0];
    

    Since you're just getting the first match document.querySelector() is cleaner

  5. +++ b/core/misc/tableheader.js
    @@ -0,0 +1,32 @@
    +    const checkbox = document.querySelector('[data-drupal-toggle-sticky-header]');
    

    The theme function might be overridden and not have this selector. While it is ultimately up to the developer to make sure these things are in place, you have the return value 2 lines above and could reference whatever is returned by stickyHeaderCheckbox

  6. +++ b/core/misc/tableheader.js
    @@ -0,0 +1,32 @@
    +    checkbox.addEventListener('change', function () {
    

    Drupal uses arrow syntax

  7. +++ b/core/misc/tableheader.js
    @@ -0,0 +1,32 @@
    +      if (checkbox.checked) {
    +        if (!stickyHeaderElement.classList.contains('sticky-header')) {
    +          stickyHeaderElement.classList.add('sticky-header');
    +        }
    +      } else {
    +         if (stickyHeaderElement.classList.contains('sticky-header')) {
    +           stickyHeaderElement.classList.remove('sticky-header');
    +         }
    +      }
    

    All this logic can be just one line by using toggle()

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

There is code there. The behavior described sounds like what might happen if the main jquery_ui module had not yet been updated to >= 1.6

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

The project update bot branch (+ updating GitlabCI to test 11x) was all that was necessary for this as we should not be updating code from jQuery UI to meet Drupal coding standards, nor is it necessary to add new linting in order to support 11x. 11x is now supported.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

Additional work was needed beyond pasting in the default DrupalCI to account for the fact that much of this module is jQueryUI code that should not be linted against Drupal's standards. The GitlabCI was adjusted to reflect that.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

By just reverting the change, it will likely reintroduce the intermittent failure. You can determine if this is happening by customizing drupalci.yml β†’ to run the test multiple times while skipping other tests, such as how it was done in issue #3350972. See this patch β†’ for how the test in question was run repeatedly to check for the intermittent failure.

  • If the test is no longer intermittently failing, it's probably worth checking with contributors that worked on #3350972 and see if they are confident that this reversion is fine
  • If the test is still intermittently failing, then the fix from #3350972 or something equivalent should be added in a test-only module so it addresses the intermittent failures without impacting real-user UI interaction.
πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

This can be reproduced without contrib, but will require a simple test module that form_alters the views bulk operations form and adds an additional item to $form['actions']. In HEAD, you can confirm the reported bug is occuring as that new item will not appear in the views bulk form. The solution is for Claro to still unset $form["actions"]["submit"] but anything else in $form["actions"] should get moved to $form['bulk_actions_container']

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

Changing the issue title to describe the problem and not a potential solution.

πŸ› | Drupal core | Error JS
πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

Transpiled JS files were removed in Drupal 10.0 and Drupal 9 is no longer supported thus this issue will not be addressed here. Fortunately, upgrading will likely address this issue.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

I lost more time than I'd care to admit on a Project Browser issue due to there not being a JS deprecation, so I'm a big +1 on having this done to spare other 10 > 11 upgraders that annoyance.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

On a drupal install running the Umami demo, I compared the time it took to convert the static buttons into a dropbutton with and without aggregation. The aggregated version was ~20% faster.

I notice the screenshot in the filed issue has an option for Automatic Entity Labels which is not part of core. That doesn't necessarily mean that Automatic Entity Labels are the cause of the issue, but it does indicate this is occurring on a site running more than just core so it's difficult to know how to reproduce the issue, and that needs to happen before any steps can be made to resolve it.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

Re #12
The current MR works only if we change table.sticky-enabled to table.position-sticky in this

Claro tableheaders are sticky even though it isn't initializing in tableheader.js, so that means that stickiness is being achieved

Claro moved away from using tableheader.js mid-2023 in favor of CSS positioning. πŸ“Œ Use position: sticky for views sticky table header Fixed . Even if this wasn't directly obvious, the fact that the tableheaders are sticky despite not being seen by tableheader.js strongly suggests the stickiness is being implemented without tableheader.js.

But before you go digging in that direction, it's probably worth confirming the issue as reported can even be reproduced in Claro. I can't reproduce it and I assume it works fine because it now uses CSS positioning.

From the looks of it there's nothing needed in Claro. Even if that was reported in the issue, it's always possible something changed in an update and the reporter might be using an earlier version of Drupal.

If you try a theme that isn't Claro, this does appear to still be an issue

While updating tableheader might fix it, perhaps using CSS positioning in the same manner as Claro could do the trick as well. I recommend seeing if there aren't already issues-in-progress for either of those before determining what to do here.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

Hi, I was trying to make the patch from #3163765-2: Add option to un-sticky table headers to benefit assistive tech users compatible with 11.x, but while trying to apply tableheader.js's changes, I noticed that TableHeader is not getting initialised. Can someone tell me how to approach this? For now I have created a new instance of TableHeader which doesn't seem right

To understand why this might not be working Do a search for sticky-enabled (the class Tableheader looks for to initialize) in the codebase and it's a pretty manageable 15 matches. One of them is this line in Claro that intentionally removes this class from tables.

public static function tablePositionSticky(array $element) {
    if (isset($element['#attributes']['class']) && is_array($element['#attributes']['class']) && in_array('sticky-enabled', $element['#attributes']['class'], TRUE)) {
      unset($element['#attributes']['class'][array_search('sticky-enabled', $element['#attributes']['class'])]);
      $element['#attributes']['class'][] = 'position-sticky';
    }
    return $element;
  }

A git blame will show this was added in the following issue: πŸ“Œ Use position: sticky for views sticky table header Fixed which updates Claro to use CSS for positioning sticky.

Another means of narrowing this down would have been to use your browser dev tools to inspect the sticky element and see what is making it sticky.

Either way, it looks like the approach will need to be adjusted to account for changes to core in the 4+ years since the last patch was added.

So for non-Claro themes, Tableheader will need to be updated to make sticky toggle-able. Claro is using a different approach to making the header sticky, and it will require a different toggle implementation based on that.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

If we ever have to discuss again lets just open a new issue instead of looking at graying tumbleweeds.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

#12 and #13 both point out that the approach of simply removing the apostrophe is not sufficient. It eliminates the JS error, but it doesn't result in the active link JS working properly as the query it builds will not match the correct link.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

When resizing quickly, the solution in #16 results in an unwanted width increase of the off canvas dialog.

#15 seems to work but it does so at the expense of removing debounce functionality which had been working fine until the fix-the-tests changes in πŸ› [random test failure] Drupal\Tests\layout_builder\FunctionalJavascript\LayoutBuilderUiTest::testReloadWithNoSections() Fixed . It is probably worth exploring options that maintain debounce and allow tests to work. It's possible the ideal solution is with changes that only impact test runs in a test-only module, allowing the debounce calls to be switched back to the way they were when they worked reliably.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

The z-indexes in Drupal are documented β†’ and considered stable. Custom Themes and modules are built knowing what these core z-indexes are, so reducing the z-index of a sticky header by 400 could be disruptive, where suddenly many elements can bleed through sticky header that were previously covered by it.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

MR has a question about why this particular solution works and evidence that it will not result in other problems. This may require a bit of research, which is a great way to get more familiar with how core works.

This will also need a test to confirm #type is getting passed to the label elements. Forms in general have many tests, to there may be an existing test that you can use this so it's only necessary to add a few lines instead of having to write an entirely new test.

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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?

πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

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.

Production build 0.69.0 2024