Problem/motivation

Currently there is a overflow/scrollbar for the entire page, if the page is viewed on a mobile device and the page contains a table. Even with the responsive tables feature.

A CSS-only approach will not work. When scrolling horizontally, sticky headers will not move with the rest of the table. This happens:

Proposal

Instead of hiding columns, display all columns and introduce a mobile-friendly horizontal scroll on large tables.

Currently discussing if we should let claro override the responsive table feature with this new behavior, or introduce a dedicated #scrollable property to all tables to trigger this behavior when necessary. So one could have a responsive table that scrolls if too large.

CommentFileSizeAuthor
#159 Screen Shot 2022-02-28 at 8.53.36 AM.png80.27 KBbnjmnm
#157 after.gif366.7 KBmherchel
#157 before.gif139.07 KBmherchel
#147 3068696--after--patch--pic.png36.55 KBvikashsoni
#147 3068696--before--patch.png36.55 KBvikashsoni
#143 ff-mobile-table.gif2.72 MBmherchel
#138 After-patch-table.png21.34 KBMadhu kumar
#133 Screenshot 2021-05-17 at 1.38.05 PM.png161.05 KBpragati_kanade
#132 Screenshot 2021-05-17 at 1.39.08 PM.png153.08 KBpragati_kanade
#129 3068696-after_2.png29.44 KBAbhijith S
#129 3068696-after_1.png28.96 KBAbhijith S
#129 3068696-before_2.png28.94 KBAbhijith S
#129 3068696-before_1.png25.56 KBAbhijith S
#127 3068696-127.patch1015 bytesdjsagar
#127 interdiff_125-127.txt1.53 KBdjsagar
#125 3068696_after_125.png85 KBhinal05
#125 3068696_before_125.png93.66 KBhinal05
#125 3068696_125.patch794 byteshinal05
#122 Screenshot 2021-01-14 at 4.43.38 PM.png55.92 KBlokeshsahu
#121 interdiff_112-121.txt826 bytesravi.shankar
#121 3068696-121.patch44.9 KBravi.shankar
#112 3068696-112.patch45.06 KBbnjmnm
#112 interdiff_109-112.txt9.56 KBbnjmnm
#109 3068696-109.patch41.5 KBbnjmnm
#109 interdiff_106-109.txt5.05 KBbnjmnm
#108 Screen Shot 2020-07-02 at 16.26.57.png59.06 KBlauriii
#106 interdiff-102-106.txt7.68 KBnod_
#106 core-tablescroll-3068696-106.patch40.18 KBnod_
#102 interdiff_100-102.txt947 bytesbnjmnm
#102 3068696-102.patch39.89 KBbnjmnm
#100 3068696-100.patch39.9 KBbnjmnm
#100 interdiff_98-100.txt4.29 KBbnjmnm
#98 interdiff__95-98.txt20.13 KBbnjmnm
#98 3068696--98.patch40.31 KBbnjmnm
#95 interdiff_92-95.txt38.5 KBbnjmnm
#95 3068696-95.patch28.4 KBbnjmnm
#92 core-tablescroll-3068696-91.patch19.02 KBnod_
#91 interdiff-85-91.patch9.26 KBnod_
#87 filters headers twice.png38.85 KBpriyanka.sahni
#87 filters headers twice.mp48.68 MBpriyanka.sahni
#87 Menu Toolbar.png97.12 KBpriyanka.sahni
#87 Menu toolbar.mp410.48 MBpriyanka.sahni
#85 3068696-TRYING-SCROLLABLE-PROPERTY-85.patch18.74 KBbnjmnm
#80 ezgif.com-crop (3).gif574.71 KBKondratievaS
#79 interdiff_76-78.txt1.43 KBbnjmnm
#79 068696-78.patch16.87 KBbnjmnm
#77 selected_8722.png80.96 KBKondratievaS
#76 seven-toolbar-scroll-1.png199.1 KBbnjmnm
#76 seven-toolbar-scroll-2.png155.32 KBbnjmnm
#76 interdiff_71-76.txt5.59 KBbnjmnm
#76 3068696-76.patch16.82 KBbnjmnm
#75 Screen Recording 2020-05-08 at 20.58.41.gif6.45 MBlauriii
#72 Screen_Recording_20200505-170236.mp424.2 MBKondratievaS
#71 shadows.png196.41 KBbnjmnm
#71 interdiff_66-71.txt4.85 KBbnjmnm
#71 3068696-71.patch14.9 KBbnjmnm
#66 interdiff__61-66.txt6.85 KBbnjmnm
#66 3068696--66.patch14.19 KBbnjmnm
#64 Screenshot 2020-04-27 at 7.01.33 PM.png17.1 KBKapilV
#64 Screenshot 2020-04-27 at 6.59.46 PM.png30.12 KBKapilV
#63 Screenshot 2020-04-27 at 6.37.43 PM.png30.53 KBKapilV
#61 3068696-61.patch13.47 KBbnjmnm
#61 interdiff_59-61.txt2.87 KBbnjmnm
#60 Screen Shot 2020-04-24 at 15.13.33.png36.14 KBlauriii
#59 whole-buncha-extra-whitespace.png37.3 KBbnjmnm
#59 lilbit-extra-whitespace.png16.61 KBbnjmnm
#59 interdiff_56-59.txt6.67 KBbnjmnm
#59 3068696-59.patch13.67 KBbnjmnm
#56 interdiff_53-55.patch20.37 KBbnjmnm
#56 3068696-55.patch11.15 KBbnjmnm
#54 Screen Shot 2020-04-16 at 12.01.34.png7.79 KBlauriii
#54 Screen Shot 2020-04-16 at 12.01.28.png9.34 KBlauriii
#53 3068696-53.patch11.6 KBbnjmnm
#53 interdiff_42-53.txt11.94 KBbnjmnm
#52 3068696-52.patch9.7 KBbnjmnm
#52 interdiff_42-52.txt10.05 KBbnjmnm
#46 table-responsive.gif931.74 KBckrina
#45 selected_7840.png30.49 KBKondratievaS
#44 tables-overflow-after-patch.gif1.93 MBSuesdesign
#44 tables-overflow-before-patch.gif2.77 MBSuesdesign
#42 interdiff_24-42.txt9.21 KBamaisano
#42 interdiff_36-42.txt7.01 KBamaisano
#42 interdiff_38-42.txt2.02 KBamaisano
#42 responsive_table-3068696-42.patch7.15 KBamaisano
#38 responsive_table-3068696-33.diff6.09 KBthomas.frobieter
#36 Tables-overflow-3068696-35.patch434 bytesjunaidmasoodi
#35 Screen Recording 2020-01-10 at 1.10.19 AM.mov21.36 MBjunaidmasoodi
#30 sticky_table_test.mov788.56 KBJohn Cook
#24 responsive_table-3068696-24.patch5.76 KBPeter Majmesku
#13 responsive_table-3068696-13.patch1.21 KBkunalkapoor
#8 table-overflow-2.patch580 bytesMithun S
#7 table-overflow-3068696-1.patch578 bytesMithun S
#3 table.gif7.39 MBlauriii
Screen Shot 2019-07-18 at 20.43.19.png27.8 KBlauriii

