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.
Comment | File | Size | Author |
---|---|---|---|
#159 | Screen Shot 2022-02-28 at 8.53.36 AM.png | 80.27 KB | bnjmnm |
#157 | after.gif | 366.7 KB | mherchel |
#157 | before.gif | 139.07 KB | mherchel |
#147 | 3068696--after--patch--pic.png | 36.55 KB | vikashsoni |
#147 | 3068696--before--patch.png | 36.55 KB | vikashsoni |
Issue fork drupal-3068696
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:
Comments
Comment #2
ckrinaWould 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.
Comment #3
lauriiiI replaced the screenshot with a gif ✌️
Comment #4
ckrinaAdding stable blocker tag. Also animated gifs ++
Comment #5
anevins CreditAttribution: anevins at Investis Digital commentedI 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-...
Comment #6
fhaeberle@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.
Comment #7
Mithun SEven 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.
Comment #8
Mithun SPlease consider the new patch applied.
Comment #9
fhaeberle@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.Comment #10
junaidmasoodi CreditAttribution: junaidmasoodi as a volunteer and at Acquia commentedComment #11
junaidmasoodi CreditAttribution: junaidmasoodi as a volunteer and at Acquia commentedI am looking into this over this weekend
Comment #12
fhaeberleActually, 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.
Comment #13
kunalkapoor CreditAttribution: kunalkapoor as a volunteer commentedI have added a patch it will make the table responsive please try this and let me know if it works
Comment #14
huzookaWouldn'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.
Comment #15
huzookaComment #16
lauriiiI 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.
Comment #17
fhaeberle+1 fixing it in Claro
Comment #18
fhaeberleComment #19
fhaeberleComment #20
huzookaComment #21
Peter MajmeskuI 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.
Comment #22
Peter MajmeskuThe option from #6 looks like broken to me and should not be the demand for a core theme.
Comment #23
fhaeberleAt 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?
Comment #24
Peter MajmeskuI had applied the patch from #13 and could not make it working:
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.
Comment #25
elly175 CreditAttribution: elly175 commentedThis issue needs a summary, please could you clarify what needs updating/reviewing?
Comment #26
rachel_norfolkretagging
Comment #27
Peter MajmeskuThe issue summary is completed. Please review.
Comment #28
Peter MajmeskuComment #29
Peter MajmeskuComment #30
John Cook CreditAttribution: John Cook commentedI'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.
Comment #31
junaidmasoodi CreditAttribution: junaidmasoodi as a volunteer and at Acquia commentedComment #32
fhaeberle@junaidmasoodi Are you working on this?
Comment #33
thomas.frobieterWhat 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):
Comment #34
fhaeberleThat could be helpful but should be verified by design if we implement such a shadow. @thomasfrobieter Do you want to submit a patch?
Comment #35
junaidmasoodi CreditAttribution: junaidmasoodi as a volunteer and at Acquia commentedThe 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.
Comment #36
junaidmasoodi CreditAttribution: junaidmasoodi as a volunteer and at Acquia commentedComment #37
junaidmasoodi CreditAttribution: junaidmasoodi as a volunteer and at Acquia commentedComment #38
thomas.frobieterPatch 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.
Comment #39
fhaeberle@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 onyarn 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 coreComment #40
fhaeberleComment #41
amaisano CreditAttribution: amaisano commentedI'll take a whack at the tasks in #39 today.
Comment #42
amaisano CreditAttribution: amaisano as a volunteer commentedAdjusted 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.
Comment #43
amaisano CreditAttribution: amaisano as a volunteer commentedComment #44
Suesdesign CreditAttribution: Suesdesign as a volunteer commentedI 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.
Comment #45
KondratievaS CreditAttribution: KondratievaS at Skilld commentedTested under Android and IOS and result is OK
Comment #46
ckrinaThanks 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:
Here is a gif to showcase what I'm talking about:
Comment #47
rohitreddygaddam CreditAttribution: rohitreddygaddam as a volunteer commentedI would think all the tables can be wrapped in a div with Overflow x set to auto
Comment #48
thomas.frobieterI 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?
@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 (:
Comment #49
rohitreddygaddam CreditAttribution: rohitreddygaddam as a volunteer commented@thomasfrobieter i agree with the javascript solution, but it will be better to implement something with vanilla javascript rather than depending on Jquery
Comment #50
thomas.frobieter(#48)
;)
Comment #51
mradcliffeI 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.
Comment #52
bnjmnmThis 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
Comment #53
bnjmnm#52 was missing a compiled JS file. Ignore that one and refer to this instead.
Comment #54
lauriiiThis is how the head looks like when it's positioned normally:
I think we should try to implement this in non-Views tables too.
Comment #55
piyuesh23 CreditAttribution: piyuesh23 at QED42 commentedComment #56
bnjmnm#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.
Comment #57
ckrinaThanks 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.
Comment #58
Anybody@bnjmnm thank you very much, this is great work!
Comment #59
bnjmnmThis 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).
Comment #60
lauriiiI'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.
Comment #61
bnjmnmThis 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.
Comment #62
lauriiiI 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.
Comment #63
KapilV CreditAttribution: KapilV as a volunteer and at OpenSense Labs commentedI am facing a similar issue. and I am working on responsive issue.
Comment #64
KapilV CreditAttribution: KapilV as a volunteer and at OpenSense Labs commentedI am applied patch #61 but the issue not fixed.
Comment #65
bnjmnmWorking 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.
Comment #66
bnjmnmGood suggestion in #62, this implements it. The drop shadow only appears when there is overflow on that side.
Comment #67
BhumikaVarshney CreditAttribution: BhumikaVarshney as a volunteer and at OpenSense Labs commentedHi @bnjmnm,
This patch works good for me.
Thanks.
Comment #68
xjmComment #69
lauriiiThis 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.
Comment #70
thomas.frobieterCSS pseudo elements should be written with double colons (::before / ::after, ==> https://css-tricks.com/to-double-colon-or-not-do-double-colon/)
Comment #71
bnjmnmThe 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.
Comment #72
KondratievaS CreditAttribution: KondratievaS at Skilld commentedTested patch from #71 and result is OK for me in Android and IOS (video in attachment)
Comment #73
KondratievaS CreditAttribution: KondratievaS at Skilld commentedComment #74
xjmTagging for frontend committer review. Thanks!
Comment #75
lauriiiShould we have more clarity on the event name by mentioning that the event is triggered after the sticky table header was added?
Comment #76
bnjmnm#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.
Comment #77
KondratievaS CreditAttribution: KondratievaS at Skilld commentedTested patch from #76 and there are 2 bugs when close/open toolbar
Comment #78
KondratievaS CreditAttribution: KondratievaS at Skilld commentedComment #79
bnjmnmThanks @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.
Comment #80
KondratievaS CreditAttribution: KondratievaS at Skilld commentedTested patch form #79 and on my side result is OK. Tested using emulator and on real device
Comment #81
KondratievaS CreditAttribution: KondratievaS at Skilld commentedComment #82
nod_Having a look.
Comment #83
nod_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.
better to put the result of parent.getBoundingClientRect() in a variable.
This script doesn't use jquery.once, but it definitely should. otherwise the event listener is duplicated for every ajax request on the page.
missing dependency on core/jquery.once
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.
need to .once() this.
Shouldn't this be in the core file? why only claro?
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.
save result of wrapper.getBoundingClientRect() in a variable, it's used a lot later one. same for table.getBoundingClientRect().
Need to rename this.
Comment #84
bnjmnmI'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
Comment #85
bnjmnmThis 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.
Comment #86
priyanka.sahni CreditAttribution: priyanka.sahni at Srijan | A Material+ Company for Drupal India Association commentedComment #87
priyanka.sahni CreditAttribution: priyanka.sahni at Srijan | A Material+ Company for Drupal India Association commentedVerified 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.
Comment #88
priyanka.sahni CreditAttribution: priyanka.sahni at Srijan | A Material+ Company for Drupal India Association commentedComment #89
KondratievaS CreditAttribution: KondratievaS at Skilld commentedComment #90
bnjmnmThanks 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
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
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.
Comment #91
nod_small changes over 85, don't mind me.
Comment #92
nod_Comment #93
nod_update IS with current state of things, feel free to detail things and pass it around :)
Comment #94
droplet CreditAttribution: droplet commentedthis 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)
Comment #95
bnjmnmThis
class{}
- which we should do regardless but makes #94much easierThis was a major refactor so it'll definitely need another round of manual testing.
Comment #96
nod_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?
Could we do some of that from PHP/twig?
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?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 :)
Comment #97
nod_I would trigger that on document, not window. Any reasons it should be on window?
Comment #98
bnjmnmAddresses #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.Comment #99
nod_no-js version is great, thanks :)
Since we remove #responsive this shouldn't be needed?
We have
Drupal.TableScrolls
andDrupal.TableScroll
in the code, that's a little confusing. Any way to get that into one object in the Drupal namespace?+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.
Comment #100
bnjmnmAddresses #99
Comment #101
nod_code is ok for me.
Seems like the horizontal scrollbar is always displayed on desktop.
Comment #102
bnjmnmGood 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.Comment #103
nod_Looks good to me :)
Comment #104
droplet CreditAttribution: droplet commentedAll debounce is incorrect. It re-creates the instance each time.
Comment #105
nod_right and it doesn't even debounce since the function is called and it's the return of that that is debounced.
Comment #106
nod_fixed the debounce usage. It now debounce across the different events.
We need
.bind(this)
because of how the code is compiled.Comment #107
droplet CreditAttribution: droplet commentedComment #108
lauriiiI 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.
Comment #109
bnjmnmThis addresses both points in #108
Comment #110
nod_Agreed with #108.1, fix looks good too. RTBC on my end, I'll let the final say to twig folks
Comment #111
droplet CreditAttribution: droplet commentedIs 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)
-----------
remove also?
-----------
should be either one
--------------
Drupal.TableScroll.tables = [];
it should be
Drupal.TableScroll.tables = Drupal.TableScroll.tables || [];
or consider to remove it?
Comment #112
bnjmnmRe: #111
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.
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.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.
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.Comment #113
samiullah CreditAttribution: samiullah at Salsa Digital commentedHave 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
Comment #114
lauriiiDiscussed 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.
Comment #115
Anybody@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
Comment #116
bnjmnmMade the followup: #3164041: Improve horizontally scrollable table behavior for non-touch users
Comment #117
nod_Don't need this polyfill no use of assign in the latest version of the code.
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.
Comment #119
fhaeberleComment #120
lauriiiMoving to NW to address #117.
Comment #121
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedTried to address comment #117.
Comment #122
lokeshsahu CreditAttribution: lokeshsahu at OpenSense Labs commentedThe patch has applied correctly but it has not resolved the responsive issue.
Comment #123
bnjmnm@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.
Comment #124
djsagar CreditAttribution: djsagar at OpenSense Labs commentedHere is the same issue on
https://www.drupal.org/project/drupal/issues/3132811#comment-13970382
Patch was created there please review.
Thanks!
Comment #125
hinal05 CreditAttribution: hinal05 as a volunteer and at QED42 for Drupal India Association commented@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.
Comment #126
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commented@hinal05, please fix custom command fails.
Comment #127
djsagar CreditAttribution: djsagar at OpenSense Labs commentedRolling 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!
Comment #128
hinal05 CreditAttribution: hinal05 as a volunteer and at QED42 for Drupal India Association commented@djsagar Thanks for the correction.
Comment #129
Abhijith S CreditAttribution: Abhijith S as a volunteer and at Zyxware Technologies commentedApplied patch #127 and it works fine.Added screenshots.
Before patch;
After patch:
RTBC +1
Comment #132
pragati_kanade CreditAttribution: pragati_kanade at QED42 for Drupal India Association commentedBefore
Comment #133
pragati_kanade CreditAttribution: pragati_kanade at QED42 for Drupal India Association commentedAfter
Comment #134
pragati_kanade CreditAttribution: pragati_kanade at QED42 for Drupal India Association commented#127 Patch Applied successfully. Looks good to me.
Comment #135
bnjmnmThe 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
Comment #137
bnjmnmMerge request open with the logic that addresses #46. Reminder that the css only approach will not work. Due to sticky headers.
Comment #138
Madhu kumar CreditAttribution: Madhu kumar as a volunteer and at Zyxware Technologies commentedPatch #127 applied cleanly and working as expected, sharing screenshot for reference.
Comment #139
bnjmnmRe #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 🙂)
Comment #140
bnjmnmComment #141
nod_fixed the leftover from #117 and updated for branch 9.3.x
Comment #142
nod_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 :)
Comment #143
mherchelThere’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 toposition: fixed
.We need a followup issue.
More comments incoming.
Comment #144
nod_Probably related to #2958478: Toolbar height calculation is faulty in multiple cases and #2516938: Set the toolbar to position fixed on mobile
Happens when viewport is below 600-ish px width
Comment #145
mherchelThis 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.Comment #146
lauriiiPosted 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.
Comment #147
vikashsoni CreditAttribution: vikashsoni as a volunteer and at Zyxware Technologies commentedApplied patch #127 applied successfully looks good for me
for ref sharing screenshot ....
Comment #148
bnjmnmRemoving credit for #147 because:
Comment #151
andy-blumRe-queued the tests, the failures don't seem like anything really related to this issue.
Comment #152
xjmJust 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:
It gets displayed all the time, even in passing tests, so is unrelated. Your tests are failing much earlier than that.
Comment #153
xjmThe MR also does not apply:
Comment #154
andy-blum@xjm just saw your comment about rebasing after already merging - sorry.
Comment #156
xjmOK, 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. 😂 🤞
Comment #157
mherchelI 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
Comment #158
lauriii#157: Isn't that because we are setting
overflow-y: hidden;
? Is there no way for us to change that tooverflow-y: visible;
without introducing a vertical scroll?Comment #159
bnjmnm#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.
Comment #160
mherchelThe 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.
Comment #161
ckrinaWe’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.Comment #163
ckrinaComment #164
andy-blumremoving stable blocker tag per #161
Comment #166
Anybody