Z-index issue with table with sticky header

#2 Please review the patch

Thanks for the work on this.

In my opinion, the sticky table header needs to be placed below the Olivero fixed menu (when it is present). There is a Drupal.displace API that may be able to help with this: https://www.drupal.org/node/1956804

Added patch following recommendations from @mherchel. Also attached the screencast for the new behavior.

Applied patch #5 and its working fine.The overlapping of sticky table header is gone after applying this patch.

Screenshot after patch:

RTBC +1

Verified and tested patch #5.
Patch applied successfully and looks good to me.

Testing Steps:
# Goto: Appearance -> Apply Olivero Theme
# Check the table format on /admin/content
# See the Z-index issue

Expected Results:
# After applying patch #5, The overlapping of the sticky table header is gone.

Please refer attached video for the same.
Looks good to me.
Can be a move to RTBC.

The patch needs a reroll plus I think we can add an automated test for this and use \Drupal\FunctionalJavascriptTests\JSWebAssert::assertVisibleInViewport() to make sure that sticky headers are working in Olivero.

Re-roll of the patch against 9.3.x & keep this issue in Needs Work for tests.

Tested patch #15 with Drupal 10.1.x and it broke the site layout.
Attaching gif for after applying patch #15.

Please refer to attached gif for the same.
Changing status to needs work.

Applied patch #15 but after applying layout was borken.
For ref attached two recording.

Applied Patch cleanly. But table header sticky is not working.

@Vinayak.Ambig Thanks for the review, and welcome to the community! While the video would normally be helpful as a patch review, there are already two comments reviewing patch #15 asserting that the site layout is broken. Please take a look at the Drupal Issue Etiquette.

Since this patch has already been reviewed, we want this issue to remain as "needs work".

inset-block-start causes the issue, I have posted a patch which fixes the issue but I will try to come up with a better solution for this issue

A few issues with #20:

custom commands failure - be sure to run yarn lint:core-js-passing to make sure the JS file will pass the linting tests.

Let's avoid overwriting the entire style attribute by using the element's style property:

      if (pinnedState === true) {
        siteHeaderFixable.setAttribute('data-offset-top', '');
        siteHeaderFixable.setAttribute('style','inset-block-start: auto;');
      }
      else {
        siteHeaderFixable.removeAttribute('data-offset-top');
      }

siteHeaderFixable.style.insetBlockStart = 'auto';

Instead of attaching the sticky tableheader JS to the Olivero global library, we should do a library-override. See the olivero.info.yml and olivero.libraries.yml files and look to the drupal.message library to see an example of this.

Tried to fix custom commands failure of #20 patch

Tested #22 and it doesn't fix the suggestions made in #21, changing the status to needs work again.

@Andy-blum thanks for the suggestions, I've changed the style attribute usage as per your suggestion. Im not sure why we need library-override because we are not extending / overriding any external library and the js/navigation-utils.js: {} that does the toogle function is a part of the global styling. Let me know your thoughts

@ameymudras

Im not sure why we need library-override because we are not extending / overriding any external library and the js/navigation-utils.js: {}

We are not currently doing this, but we have a hidden dependency there. For example in the code below:

  function adjustStickyTableHeader() {
    Drupal.TableHeader.tables.forEach(function (table) {
      table.recalculateSticky();
    });
  }

The Drupal.TableHeader object is coming from core/misc/tableheader.js, which is only present when the core/drupal.tableheader library is attached. Olivero does not declare any dependencies on this library, so there's the possibility that Olivero attaches javascript that will break when core JS has not been also been attached.

We do a similar thing with the core/drupal.message library, which is why I pointed to it as an example.

In olivero.info.yml we have

libraries-extend:
  core/drupal.message:
    - olivero/drupal.message

This tells Drupal that when Olivero is the theme, and the core/drupal.message library is attached, we need to also attach the olivero/drupal.message library.

Then, in olivero.libraries.yml we have

drupal.message:
  version: VERSION
  js:
    js/message.theme.js: {}
  dependencies:
    - olivero/messages

Saying that when we attach olivero.drupal.message, we need to send some additional JS as well as pull in the olivero/messages library.

For this issue, we need to add a library-extend entry for core/drupal.tableheader, and then a new library that serves the files Olivero needs to make this fix work.

Moving to NW for #25 points.

Also was tagged for tests which still appear to be needed.

This came up as a daily BSI target.

Think this definitely needs an issue summary to show what's being solved.

#3439580: Make drupal.tableheader only use CSS for sticky table headers changed the core implementation here quite a lot, so this probably needs to be manually tested again too.

I also opened #3515093: Olivero table.css should be in its own library and #attached to tables which is sort of related.

The Olivero theme was approved for removal in #3590816: [policy, no patch] Deprecate Olivero and move to contrib.

This is Postponed. The status is set according to two policies. The Remove a core extension and move it to a contributed project and the Extensions approved for removal policies.

The deprecation work is in #3595082: [meta] Tasks to deprecate the Olivero theme and the removal work in #3595085: [meta] Tasks to remove the Olivero theme.