Issue fork drupal-3068696

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

lauriii created an issue. See original summary.

ckrina’s picture

Would it be possible to have some clearer screenshots, video or URLs to help designers find what needs to be designed?

Edit: discussed at today's meeting that we need to do some research for possible responsive solutions, like from exiting libraries.

lauriii’s picture

Issue summary: View changes
FileSize
7.39 MB

I replaced the screenshot with a gif ✌️

ckrina’s picture

Issue tags: +stable blocker
Related issues: +#3021388: Table style update

Adding stable blocker tag. Also animated gifs ++

anevins’s picture

I recommend that we look into accessible-only solutions to this problem, as making a table responsive (apart from simply adding a horizontal scrollbar) can disrupt the semantics of the table to people who need it the most.

My preferred: http://adrianroselli.com/2017/11/a-responsive-accessible-table.html
Also @saschaeggi mentioned: https://ux.shopify.com/lessons-from-building-mobile-friendly-accessible-...

fhaeberle’s picture

@anevins How does your preferred solution works with sorting top-down/down-top?

I would go for the horizontal scroll version with a scroll helper (accessibility) at the top&bottom of the table like @ckrina showed us in one of the screenshots in Figma I think. Going for this solution, no JS is required. https://www.figma.com/file/OqWgzAluHtsOd5uwm1lubFeH/Drupal-Design-system...

This would also be a good option for not cutting out the columns of the author and the updated date on mobile screens. This cutting out should be the last option to consider in my opinion but it's an inherited behavior of the "Seven" theme but could be also changed there in my opinion.

Mithun S’s picture

Component: Needs design » Code
Status: Active » Needs review
FileSize
578 bytes

Even I prefer having the scrollbar for the table in the mobile view so the layout doesn't break and provides a good design. I have provided a patch for the same please review the patch.

Mithun S’s picture

FileSize
580 bytes

Please consider the new patch applied.

fhaeberle’s picture

Status: Needs review » Active

@mithun-s Thanks for working on this but in my opinion, before we can do code, we have to clarify in which places we want to use this behavior and talk about the behavior in general and also considering those mentioned by the other comments. After, we have to get design approval/update for the decision. As I see your solution is practical and easy, it's maybe too general to set all table's with overflow-x: scroll. You're welcome in the Drupal Slack #admin-ui and #admin-ui-design for discussing that or post comments here.

junaidmasoodi’s picture

Assigned: Unassigned » junaidmasoodi
junaidmasoodi’s picture

I am looking into this over this weekend

fhaeberle’s picture

Actually, I would consider that as a topic for the next weekly meeting in the slack #admin-ui channel to come to an conclusion/solution what fits best here, as I mentioned some concerns about the current patch in #6 and #9.

kunalkapoor’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
1.21 KB

I have added a patch it will make the table responsive please try this and let me know if it works

huzooka’s picture

Status: Needs review » Active

Wouldn't it be better to fix this in the core?
This is not specific to Claro.
The only thing that made this more visible is that Claro uses 100% as the default font size and that is much more bigger than in Seven (or in Bartik).

Re #13:
@kunalkapoor, please don't change issue status if it is assigned to someone else.

Back to Active.

huzooka’s picture

Priority: Major » Normal
lauriii’s picture

Category: Bug report » Feature request
Priority: Normal » Major

I discussed this with @huzooka and we agreed that this is a major feature request (not a bug). We have to make a decision whether this should be fixed in Claro or in core. My recommendation is to fix this in Claro.

fhaeberle’s picture

+1 fixing it in Claro

fhaeberle’s picture

Issue tags: +Novice, +DrupalCon Amsterdam 2019
fhaeberle’s picture

Assigned: junaidmasoodi » Unassigned
huzooka’s picture

Project: Claro » Drupal core
Version: 8.x-1.x-dev » 8.9.x-dev
Component: Code » Claro theme
Peter Majmesku’s picture

I am also for fixing the tables in Claro. Shall we make it like in https://ux.shopify.com/lessons-from-building-mobile-friendly-accessible-... ?

@kunalkapoor: Regarding #13: The patch cannot be applied. Also it's just wrong to touch the css file directly. You must touch the post css files (*.pcss.css). Afterwards you must compile your assets via Yarn/NPM.

Peter Majmesku’s picture

The option from #6 looks like broken to me and should not be the demand for a core theme.

fhaeberle’s picture

Status: Active » Needs work
Issue tags: +Needs reroll

At the time of the patch @kunalkapoor submitted it, we worked on the .css files directly because we hadn't x.pcss.css files, they were created later in the process of shipping Claro into Core. @kunalkapoor patch can't be applied because it likely needs an reroll/rewrite.

@peter-majmesku Do you have an updated patch with your proposed solution?

Peter Majmesku’s picture

Status: Needs work » Needs review
FileSize
5.76 KB

I had applied the patch from #13 and could not make it working:

+++ b/css/src/components/tables.css
@@ -206,3 +206,51 @@ td.priority-medium {
+  /* Force table to not be like tables anymore */

Could not apply the code successfully be inserting it directly into the tables.pcss.css file and re-compiling the assets.

It tend out, that the article from https://ux.shopify.com/lessons-from-building-mobile-friendly-accessible-... had no code example and it was unclear how to solve that in a reasonable amount of time. Also I am not sure which approach is acceptable here, so I won't waste time. 🤷🏼‍♂️

The first approach of that patch here is to make the table scrollable on mobile devices. Please share your thoughts.

Only local images are allowed.

elly175’s picture

This issue needs a summary, please could you clarify what needs updating/reviewing?

rachel_norfolk’s picture

Issue tags: -DrupalCon Amsterdam 2019 +Amsterdam2019

retagging

Peter Majmesku’s picture

The issue summary is completed. Please review.

Peter Majmesku’s picture

Issue summary: View changes
Peter Majmesku’s picture

John Cook’s picture

Status: Needs review » Needs work
FileSize
788.56 KB

I've manually tested this.

The table scrolls horizontally separately from the rest of the page as shown by the gif in comment #24 and uses the solution suggested in the summary.

I've noticed that the sticky header does not scroll with the rest of the table. This will need to be addressed as well.
See https://www.drupal.org/files/issues/2019-11-01/sticky_table_test.mov

Setting to needs work for the sticky header.

junaidmasoodi’s picture

Assigned: Unassigned » junaidmasoodi
fhaeberle’s picture

@junaidmasoodi Are you working on this?

thomas.frobieter’s picture

What i found really helpful on this responsive tables technique is a visual indication for the user so they are able to see in what direction can be scrolled. We use this technique: http://lea.verou.me/2012/04/background-attachment-local/ to show an inner shadow on the scrollable side:

The only con here is: The table and its cells cant have a background color.

Our code (SCSS):

.table-scroll{
  max-width: 100%;
  background: linear-gradient(to right, white 30%, rgba(255, 255, 255, 0)), linear-gradient(to right, rgba(255, 255, 255, 0), white 70%) 0 100%, radial-gradient(farthest-side at 0% 50%, rgba(0, 0, 0, 0.2), rgba(0, 0, 0, 0)), radial-gradient(farthest-side at 100% 50%, rgba(0, 0, 0, 0.2), rgba(0, 0, 0, 0)) 0 100%;
  background-repeat: no-repeat;
  background-color: white;
  background-size: 40px 100%, 40px 100%, 14px 100%, 14px 100%;
  background-position: 0 0, 100%, 0 0, 100%;
  background-attachment: local, local, scroll, scroll;
  table{
    margin:0;
  }
}
fhaeberle’s picture

That could be helpful but should be verified by design if we implement such a shadow. @thomasfrobieter Do you want to submit a patch?

junaidmasoodi’s picture

The above snipped won't work in this case because of two things
1. The table is scrolling on X-axis
2. Table height is more than a screen size which will be really hard to add a background of full height.

I have added a patch to scroll on the x-axis only in case if the content is overflowing in the table. Moreover added a screen recording as well.

junaidmasoodi’s picture

junaidmasoodi’s picture

Assigned: junaidmasoodi » Unassigned
Status: Needs work » Needs review
thomas.frobieter’s picture

That could be helpful but should be verified by design if we implement such a shadow. @thomasfrobieter Do you want to submit a patch?

Patch for #33 attached - i'm not very familiar with creating patches, hopefully its correct ;)

Works for me in our Drupal test instance. As mentioned before, the sticky table header feature is a problem with the scroll solution, kinda problematic to fix this, while it uses position fixed. Maybe position:sticky is a possible solution, but lacks browser support. The sticky header works, but will overlap the .table-responsive wrapper.

fhaeberle’s picture

Status: Needs review » Needs work
Issue tags: -Amsterdam2019

@thomas.frobieter Thanks for submitting a patch! Unfortunately, this is not the common way. First, you have to generate a .patch file and additionally, for issues where patches are already there, interdifffs. How to generate a patch file or an interdiff based on your changes you can find instructions here: Making a Drupal patch with Git and Creating an interdiff.

There are also helpful tools like dorgflow which automate the process: https://github.com/joachim-n/dorgflow

Second, I applied your patch and it works but you are missing your styles in the .pcss.css file and therefore the provided styles in tables get overridden on yarn build by our preprocessing. Both .css and .pcss.css are going in the repository. The preprocessing instructions can be found here: frontend developer tools in core

fhaeberle’s picture

amaisano’s picture

Assigned: Unassigned » amaisano

I'll take a whack at the tasks in #39 today.

amaisano’s picture

Adjusted source .pcss.css file. Not sure if we need both that and the normal .css file, since the latter is generated by yarn from the former.

amaisano’s picture

Status: Needs work » Needs review
Suesdesign’s picture

I have tested the patch responsive_table-3068696-42.patch in Firefox and Chrome on Mac OS X and I have found the patch to be working. I have attached animated gifs before and after the patch.

KondratievaS’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
30.49 KB

Tested under Android and IOS and result is OK

ckrina’s picture

Status: Reviewed & tested by the community » Needs work
FileSize
931.74 KB

Thanks for working on this, it looks great and it's a great improvement. Almost there!

I just miss 2 small things, so moving this back to Needs work:

  • When scrolling down, the sticky header gets fixed below the top. It should ideally get fixed at the very top without leaving any space.
  • When scrolling horizontally, the sticky header is horizontally fixed to it doesn't point correctly to the column below (which does scroll horizontally).

Here is a gif to showcase what I'm talking about:

rohitreddygaddam’s picture

I would think all the tables can be wrapped in a div with Overflow x set to auto

overflow-x:auto;
thomas.frobieter’s picture

I think the only chance to fix the sticky header is Javascript, i don't see any solution with pure css.

Something like: https://codepen.io/springborg/pen/MvPmPP (Example 1). But they used table-layout: fixed to simplyfy this, we would need to get the width of each column and set it to the sticky header table coloumns (also to the original table columns, to prevent differences if the content changes). Furthermore the example is jquery.. of course, we need to solve this with vanilla js.

Thoughts?

I would think all the tables can be wrapped in a div with Overflow x set to auto

@rohitreddygaddam Sadly this won't work because the sticky header uses position: fixed, what is relative to the document, not to the container.

--------------------

BTW big thx @amaisano for creating the patch (:

rohitreddygaddam’s picture

@thomasfrobieter i agree with the javascript solution, but it will be better to implement something with vanilla javascript rather than depending on Jquery

thomas.frobieter’s picture

Furthermore the example is jquery.. of course, we need to solve this with vanilla js.

(#48)

;)

mradcliffe’s picture

Issue tags: +midcamp2020

I performed Novice Triage on this issue. I am leaving the Novice tag on this issue because there is a clear proposed resolution and the path forward listed by @ckrina in #46 and @thomas.frobieter in #48.

bnjmnm’s picture

Version: 8.9.x-dev » 9.1.x-dev
Status: Needs work » Needs review
FileSize
10.05 KB
9.7 KB

This rerolls to account for Classy templates being added to Claro and addresses the sticky header horizontal scrolling.

In myscrolling solution, I was unable to find a way to get it fully working without a small modification to tableheader.es6.js. Otherwise, the repositioning of the sticky header on scroll would result in it being pushed to the left. In this solution, it now checks if the sticky header is in a responsive table before deciding on a left offset. Addressing the flickering in Safari was tricky as well, I'd love to find out there's a simpler solution than the one I added.

Note that the version being tested against was changed to 9.1

bnjmnm’s picture

#52 was missing a compiled JS file. Ignore that one and refer to this instead.

lauriii’s picture

Issue summary: View changes
Status: Needs review » Needs work
FileSize
9.34 KB
7.79 KB
  1. Our pre-existing responsive tables solution is hiding information from the tables based on the screen width. Should we disable that in Claro in the light of a more scalable responsive tables solution?
  2. The table head is overflowing to the right when the head is sticky:

    This is how the head looks like when it's positioned normally:

  3. +++ b/core/themes/claro/js/responsive-table.js
    --- a/core/themes/claro/templates/classy/views/views-view-table.html.twig
    +++ b/core/themes/claro/templates/views/views-view-table.html.twig
    

    I think we should try to implement this in non-Views tables too.

piyuesh23’s picture

Issue tags: +DIACWApril2020
bnjmnm’s picture

Status: Needs work » Needs review
FileSize
11.15 KB
20.37 KB

#54 Exposed a need for a big refactor. Previously there was additional logic and a template customization to identify responsive tables, but that functionality already exists. This patch leverages the existing responsive table functionality instead, and replaces core/tableresponsive with a much smaller JS file as there's no need to hide/show columns. Some bits in the new file were copied from there.

Also removed the shadow effect, which I liked but could be disruptive to contrib modules as table rows were styled with transparent backgrounds in order for it to be visible.

This was also tested on RTL and IE.

ckrina’s picture

Thanks for the work! It's great to see the top table header moving together with the rest of the table.
I'd suggest to keep the shadow because it helps understanding the behavior and the different UI elements. It's a visual clue to understand there is something else if for some reason there isn't any element half cut below the margin.

Anybody’s picture

@bnjmnm thank you very much, this is great work!

bnjmnm’s picture

This brings back the shadow in a way that does not require the table cells to have transparent backgrounds. This approach did not work with the previous gradient colors/position settings so the new gradient is my best attempt at getting it to look like the earlier version. It may require someone with a more refined eye to

It was also necessary to add conditional logic so the shadow is only present if it is scrollable. This was especially necessary since this was expanded to be available to all tables. Without this it would appear on many responsive-enabled tables such as the ones present when adding a field in views. In cases like that the shadow definitely looks weird if it can't actually be scrolled

Also had to make an (odd?) change to visually hidden labels in the table. I happened to have the checkboxes in the rightmost column due to working on another issue, and this surfaced the problem. The visually hidden label styling results in incorrect width calculations, which can result in excess scroll either within the responsive table container or the document itself (depending on the use of Node bulk operations vs Views bulk operations).

lauriii’s picture

Issue summary: View changes
Status: Needs review » Needs work
FileSize
36.14 KB

I'm now getting scrollbar to both directions:

I also noticed that the shadow is always visible on both sides. I think the shadow should be only visible on the side where the table can be scrolled to indicate that there's more content available.

bnjmnm’s picture

Status: Needs work » Needs review
Issue tags: -Novice
FileSize
2.87 KB
13.47 KB

This addresses #60.
Looks like the vertical scrolling within the container happens in some browsers and not others. I could reproduce the issue in Safari, the fix is simply an explicit setting of overflow-x to hidden to rule out browsers differently interpreting "auto".

Based on #46, the shadow had been on both sides at some point, but I agree it works better on just one side. Did this + RTL.

lauriii’s picture

I think we should try to imitate how the shadow works on this implementation of responsive tables because the way it works increases clarity on which way the table can be scrolled: https://github.com/iKristjan/bootstrap-responsive-table-scrolling-shadows.

KapilV’s picture

I am facing a similar issue. and I am working on responsive issue.

KapilV’s picture

I am applied patch #61 but the issue not fixed.

bnjmnm’s picture

Assigned: Unassigned » bnjmnm

Working on the suggestions in #62.

I noticed that #63 has drop shadow on both sides, which means it's loading assets from something prior to #61, which addresses the vertical scrolling reported in #60.

#64 are screenshots from the Seven admin theme. This is a Claro issue, so Seven will not be impacted.

bnjmnm’s picture

Assigned: bnjmnm » Unassigned
FileSize
14.19 KB
6.85 KB

Good suggestion in #62, this implements it. The drop shadow only appears when there is overflow on that side.

BhumikaVarshney’s picture

Status: Needs review » Reviewed & tested by the community

Hi @bnjmnm,
This patch works good for me.
Thanks.

xjm’s picture

lauriii’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: -Needs frontend framework manager review

This looks great! I'm wondering if we could make the shadow effect look a bit more natural. The shadow on this blog post looks great for example: http://lea.verou.me/2012/04/background-attachment-local/. On the demo, the shadow appears slowly when scrolling. Also, the shadow is curved. I know it means we will have to make some changes to the way this has been implemented but I'm wondering if it's something that could be done in a reasonable amount of time.

thomas.frobieter’s picture

+++ b/core/themes/claro/css/components/tables.css
@@ -301,28 +301,52 @@ tfoot tr:first-child td {
+.responsive-enabled-outline--scroll:after,
+.responsive-enabled-outline--scroll:before {
...
+.responsive-enabled-outline--scroll-left:before {
...
+.responsive-enabled-outline--scroll-right:after {

+++ b/core/themes/claro/css/components/tables.pcss.css
@@ -211,27 +211,46 @@ tfoot tr:first-child td {
+.responsive-enabled-outline--scroll:after,
+.responsive-enabled-outline--scroll:before {
...
+.responsive-enabled-outline--scroll-left:before {
...
+.responsive-enabled-outline--scroll-right:after {

CSS pseudo elements should be written with double colons (::before / ::after, ==> https://css-tricks.com/to-double-colon-or-not-do-double-colon/)

bnjmnm’s picture

Status: Needs work » Needs review
FileSize
14.9 KB
4.85 KB
196.41 KB

The approach suggested in #69 was in the patch in #38, but has two issues I couldn't find a solution to. First, it requires all child elements to have transparent backgrounds, which I don't think is feasible as this applies to all responsive tables. Second: the shadow winds up behind the sticky header, and adjusting Z-index hides the sticky header entirely. There may be solutions to both of these, but I wasn't able to find them.

I attempted an approximation by adding border-radius and opacity transitions to the current approach. Not entirely sure if it's better/worse than the prior patch, so feedback is very welcome.

KondratievaS’s picture

Tested patch from #71 and result is OK for me in Android and IOS (video in attachment)

KondratievaS’s picture

Status: Needs review » Reviewed & tested by the community
xjm’s picture

Tagging for frontend committer review. Thanks!

lauriii’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Needs work
FileSize
6.45 MB
  1. The sticky table header isn't playing out nicely with the toolbar:
  2. +++ b/core/misc/tableheader.es6.js
    @@ -223,6 +223,9 @@
    +        $(window).trigger('tableheaderAddSticky');
    
    +++ b/core/themes/claro/css/components/tables.css
    @@ -301,28 +301,66 @@ tfoot tr:first-child td {
    +  top: 0;
    

    Should we have more clarity on the event name by mentioning that the event is triggered after the sticky table header was added?

bnjmnm’s picture

Status: Needs work » Needs review
FileSize
16.82 KB
5.59 KB
155.32 KB
199.1 KB

#75.1 - it looks like Seven's sticky header doesn't work great with toolbar either, as evidenced by the attached screenshots, but I think I was able to take care of it. I couldn't reproduce the exact symptoms shown, but I was running into several toolbar/header problems that were addressed by this solution.

#75.2 made the event naming clearer.

KondratievaS’s picture

FileSize
80.96 KB

Tested patch from #76 and there are 2 bugs when close/open toolbar

bug

KondratievaS’s picture

Status: Needs review » Needs work
bnjmnm’s picture

Status: Needs work » Needs review
FileSize
16.87 KB
1.43 KB

Thanks @KondratievaS! I was a able to consistently reproduce the issue by having the viewport be at a width where opening the tray results in the left/right padding of the page changing. This patch took care of it for me, but definitely needs manual tests from another contributor so we can sure I caught all the potential use cases.

KondratievaS’s picture

FileSize
574.71 KB

Tested patch form #79 and on my side result is OK. Tested using emulator and on real device

OK

KondratievaS’s picture

Status: Needs review » Reviewed & tested by the community
nod_’s picture

Assigned: Unassigned » nod_
Issue tags: +JavaScript

Having a look.

nod_’s picture

Assigned: nod_ » Unassigned
Status: Reviewed & tested by the community » Needs work

I like this new behavior, mainly because it looks good and there is very little JS necessary for it, so yay, but got a few problems that are blocking to me.

  1. +++ b/core/misc/tableheader.es6.js
    @@ -244,6 +247,18 @@
    +            parent.getBoundingClientRect().x ||
    +            parent.getBoundingClientRect().left;
    

    better to put the result of parent.getBoundingClientRect() in a variable.

  2. +++ b/core/themes/claro/claro.libraries.yml
    @@ -271,6 +271,23 @@ filter:
    +tableheader:
    +  version: VERSION
    +  js:
    +    js/tableheader.js: {}
    +  dependencies:
    +    - core/jquery
    +    - core/drupal
    

    This script doesn't use jquery.once, but it definitely should. otherwise the event listener is duplicated for every ajax request on the page.

  3. +++ b/core/themes/claro/claro.libraries.yml
    @@ -271,6 +271,23 @@ filter:
    +tableresponsive:
    +  version: VERSION
    +  js:
    +    js/tableresponsive.js: {}
    +  dependencies:
    +    - core/jquery
    +    - core/drupal
    +    - core/drupal.object.assign
    

    missing dependency on core/jquery.once

  4. +++ b/core/themes/claro/css/components/tables.pcss.css
    --- /dev/null
    +++ b/core/themes/claro/js/tableheader.es6.js
    

    This file should be named something else. Here we add to the exiting base behavior whereas the other file replace the feature entierly. Need to be coherent.

  5. +++ b/core/themes/claro/js/tableheader.es6.js
    @@ -0,0 +1,18 @@
    +      $(document).on('drupalToolbarTrayChange', () => {
    

    need to .once() this.

    Shouldn't this be in the core file? why only claro?

  6. +++ b/core/themes/claro/js/tableresponsive.es6.js
    @@ -0,0 +1,143 @@
    +/**
    + * @file
    + * Responsive table functionality.
    + */
    

    I have a problem with this file.

    This is not a "reponsive table", this is a "scrollable table" or something. For drupal a responsive table has the assumption that columns will be hidden to fit the viewport without scrolling.

    Here the assumption is that we display all the columns, all the time. As far as Drupal is concerned this is not "reponsive" the way we mean it.

    I don't think it is ok to have a the same feature with two totally different assumptions and implementations.

    We need a new table feature in core, that is different from the responsive tables. To me the change needs to happen on the render array level with a #scrollable property or something, the same way we have #responsive.

  7. +++ b/core/themes/claro/js/tableresponsive.es6.js
    @@ -0,0 +1,143 @@
    +        const wrapperWidth = wrapper.getBoundingClientRect().width;
    

    save result of wrapper.getBoundingClientRect() in a variable, it's used a lot later one. same for table.getBoundingClientRect().

  8. +++ b/core/themes/claro/js/tableresponsive.es6.js
    @@ -0,0 +1,143 @@
    +  Drupal.TableResponsive = TableResponsive;
    

    Need to rename this.

bnjmnm’s picture

Assigned: Unassigned » bnjmnm

I'm working on addressing the feedback in #83, assigning to myself.

Patch is on the way, but I already created a followup issue based the comment #83.5

This was little more than force of habit, there are many Claro features I'd love to see in core only to discover it isn't feasible. In this case, it's a very simple change that doesn't break BC, I'm glad this got pointed out. Created #3145930: Tableheader should recalculate on toolbar tray toggle if anyone feels like reviewing a two line patch.

bnjmnm’s picture

Assigned: bnjmnm » Unassigned
Status: Needs work » Needs review
FileSize
18.74 KB

We need a new table feature in core, that is different from the responsive tables. To me the change needs to happen on the render array level with a #scrollable property or something, the same way we have #responsive.

This patch is an attempt at that approach. Views doesn't use the render element for tables such as the one at admin/content so the library isn't attached based on #scrollable. In this patch I opted to attach the library via Twig, but it could just as easily be attached permanently via preprocess_views_view_table. Wasn't sure what the consensus preference would be, but also figured there'd be discussion regardless, so that can get figured out here.

So after folks have had an opportunity to look over the approach, the next steps would be one of two things:
If the #scrollable property is a good approach:
Determine if it should be done in the scope of this issue, or a separate issue. If the former, the issue summary (and possibly metadata_ should be updated accordingly.
If this doesn't seem like the right approach (or if it is offloaded to a separate issue)
Development/interdiff should continue from the patch in #79

The patch here also addresses the other feedback in #83, where applicable.

priyanka.sahni’s picture

Assigned: Unassigned » priyanka.sahni
priyanka.sahni’s picture

Verified and tested by applying patch #85.It was applied successfully.

Below are my observations :
1. On scrolling the content page , the menu toolbar is moving downwards and it should be static.
2. On scrolling the content page and open the menu toolbar and close it , at times the filters is getting displayed twice.

Refer to the images and videos attached.

After Patch

After Patch

priyanka.sahni’s picture

Assigned: priyanka.sahni » Unassigned
KondratievaS’s picture

Status: Needs review » Needs work
bnjmnm’s picture

Status: Needs work » Needs review

Thanks for the the review (especially with video examples) in #87 @priyanka.sahni. In this case the two items pointed out are not in the scope

  • On scrolling the content page , the menu toolbar is moving downwards and it should be static.

    This about the its the toolbar tray, not table overflow, and the behavior was not caused by the patch. There is an existing issue for this, and this links to when this specific symptom was first noticed: #2516938: Set the toolbar to position fixed on mobile

  • On scrolling the content page and open the menu toolbar and close it , at times the filters is getting displayed twice.

    In #83.5, @_nod requested that this be moved to a dedicated issue that provides the fix to all of core, not just Claro. this issue is currently RTBC #3145930: Tableheader should recalculate on toolbar tray toggle

The biggest review need at the moment is feedback due to #85 using a very different approach that was suggested in #83.6. It would be good to determine sooner than later if the #85 or #79 is preferred so additional work is done based on the patch that will actually land.

nod_’s picture

small changes over 85, don't mind me.

nod_’s picture

nod_’s picture

Issue summary: View changes

update IS with current state of things, feel free to detail things and pass it around :)

droplet’s picture

this structure has another approach.

do not update the `left` value on scroll. Just set `sticky-header` to `absolute` when `scrollLeft > 0`. Then the browser handles scrolling natively. No more flickering on scroll left/right :)

** memorize the Fixed style and scrollTop. If `prev.scrollTop !== now.scrollTop`, restore back to Fixed (a.k.a monitoring vertical scroll)

bnjmnm’s picture

This

  • Applies the approach in #94
  • Per a Slack discussion with @_nod and @lauriii, we are replacing #responsive with #scrollable, but in Claro only.
  • The JS rewritten to use class{} - which we should do regardless but makes #94much easier

This was a major refactor so it'll definitely need another round of manual testing.

nod_’s picture

Status: Needs review » Needs work

Still looks good :) didn't test exhaustively.
When JS is disabled it's not much better than before though, could we improve something in no-js? even if we don't have the full experience?

  1. +++ b/core/themes/claro/js/tablescroll-init.es6.js
    @@ -0,0 +1,38 @@
    +          .wrap('<div class="scrollable-table-outline"></div>')
    +          .wrap(
    +            '<div class="scrollable-table-wrapper" data-drupal-scrollable-table-wrapper></div>',
    +          );
    

    Could we do some of that from PHP/twig?

  2. +++ b/core/themes/claro/js/tablescroll-init.es6.js
    @@ -0,0 +1,38 @@
    +        const il = $tables.length;
    +        for (let i = 0; i < il; i++) {
    +          Drupal.TableScrolls.tables.push(new Drupal.TableScroll($tables[i]));
    +        }
    

    That's a pretty old way of doing it (I'm guilty of it), can we use Drupal.TableScrolls.tables = $tables.map((index, table) => new ...); or something like that?

  3. +++ b/core/themes/claro/js/tablescroll.es6.js
    @@ -0,0 +1,164 @@
    +      this.wrapper.addEventListener('scroll', () =>
    ...
    +      window.addEventListener('resize', () =>
    ...
    +      $(document).on('drupalToolbarTrayChange', () =>
    ...
    +      $(window).on('tableheaderCreateSticky', () => {
    ...
    +        this.wrapper.addEventListener('scroll', () =>
    ...
    +        window.addEventListener('scroll', () => this.updateStickyHeader());
    +        window.addEventListener('resize', () => this.updateStickyHeader());
    ...
    +        $(document).on('drupalToolbarTrayChange', () =>
    

    For event it should be all jQuery or no jQuery but mixing the two won't do. Using jquery here would make the code simpler and more readable as well. Usually we suffix the event name with a qualifier to make it easier to clean up if necessary, in the tableheader script we use scroll.TableHeader for exemple.

    I would add some debounce on the scroll and resize handlers too because they fire, way way too much.

Thanks! don't think we're too far away from the end :)

nod_’s picture

+++ b/core/misc/tableheader.es6.js
@@ -223,6 +225,9 @@
+        $(window).trigger('tableheaderCreateSticky');

I would trigger that on document, not window. Any reasons it should be on window?

bnjmnm’s picture

Status: Needs work » Needs review
FileSize
40.31 KB
20.13 KB

Addresses #96 and #97, plus removes an excess addition of data-drupal-table-scrollable in preprocess_table() and adds initialization of scrollable attributes in the TableScroll constructor. (particularly pleased that no-js is much better thanks to #96.1)

Something to note during review: The diff shows many changes made on the two twig files, but anything below {% table %} is just an indentation change because it's now contained in a block.

nod_’s picture

Status: Needs review » Needs work

no-js version is great, thanks :)

  1. +++ b/core/themes/claro/claro.info.yml
    @@ -52,6 +52,8 @@ libraries-override:
    +  core/drupal.tableresponsive: false
    

    Since we remove #responsive this shouldn't be needed?

  2. +++ b/core/themes/claro/js/tablescroll-init.es6.js
    @@ -0,0 +1,29 @@
    +  Drupal.TableScrolls = {
    +    tables: [],
    +  };
    

    We have Drupal.TableScrolls and Drupal.TableScroll in the code, that's a little confusing. Any way to get that into one object in the Drupal namespace?

  3. +++ b/core/themes/claro/js/tablescroll.es6.js
    @@ -0,0 +1,171 @@
    +      // Add/remove classes used for styles that indicate available scrolling.
    +      wrapperOutline.classList.toggle(
    +        'scrollable-table-outline--scroll',
    +        wrapperWidth < tableWidth,
    +      );
    +      wrapperOutline.classList.toggle(
    +        'scrollable-table-outline--scroll-left',
    +        wrapperOffset > tableOffset,
    +      );
    +      wrapperOutline.classList.toggle(
    +        'scrollable-table-outline--scroll-right',
    +        wrapperOffset + wrapperWidth < Math.floor(tableOffset + tableWidth),
    +      );
    

    +1 to that but the second argument is not supported by any version of IE: https://developer.mozilla.org/en-US/docs/Web/API/Element/classList. I would go with jQuery toggle unfortunately.

bnjmnm’s picture

Status: Needs work » Needs review
FileSize
4.29 KB
39.9 KB

Addresses #99

nod_’s picture

code is ok for me.

Seems like the horizontal scrollbar is always displayed on desktop.

bnjmnm’s picture

Seems like the horizontal scrollbar is always displayed on desktop.

Good catch! For some reason I thought overflow: auto; wouldn't work, but I tested it on all available browsers and it's fine and IE11 will only display the scrollbar when there is overflow.

nod_’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me :)

droplet’s picture

+++ b/core/themes/claro/js/tablescroll.es6.js
@@ -0,0 +1,174 @@
+      $(this.wrapper).on('scroll.TableScroll', () =>
+        debounce(this.applyHorizontalScrollSelectors(), 150),
+      );

All debounce is incorrect. It re-creates the instance each time.

nod_’s picture

Status: Reviewed & tested by the community » Needs work

right and it doesn't even debounce since the function is called and it's the return of that that is debounced.

nod_’s picture

Status: Needs work » Needs review
FileSize
40.18 KB
7.68 KB

fixed the debounce usage. It now debounce across the different events.

We need .bind(this) because of how the code is compiled.

droplet’s picture

Status: Needs review » Reviewed & tested by the community
lauriii’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Needs work
FileSize
59.06 KB
  1. +++ b/core/themes/claro/claro.theme
    @@ -989,6 +1002,18 @@ function claro_preprocess_views_view_table(&$variables) {
    +  // The value of $variables['responsive'] for views tables is not determined
    +  // by a render array value. Instead \template_preprocess_views_view_table()
    +  // checks for any column configured to be hidden at narrower widths. This
    +  // prevents the removal of the `responsive` property earlier in rendering.
    +  if (!empty($variables['responsive'])) {
    
    +++ b/core/themes/claro/src/ClaroPreRender.php
    @@ -186,6 +186,30 @@ public static function messagePlaceholder(array $element) {
    +  public static function table(array $element) {
    +    $is_scrollable = $element['#scrollable'] ?? !empty($element['#responsive']);
    +    if ($is_scrollable) {
    

    I don't think we should rely on what people have configured with the responsive setting because it works very differently. Sometimes all the information displayed on a table is important which people don't want to hide. On those instances, the table could be scrollable because it doesn't remove any data. I think we should just remove the #responsive everywhere, add #scrollable everywhere except if it has been explicitly set as false.

  2. Manually tested this on a Views table where the scrolling should be disabled. It seems like for some reason the wrapper is still being added which makes the table scrollable:
bnjmnm’s picture

Status: Needs work » Needs review
FileSize
5.05 KB
41.5 KB

This addresses both points in #108

nod_’s picture

Agreed with #108.1, fix looks good too. RTBC on my end, I'll let the final say to twig folks

droplet’s picture

Is it enabled by default? other non-views tables don't work, for example `admin/structure/views`

If it's enabled by default (,make a API-broken like change), I suggested adding table scroll wrapper all time.
(for better developer experience, yes or no I'd add the wrapper all time.)

and we should call it `table-wrapper` instead.

and `scrollable-table-outline` is unnecessary I think (a single wrapper is okay)

-----------

      $wrapperOutline.toggleClass(
        'scrollable-table-outline--scroll',
        wrapperWidth < tableWidth,
      );

remove also?

-----------

<div class="scrollable-table-wrapper" data-drupal-scrollable-table-wrapper="">

should be either one

--------------

Drupal.TableScroll.tables = [];

it should be

Drupal.TableScroll.tables = Drupal.TableScroll.tables || [];

or consider to remove it?

bnjmnm’s picture

Re: #111

Is it enabled by default? other non-views tables don't work, for example `admin/structure/views`

It looks like there are multiple tables that were still not receiving the scrollable = true property, but should have been. I tried to identify these additional use cases and update them in the patch.

If it's enabled by default (,make a API-broken like change), I suggested adding table scroll wrapper all time.
(for better developer experience, yes or no I'd add the wrapper all time.)

The wrapper is conditional to support nojs. This ensures that tables configured with scrollable: FALSE are not horizontally scrollable. This guarantees that the value of #scrollable has a consistient result.

and `scrollable-table-outline` is unnecessary I think (a single wrapper is okay)

I'm certainly open to the possibility that it's possible to have a single wrapper, and this was attempted. The two wrappers was the only solution I could find where the the left/right shadows indicating overflow did not scroll horizontally with the rest of the table. The outline guarantees that right:0 / left: 0 is actually on their respective side of the container regardless of scroll position.

'scrollable-table-outline--scroll' remove also?

I think there's a benefit in having this class available to contrib developers, not just to have a single selector for styling horizontally-scrollable tables, but it makes it much easier to target non-horizonatlly-scrollable tables with a not(.scrollable-table-outline--scroll). I'm curious what the benefits in removing this would be vs. the rationale for having it.

should be either one

This was to separate styling from functionality -- does this not seem applicable in this instance? The class is for CSS to target, and the the data attribute is used by JavaScript. There's been a preference for data attributes vs js- prefixed classes, but this was an attempt to separate concerns much like other modules/themes have.

Drupal.TableScroll.tables = [];

it should be

Drupal.TableScroll.tables = Drupal.TableScroll.tables || [];

I'm making the change since there's no apparent drawbacks but I'm curious: If libraries are properly used, are there instances where this could be initialized a second time even though it's not inside a behavior? I assume it's just something I'm not aware of

**********************

Also made a change to fix an issue with the right overflow shadow not consistently disappearing when there is no remaining scrollable area on the right.

samiullah’s picture

Have Tested the patch given on #111
Looks good
Horizontal scroll with little shadow effect looks good

@bnjmnm if code review is needed then after that it can be moved to RTBC

lauriii’s picture

Issue tags: +Needs followup

Discussed this on the Claro weekly call with @yoroy, @bnjmnm, @nod_ and @katherined. We reviewed #112 and agreed that it addresses #111. @nod_ raised some concerns on the impacts of this for non-touch devices. Horizontal scrolling is jarring on non-touch devices, so the solution isn't idea for them. While it might seem that the scrolling would only happen on mobile devices, it could happen on desktop environment too when the table has lots of columns, or when browser is not being used in full-screen. On desktop, the current experience is that there's a scroll bar on the bottom of the browser when the table doesn't fit the page. After this change, the scrollbar is below the table, meaning that on long tables, it could be harder for users to scroll. We thought that the benefits of this change outweigh the potential regression for some desktop users, so we decided to recommend moving forward with the patch as it is. However, we agreed that we should open a follow-up to see if we could improve the experience of non-touch devices.

Anybody’s picture

@lauriii: Thank you, I 100% agree!
We might afterwards have a look at how other large UI libraries solve this. But the benefits are a lot larger. The explained behaviour could even be beneficial for desktop browsers on pages with several tables.... one could discuss that.

Thank you and RTBC+1

bnjmnm’s picture

nod_’s picture

  1. +++ b/core/themes/claro/claro.libraries.yml
    @@ -279,6 +279,18 @@ filter:
    +    - core/drupal.object.assign
    

    Don't need this polyfill no use of assign in the latest version of the code.

  2. +++ b/core/themes/claro/js/tablescroll.es6.js
    @@ -0,0 +1,179 @@
    +  Drupal.TableScroll.tables = Drupal.TableScroll.tables || [];
    

    since we declare the class above I don't see how the tables property would exist. In any case it's not going to break anything, can go in like this.

I'd be tempted to put that RTBC, no reason to hold this up longer now we need people to try it out and see if that's better than before.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

fhaeberle’s picture

lauriii’s picture

Status: Needs review » Needs work

Moving to NW to address #117.

ravi.shankar’s picture

Status: Needs work » Needs review
FileSize
44.9 KB
826 bytes

Tried to address comment #117.

lokeshsahu’s picture

Status: Needs review » Needs work
FileSize
55.92 KB

The patch has applied correctly but it has not resolved the responsive issue.

bnjmnm’s picture

@lokeshsahu I noticed your screenshot is for the Seven theme. You'll only see the benefits of this patch if you are using the Claro theme.

djsagar’s picture

Status: Needs work » Needs review

Here is the same issue on https://www.drupal.org/project/drupal/issues/3132811#comment-13970382

Patch was created there please review.

Thanks!

hinal05’s picture

@djsagar
Applied patch #124 (https://www.drupal.org/project/drupal/issues/3132811#comment-13970382).
It is working fine for seven and I have applied same changes for claro and it's also working now. Please review the screenshot and claro patch file.

ravi.shankar’s picture

Status: Needs review » Needs work

@hinal05, please fix custom command fails.

djsagar’s picture

Status: Needs work » Needs review
FileSize
1.53 KB
1015 bytes

Rolling up patch as it's custom command fails.

@hinal05, In your patch need to compile css by PostCSS.

Follow this steps which is provided in this link https://www.drupal.org/node/3084859.

Thanks!

hinal05’s picture

@djsagar Thanks for the correction.

Abhijith S’s picture

Applied patch #127 and it works fine.Added screenshots.

Before patch;
before

before2

After patch:
after

after2

RTBC +1

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

pragati_kanade made their first commit to this issue’s fork.

pragati_kanade’s picture

Before

pragati_kanade’s picture

After

pragati_kanade’s picture

Status: Needs review » Reviewed & tested by the community

#127 Patch Applied successfully. Looks good to me.

bnjmnm’s picture

Status: Reviewed & tested by the community » Needs work

The approach that got RTBC'd is one that was shown not to work way back in comment #46. That simple CSS approach breaks sticky headers. This has completely discarded the changes that successfully address that concern. The issue that this incorrect approach was inspired by also had a comment explaining why the approach wouldn't work #3132811: Tables cause layout issues if wider than the viewport, especially on mobile devices.

I know going through all those comments takes a while, but it's necessary to avoid duplicating work that was already proven to not be an effective solution. I'm creating an MR with the contents of #121

bnjmnm’s picture

Status: Needs work » Needs review

Merge request open with the logic that addresses #46. Reminder that the css only approach will not work. Due to sticky headers.

Madhu kumar’s picture

FileSize
21.34 KB

Patch #127 applied cleanly and working as expected, sharing screenshot for reference.

bnjmnm’s picture

Re #138 That is applying a patch that was shown to not fully work in #46, which I provided a reminder for just a few comments up in #135 and #137

This may come across as obnoxious, but it seems like it's not clear that the approach in the patches is one that will not work, so here goes:

THE CSS ONLY APPROACH IN THE PATCHES WILL NOT WORK. IT WILL BREAK WITH STICKY HEADERS + HORIZONTAL SCROLLING. USE THE APPROACH IN THE MERGE REQUEST (not trying to yell, just want visibility 🙂)

bnjmnm’s picture

Issue summary: View changes
nod_’s picture

fixed the leftover from #117 and updated for branch 9.3.x

nod_’s picture

also for reference, with latest chrome update a css only solution exists. Probably won't work for us unless we rewrite/remove tableheader though. Just pointing out possibilities :)

mherchel’s picture

FileSize
2.72 MB

There’s still an issue where the sticky table header doesn’t properly bind to the top of the page when the toolbar isn’t fixed.

I did a good amount of debugging, and traced this into the Drupal.displace() method, though. The issue is that the method searches for [data-offset-top] and will calculate the height of that, even if the element is not set to position: fixed.

We need a followup issue.

More comments incoming.

nod_’s picture

mherchel’s picture

Status: Needs review » Reviewed & tested by the community

This is looking amazing! I tested out the functionality as much as I could and didn't find any issues.

The code looks great. I looked through the changes, but didn't go quite as far as stepping through or anything.

Note there is one potential issue:

If the table has overflow on it, and the last row of the table has a dropbutton component with a lot of items, it can get clipped, and the user would not be able to access all of the items. I'm hoping this can be a followup issue though, since it's somewhat of an edge case.

lauriii’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: -ContributionWeekend2020, -ContributionWeekendCH, -midcamp2020, -DIACWApril2020, -Needs frontend framework manager review

Posted feedback on the MR. Functionality is starting to look really solid. I've tested this manually couple of times, but this time I also tested this with Tours and it seems to integrate very nicely. Tour is able to scroll the table as it's supposed to given that we are using built in browser functionality for the scrolling.

vikashsoni’s picture

Applied patch #127 applied successfully looks good for me
for ref sharing screenshot ....

bnjmnm’s picture

Removing credit for #147 because:

  • There are sufficient screen caputres already
  • A static screenshot isn't particularly useful, this issue requires motion to prove it works
  • The before and after screenshots are identical, so there's no before/after. In this case, they are showing "before" so it's effectively two screenshots of Claro that aren't specific to this issue.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

andy-blum made their first commit to this issue’s fork.

andy-blum’s picture

Re-queued the tests, the failures don't seem like anything really related to this issue.

ERROR: Step ‘Publish JUnit test result report’ failed: No test report files were found. Configuration error?

xjm’s picture

Just a recommendation for the future: Always rebase MRs. There's even a button in the UI for this if you click on the MR link in the summary. If you must, rebase locally and force push. Do not merge HEAD. It's even preferable to open a new MR over that.

As for this message:

ERROR: Step ‘Publish JUnit test result report’ failed: No test report files were found. Configuration error?

It gets displayed all the time, even in passing tests, so is unrelated. Your tests are failing much earlier than that.

xjm’s picture

The MR also does not apply:

error: core/themes/claro/templates/admin/views-ui-views-listing-table.html.twig: does not exist in index
andy-blum’s picture

@xjm just saw your comment about rebasing after already merging - sorry.

xjm’s picture

Status: Needs work » Needs review

OK, the new MR tries to save this poor issue from Merge conflicts of DOOM!. However, I am not a frontend developer and not actually familiar with the issue, so please check the initial commit there very carefully to make sure it does not include any out-of-scope changes or regressions from other issues.

Good luck, lol. 😂 🤞

mherchel’s picture

Status: Needs review » Needs work
FileSize
139.07 KB
366.7 KB

I really want this to work so we can stabilize Claro, but to be honest, this approach seems dead in the water to me.

If you set overflow on the table container (which is fundamentally necessary for scrollable tables), then any elements inside will be clipped at the containers edges.

The problem is that Drupal's admin UIs frequently have dropdowns (either <select>s or drop buttons) that do exactly this (and rely on this functionality).

This is an example from /admin/structure/comment To reproduce this, you simply need to enable the comments module.

IMHO creating a generic one-size-fits-all responsive <table> solution is impossible with today's browser technology, which is why there's no goto solution for this on the web.

Before

After

lauriii’s picture

#157: Isn't that because we are setting overflow-y: hidden;? Is there no way for us to change that to overflow-y: visible; without introducing a vertical scroll?

bnjmnm’s picture

Issue summary: View changes
FileSize
80.27 KB

#158 right, the overflow-y: hidden; is to prevent a vertical scroll.

A native <select> works fine with this approach, so this appears to be specific to dropbutton.

I was hoping that dropbuttons were also solely responsible for the vertical scrolling happening in the first place, but it still happens without them (there is, however, more vertical scrolling when dropbuttons are present).

FWIW I tried this out in Bootstrap's responsive tables and it has the same issue. The horizontally scrollable tables can't handle an element in the table with positioning that alters the default flow.

mherchel’s picture

The only way I can see this working is to attach a JS custom event to the dropbutton open, and then we can use that event to trigger a calculation that gets the height of the expanded dropbutton and then adds padding-bottom to the overflow container.

But, IMO that's super brittle.

ckrina’s picture

We’ve discussed this issue in a call with @andy-blum @bnjmnm @lauriii @mherchel and @shaal. We did some testing on various devices to understand the status quo. It seems like the scrolling behavior is acceptable and we thought it probably shouldn’t be a Claro stable blocker because:
- Seven has the same behavior
- The table is still usable
- The way it looks&works in mobile devices is good enough nowadays, and a normal behavior for wide tables.

But we still see this as something that needs to be fixed. Per the discussion we’ve had there are several JS solutions to some of the problems found along the issue, but that might require some exploration and it shouldn’t block Claro getting into core.

Some thoughts were that we can position the dropdown against the <body> so it will show above the overflow clip with JS or use the popperjs library in core, and always scope that into elements provided by core like the splitbutton component that is under development.

ckrina credited shaal.

ckrina’s picture

andy-blum’s picture

Issue tags: -stable blocker

removing stable blocker tag per #161

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Anybody’s picture

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.