Add support for tables with two headers in views table display [#2770835] | Drupal.org

Add support for tables with two headers in views table display [#2770835] | Drupal.org
Skip to search
Can we use first and third party cookies and web beacons to
understand our audience, and to tailor promotions you see
Add support for tables with two headers in views table display
Needs work
Project:
Drupal core
Version:
main
Component:
views.module
Priority:
Normal
Category:
Feature request
Assigned:
Unassigned
Issue tags:
Usability
wcag111
Needs accessibility review
Needs Review Queue Initiative
about tags
Usability
Makes Drupal
easier to use
. Preferred over
UX
D7UX
, etc.
Needs accessibility review
Used to alert the accessibility topic maintainer(s) that an issue significantly affects (or has the potential to affect) the accessibility of Drupal, and their signoff is needed (see
the governance policy draft
for more information).
Useful links:
Drupal's accessibility standards
the Drupal Core accessibility gate
Needs Review Queue Initiative
Used to track the progress of issues reviewed by the
Drupal Needs Review Queue Initiative
Reporter:
gettysburger
Created:
21 Jul 2016 at 18:35 UTC
Updated:
31 May 2024 at 22:16 UTC
Jump to comment:
Most recent
Most recent file
Problem/Motivation
For accessibility, some tables call for having both column headers and row headers. The
WAI table concepts tutorial
describes a number of table models, with corresponding example use cases.
The tables currently generated by views are all
tables with one header
. These are quite acceptable for many cases, including many of the administrative views in Drupal core.
This feature request aims to support simple
tables with two headers
, with accessible markup. Such tables have

in the

, and a

as a header inside each

row, as appropriate. This is primarily intended as a feature for site builders, though future administration tables provided by by core or contrib modules may also benefit from it.
Recently
#1831162: Tables generated by Views need better semantics.
added the
scope="col"
for column header elements, but did not add any support for row headers.
Proposed resolution
A configuration option that allows the site builder to set
any
column in a table to be treated as a table header for that row. The site builder will also have the option not to have any row header, producing only column headers. Note that this approach is flexible enough to support
tables with an offset column of header cells
Note: supporting
tables with irregular headers
or
tables with multi-level headers
is NOT in scope for this issue. If
#893530: Multi level (multirow) header for table FAPI element
gets into core, we can create separate feature request issues for these.
Table format settings showing new Row Header column. Title field has been selected:
Content list view output with title field selected as the row header (Seven Theme):
Note: the column header styles apply to the row header field on hover (see comment #21)
Content list view output with title field selected as the row header (Bartik Theme):
Selecting a field as the row header causes that field to be rendered as a

tag inside the table row:
Remaining tasks
DONE. Usability review: do we use a column of radio buttons or a drop-down select? See comment #33, @yoroy has a preference for a column of radio buttons, but it doesn't seem like a strong preference either way.
DONE. Add a setting to the Views table display plugin config form.
DONE. Update markup in output.
DONE. Review design impact for Bartik, styling of row-headers. See comments #22 and #33, @yoroy is happy.
DONE. Review design impact for Seven, styling of row-headers. See comments #21 and #33, @yoroy's comment could do with clarifying. This change also impacts other themes that ship with core; Bartik, Seven, Classy, Olivero, Claro
DONE. Write tests. Rough plan outlined in comment #65. @lendude added tests in #65 and subsequent rerolls adopted this test.
TODO: Update existing views which use the table display plugin, to include the new config option (set to none). See comment #67.
DONE: A change record with advice for themers:
If they already have a custom
views.view-table.html.twig
template, it will need updated to benefit from this feature.
Turning on the new row-level headers option may have an unexpected effect on how tables look, if their theme doesn't already anticipate row-level table headers.
User interface changes
An additional configuration in the views style format. Either of these:
An extra column of radio buttons in the settings table, similar to the default-sort column (screenshot in comment #16)
A drop-down select, which has a description (screenshot in comment #30)
(Possibly) a different appearance for table headers on each row (if design review permits it).
We do not propose to change the configuration of any existing admin views tables in core as yet.
API changes
None.
Data model changes
Additional "row header" option in Views table display config, similar to the existing default-sort option.
Original report by @gettysburger
Accessible tables have "scope=col" in the headers and "scope=row" in the first field in a row. The first field in the row should also have a

instead of a

tag. Tables generated by Views have scope=col" but not scope ="row". A recent issue (
) added the "scope=col" but did not add "scope=row".
I proposed that "scope=row" be added to the first field in each table row and that first field in the row should also have a

instead of a

tag.
Comment
File
Size
Author
#134
interdiff_133_134.txt
774 bytes
carlygerard
#134
2770835-134.patch
14.6 KB
carlygerard
#133
interdiff_127-133.txt
6.31 KB
carlygerard
#133
2770835-133.patch
14.62 KB
carlygerard
#132
interdiff_127-132.txt
3.61 KB
carlygerard
#132
2770835-132.patch
14.57 KB
carlygerard
#131
interdiff_127-130.txt
3.61 KB
carlygerard
#131
2770835-130.patch
14.45 KB
carlygerard
#127
2770835-127.patch
13.3 KB
themusician
#126
2770835-126.patch
13.29 KB
themusician
#122
2770835-122.patch
14.43 KB
pooja saraah
#119
2770835-119.patch
14.55 KB
themusician
#119
reroll_diff_115-119.txt
10.87 KB
themusician
#117
img-before-patch(1).PNG
53.96 KB
Charchil Khandelwal
#117
img-before-patch(2).PNG
31.09 KB
Charchil Khandelwal
#117
img-after-patch(1).PNG
52.44 KB
Charchil Khandelwal
#117
img-after-patch(2).PNG
32.3 KB
Charchil Khandelwal
#115
interdiff_2770835_109-115.txt
4.26 KB
andregp
#115
2770835-115.patch
24.12 KB
andregp
#109
2770835-109.patch
29.17 KB
carlygerard
#108
2770835-108.patch
30.85 KB
carlygerard
#107
2770835-107.patch
56.74 KB
carlygerard
#106
2770835-101-9.3.x.patch
23.85 KB
carlygerard
#105
interdiff_95-102.txt
596 bytes
carlygerard
#101
2770835-101.patch
24.24 KB
carlygerard
#100
2770835-100.patch
21.92 KB
themusician
#99
2770835--after--patch--pic.png
68 bytes
vikashsoni
#99
2770835--before--patch--pic.png
68 bytes
vikashsoni
#98
2770835-98-9.4.x.patch
20.71 KB
carlygerard
#98
2770835-98-9.4.x.patch
20.71 KB
carlygerard
#97
2770835-97.patch
17.56 KB
ranjith_kumar_k_u
#96
2770835-96.patch
17.56 KB
ranjith_kumar_k_u
#95
2770835-95.patch
25.38 KB
carlygerard
#94
2770835-92.patch
16.05 KB
carlygerard
#91
2770835-th-markup.png
274.84 KB
carlygerard
#91
2770835-table-settings.png
141.32 KB
carlygerard
#90
2770835-90.patch
35.14 KB
themusician
#89
2770835-89.patch
33.8 KB
themusician
#88
2770835-88.patch
21.24 KB
themusician
#87
2770835-87.patch
10.46 KB
themusician
#74
seven-th-background-hover.png
92.51 KB
amklose
#74
seven-th-background.png
219 KB
amklose
#73
views-content-title_row_header_hover-seven-DOM.png
31.49 KB
amklose
#73
views-table_format-row_header_column-title_selected.png
49.34 KB
amklose
#73
views-content-title_row_header_hover-seven.png
28.48 KB
amklose
#73
views-content-title_row_header_hover-bartik.png
22.09 KB
amklose
#72
2770835-72.patch
9.84 KB
lendude
#72
interdiff-2770835-63-72.txt
4.2 KB
lendude
#66
interdiff_58-63.txt
1.17 KB
themusician
#63
2770835-63.patch
8.24 KB
themusician
#12
views_8_accessible_row_headers-2770835-12.patch
8.47 KB
miwayha
#16
Screen Shot 2016-09-07 at 9.34.24 PM.png
235.58 KB
miwayha
#18
tables-with-two-headers-2770836-CONFIG-AFTER.png
175.51 KB
andrewmacpherson
#19
tables-with-two-headers-2770835-MARKUP-AFTER.png
13.35 KB
andrewmacpherson
#21
tables-with-two-headers-2770837-SEVEN-BEFORE.png
100.34 KB
andrewmacpherson
#21
tables-with-two-headers-2770835-SEVEN-AFTER.png
101.11 KB
andrewmacpherson
#22
tables-two-headers-2770835-BARTK-AFTER.png
98.93 KB
andrewmacpherson
#30
tables-two-headers-2770835-select-options.png
132.59 KB
andrewmacpherson
#37
add_support_for_tables-2770835-37.patch
10.23 KB
scuba_fly
#39
add_support_for_tables-2770835-38.patch
10.34 KB
scuba_fly
#41
add_support_for_tables-2770835-41.patch
10.18 KB
manuel garcia
#41
interdiff.txt
791 bytes
manuel garcia
#47
2770835-47.patch
10.03 KB
manuel garcia
#53
interdiff-2770835-47-53.txt
1.78 KB
manuel garcia
#53
2770835-53.patch
8.25 KB
manuel garcia
#56
New view table setting.png
346.64 KB
themusician
#56
th added on output.png
53.04 KB
themusician
#58
2770835-58.patch
8.25 KB
themusician
#59
interdiff_53_58.txt
1.19 KB
themusician
Issue fork
drupal-2770835
Show commands
Start within a Git clone of the project using the
version control instructions
Add & fetch this issue fork’s repository
Or,
if you do not have
SSH keys set up on git.drupalcode.org
Add & fetch this issue fork’s repository
table-with-two-header-support
plain diff
MR
!3114
Check out this branch for the first time
Check out existing branch, if you already have it locally
About issue forks
Comments
Comment
#1
21 July 2016 at 18:35
gettysburger
created an issue. See
original summary
or
to post comments
Comment
#2
gettysburger
commented
21 July 2016 at 18:36
Issue summary:
View changes
or
to post comments
Comment
#3
gettysburger
commented
21 July 2016 at 18:37
or
to post comments
Comment
#4
gettysburger
commented
21 July 2016 at 19:36
Issue summary:
View changes
or
to post comments
Comment
#5
mgifford
he/him
commented
21 July 2016 at 20:19
Issue tags:
govcon2016
or
to post comments
Comment
#6
andrewmacpherson
commented
1 August 2016 at 06:50
Issue summary:
View changes
tidying up the issue summary
or
to post comments
Comment
#7
dawehner
German
commented
1 August 2016 at 07:04
Issue tags:
+php-novice
\template_preprocess_views_view_table
would be probably the right place to add it.
or
to post comments
Comment
#8
andrewmacpherson
commented
1 August 2016 at 08:05
Title:
Tables generated by Views should have scope=row for improved accessibility
» Add support for tables with two headers in views table display
Category:
Bug report
» Feature request
The tables currently generated by views are all
tables with one header
This isn't really a bug in my view, in that I don't think we should force authors to use a two-header table model. Looking though the
WAI table concepts tutorial
, it's clear that tables with one header are quite acceptable for many cases, including many of the administrative views in Drupal core.
Instead, let's treat this as a feature request. It would be really great if views could support some of the other table concepts described there!
I think it's feasible to support simple
tables with two headers
. The table display plugin could have an extra setting to say that a particular field is treated as a row header, and
scope="row"
could be added via the
column.attributes
twig variable (I think). This would be flexible enough to support
tables with an offset column of header cells
. I'm not sure what to do about the top-left cell, which might need to be blank.
Support for some
tables with irregular headers
might be possible, perhaps using views query grouping, but would add more configuration options. This might be best explored as a separate views display plugin provided by a contrib module.
or
to post comments
Comment
#9
1 August 2016 at 08:05
Version:
8.2.x-dev
» 8.3.x-dev
Drupal 8.2.0-beta1
was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the
Drupal 8 minor version schedule
and the
Allowed changes during the Drupal 8 release cycle
or
to post comments
Comment
#10
mgifford
he/him
commented
18 August 2016 at 19:21
First, I'd like to agree what Andrew said & disagree with this "Accessible tables have scope="col" in the headers and scope="row" in the first field in a row." It all depends on the table and the data you are trying to display. Right now you can't define the scope="row" which is a problem if have a heading there you want to use.
I'm fine with looking ahead at how to make the Views tables more complex. There is a lot of tabular data that I don't think we can very effectively show right now.
Right now we generally have the first row being headings and all the other cells being data cells. I don't know of an instance where the first field in each table row is actually a heading.
Sometimes it is a checkbox. I guess if it is a first name or title of an article that could be considered a heading.
There are great examples of accessible table structures here
I just don't know how we would get to two meaningful headers in Views table display so that it reflects the needs of
or
to post comments
Comment
#11
miwayha
commented
31 August 2016 at 00:37
I don't see where gettysburger uploaded a patch for this on 8.
I created a sandbox project for this issue on 7:
Views Table Row Headers
. I'm reasonably experienced site builder, but I'm very new to drupal development. If someone would be willing to mentor me either through releasing this as a module and porting it to 8 or making the change to core in 8, I would be very grateful.
or
to post comments
Comment
#12
miwayha
commented
6 September 2016 at 00:11
Status:
Active
» Needs review
Status
File
Size
new
views_8_accessible_row_headers-2770835-12.patch
8.47 KB
Attempting a patch for d8.
or
to post comments
Comment
#13
miwayha
commented
6 September 2016 at 00:22
Issue summary:
View changes
Cleaned up issue summary.
or
to post comments
Comment
#14
mgifford
he/him
commented
7 September 2016 at 20:38
Can I configure this through Views UI? Trying to understand what the current code does and how to see it in action.
or
to post comments
Comment
#15
miwayha
commented
7 September 2016 at 21:35
Yes! View the format settings after setting format to "table". There should be an added column of radio buttons with the heading "row header" in the table that has the field config.
or
to post comments
Comment
#16
miwayha
commented
8 September 2016 at 01:36
Status
File
Size
new
Screen Shot 2016-09-07 at 9.34.24 PM.png
235.58 KB
Uploading screen shot of new interface...
or
to post comments
Comment
#17
andrewmacpherson
commented
8 September 2016 at 08:57
Thanks @miwayha - this is very exciting progress! I'll review it more closely soon.
or
to post comments
Comment
#18
andrewmacpherson
commented
8 September 2016 at 23:57
Issue summary:
View changes
Status
File
Size
new
tables-with-two-headers-2770836-CONFIG-AFTER.png
175.51 KB
The new radio buttons for row header follows the same pattern as the default-sort configuration.
It has a "none" option which is great - it means a site builder can change their mind and go back to a single-header table if they wish. (In fact I did so several times, just to get some screenshots...)
or
to post comments
Comment
#19
andrewmacpherson
commented
8 September 2016 at 22:53
Issue summary:
View changes
Status
File
Size
new
tables-with-two-headers-2770835-MARKUP-AFTER.png
13.35 KB
The markup is what we expect. I modified the core /admin/content view so that the node "Title" is treated as a row header. Here's a screenshot of the markup:
The cell for the node title is a
th
element with a
scope="row"
attribute.
or
to post comments
Comment
#20
andrewmacpherson
commented
8 September 2016 at 23:36
Issue summary:
View changes
Related issues:
#1831162: Tables generated by Views need better semantics.
Updating issue summary to reflect the new plan as a feature request:
Outline the plan for optional row headers, re-using bits of comment #8
Storing the new row header option counts as a data model change.
Adding @gettyburger's original report.
or
to post comments
Comment
#21
andrewmacpherson
commented
8 September 2016 at 23:48
Issue summary:
View changes
Issue tags:
Seven
, +
Needs design review
Status
File
Size
new
tables-with-two-headers-2770837-SEVEN-BEFORE.png
100.34 KB
new
tables-with-two-headers-2770835-SEVEN-AFTER.png
101.11 KB
In the Seven theme, using the patch from #12, row headers inherit some different styling similar to the column headers.
The text of a row header is bold (I like this).
The row headers get the same hover effect as column headers, with a prominent underline (I think this looks weird).
For these screenshots I modified the core
admin/content
listing to output the node title as a row header. The mouse pointer is not incliuded in the screenshots, but they show the hover style on the second row.
Before patch #12, just column header:
After patch #12, treating node title as a row header:
We'll need design review for this, tagging for Seven maintainer too.
or
to post comments
Comment
#22
andrewmacpherson
commented
8 September 2016 at 23:54
Issue summary:
View changes
Issue tags:
Bartik
Status
File
Size
new
tables-two-headers-2770835-BARTK-AFTER.png
98.93 KB
For completeness, here is how row headers look in Bartik.
Like the previous screenshot for Seven theme, this shows the
admin/content
listing, modified so node title is a row header. Then I set Bartik as the admin theme.
I like this, it's very clear, and matches the column headers perfectly.
Nonetheless, it needs design review sign-off. Tagging for Bartik maintainer.
or
to post comments
Comment
#23
andrewmacpherson
commented
8 September 2016 at 23:56
Note: these screenshots also demonstrate that the proposed solution is flexible enough to support
tables with an offset column of header cells
I am very excited about this new feature!
or
to post comments
Comment
#24
andrewmacpherson
commented
8 September 2016 at 23:58
Issue summary:
View changes
or
to post comments
Comment
#25
andrewmacpherson
commented
9 September 2016 at 00:02
We still need a code review of the patch from #12
or
to post comments
Comment
#26
dawehner
German
commented
9 September 2016 at 07:31
Issue tags:
Needs usability review
I'm wondering whether we should move "row header" further to the right on the views admin screen. It seems to be for me that "sortable" might be more often changed for example. IMHO we need some better way to communicate what this feature does to the user.
or
to post comments
Comment
#27
andrewmacpherson
commented
9 September 2016 at 07:44
we need some better way to communicate what this feature does to the user.
We could take the row-header options out of the table, and present them as a drop-down select, the way the grouping option is shown. Then it could have a #description of its own, which the options in the table don't have.
or
to post comments
Comment
#28
dawehner
German
commented
9 September 2016 at 07:56
That might be indeed a good idea. Let's see whether someone from the UX team can help us with that.
or
to post comments
Comment
#29
miwayha
commented
9 September 2016 at 12:45
My sandbox module for D7 uses a dropdown select, if you'd like to test with that rather than coding something from scratch.
or
to post comments
Comment
#30
andrewmacpherson
commented
15 September 2016 at 18:30
Status
File
Size
new
tables-two-headers-2770835-select-options.png
132.59 KB
Here's a screenshot from @miwayha's D7 sandbox module.
It shows a drop-down select for choosing the row header field, as described in #27.
If we choose this rather than radio buttons in the table, we'll need to decide what weight to give it in relation to the other settings.
or
to post comments
Comment
#31
andrewmacpherson
commented
15 September 2016 at 18:38
Issue summary:
View changes
For the usability review, we're comparing two ideas for the setting widget:
a column of radio buttons (screenshot in comment 16)
a drop-down select with a description (screenshot in comment #30)
For the design review, we're looking at the screenshot in comments #21-22.
or
to post comments
Comment
#32
andrewmacpherson
commented
15 September 2016 at 18:43
Issue summary:
View changes
Updating tasks
or
to post comments
Comment
#33
yoroy
commented
30 September 2016 at 09:42
Issue tags:
Needs design review
, -
Needs usability review
Usability
A select list on a separate config modal is less clear than the column of radio buttons on the table itself. But the config modal gives us a little more room to explain the feature. Could we maybe make the "None" option explain what this is all about?
I think I prefer the radio buttons on the table. But: it's another column. Could we add a setting on the config modal to enable multiple header selection in the first place?
I think Bartik looks fine. I'm not the maintainer, but I think this is what it should be.
For Seven we should apply the same styling as is used along the top.
or
to post comments
Comment
#34
andrewmacpherson
commented
30 September 2016 at 11:39
Issue tags:
Dublin2016
or
to post comments
Comment
#35
scuba_fly
Netherlands, Vinkeveen
commented
30 September 2016 at 11:40
Assigned:
Unassigned
scuba_fly
I'm looking into this at the DrupalCon Dublin 2016
or
to post comments
Comment
#36
miwayha
commented
30 September 2016 at 12:41
Excited to see what comes of this in Dublin. I'll keep an eye on this issue, and I'll be happy to contribute any other patches to it if need be.
Thanks so much for doing a ton of work on this, @andrewmacpherson and others!
or
to post comments
Comment
#37
scuba_fly
Netherlands, Vinkeveen
commented
30 September 2016 at 14:15
Assigned:
scuba_fly
» Unassigned
Status
File
Size
new
add_support_for_tables-2770835-37.patch
10.23 KB
I talked to yoroy and moving the column more to the right should be enough for now.
In the future it would be nice to be able to hide elements in the form because it gets a little full in the table.
I'll add a interdiff in a moment.
or
to post comments
Comment
#38
30 September 2016 at 14:17
Status:
Needs review
» Needs work
The last submitted patch,
37: add_support_for_tables-2770835-37.patch
, failed testing.
or
to post comments
Comment
#39
scuba_fly
Netherlands, Vinkeveen
commented
30 September 2016 at 14:29
Status
File
Size
new
add_support_for_tables-2770835-38.patch
10.34 KB
whoops made the patch from the wrong directory.
Here is a new patch with the core path included.
or
to post comments
Comment
#40
scuba_fly
Netherlands, Vinkeveen
commented
30 September 2016 at 14:31
Status:
Needs work
» Needs review
1 file was hidden/shown/deleted
Status
File
Size
hidden
add_support_for_tables-2770835-37.patch
10.23 KB
or
to post comments
Comment
#41
manuel garcia
commented
1 October 2016 at 11:09
Status
File
Size
new
add_support_for_tables-2770835-41.patch
10.18 KB
new
interdiff.txt
791 bytes
Just reworking a bit an if/else block so its easier to follow
or
to post comments
Comment
#42
miwayha
commented
27 October 2016 at 12:07
Issue tags:
Needs usability review
or
to post comments
Comment
#43
Bojhan
commented
4 November 2016 at 04:06
Issue tags:
Needs usability review
Not sure what to review, yoroy already looked at this.
or
to post comments
Comment
#44
4 November 2016 at 04:06
Version:
8.3.x-dev
» 8.4.x-dev
Drupal 8.3.0-alpha1
will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the
Drupal 8 minor version schedule
and the
Allowed changes during the Drupal 8 release cycle
or
to post comments
Comment
#45
delacosta456
commented
5 May 2017 at 17:52
hi
it would have been nice if somebody could help us please with a patch for irregular headers in D7..
thanks
or
to post comments
Comment
#46
manuel garcia
commented
8 May 2017 at 09:17
Assigned:
Unassigned
manuel garcia
Status:
Needs review
» Needs work
Patch not applying anymore, rerolling
or
to post comments
Comment
#47
manuel garcia
commented
8 May 2017 at 09:34
Assigned:
manuel garcia
» Unassigned
Status:
Needs work
» Needs review
Status
File
Size
new
2770835-47.patch
10.03 KB
Had to manually fix these two files:
core/modules/views_ui/views_ui.theme.inc
core/modules/views/src/Plugin/views/style/Table.php
Conflicts were due to the switch to short array syntax in core, so rerolled appropriately.
or
to post comments
Comment
#48
8 May 2017 at 10:29
Status:
Needs review
» Needs work
The last submitted patch,
47: 2770835-47.patch
, failed testing.
or
to post comments
Comment
#49
8 May 2017 at 10:29
Version:
8.4.x-dev
» 8.5.x-dev
Drupal 8.4.0-alpha1
will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the
Drupal 8 minor version schedule
and the
Allowed changes during the Drupal 8 release cycle
or
to post comments
Comment
#50
ivan berezhnov
commented
5 January 2018 at 18:44
Issue tags:
CSKyiv18
or
to post comments
Comment
#51
5 January 2018 at 18:44
Version:
8.5.x-dev
» 8.6.x-dev
Drupal 8.5.0-alpha1
will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the
Drupal 8 minor version schedule
and the
Allowed changes during the Drupal 8 release cycle
or
to post comments
Comment
#52
john.lee.doe
commented
22 March 2018 at 03:26
for my project , it is great full.
or
to post comments
Comment
#53
manuel garcia
commented
22 March 2018 at 08:48
Status:
Needs work
» Needs review
Status
File
Size
new
interdiff-2770835-47-53.txt
1.78 KB
new
2770835-53.patch
8.25 KB
Just a patch cleanup:
--- a/core/modules/views/src/Plugin/views/PluginBase.php
+++ b/core/modules/views/src/Plugin/views/PluginBase.php

@@ -192,26 +192,6 @@ public function filterByDefinedOptions(array &$storage) {
- * Do the work to filter out stored options depending on the defined options.
- *
- * @param array $storage
- * The stored options.
- * @param array $options
- * The defined options.
- */
- protected function doFilterByDefinedOptions(array &$storage, array $options) {
- foreach ($storage as $key => $sub_storage) {
- if (!isset($options[$key])) {
- unset($storage[$key]);
- }
- if (isset($options[$key]['contains'])) {
- $this->doFilterByDefinedOptions($storage[$key], $options[$key]['contains']);
- }
- }
- }
- /**
* {@inheritdoc}
*/
public function unpackOptions(&$storage, $options, $definition = NULL, $all = TRUE, $check = TRUE) {
@@ -258,6 +238,26 @@ public function destroy() {

@@ -258,6 +238,26 @@ public function destroy() {

/**
+ * Do the work to filter out stored options depending on the defined options.
+ *
+ * @param array $storage
+ * The stored options.
+ * @param array $options
+ * The defined options.
+ */
+ protected function doFilterByDefinedOptions(array &$storage, array $options) {
+ foreach ($storage as $key => $sub_storage) {
+ if (!isset($options[$key])) {
+ unset($storage[$key]);
+ }
+ if (isset($options[$key]['contains'])) {
+ $this->doFilterByDefinedOptions($storage[$key], $options[$key]['contains']);
+ }
+ }
+ }
+ /**
No reason to move this method around that I know of, so undoing this from this patch to make it easier to review.
or
to post comments
Comment
#54
john.lee.doe
commented
26 March 2018 at 02:41
How to integrate with
multi level header rows
in this view table.
or
to post comments
Comment
#55
andrewmacpherson
commented
9 April 2018 at 19:31
Issue summary:
View changes
re: #54 - that's out of scope for this issue, but it's already mentioned in the issue summary as an idea for future work.
Thanks for mentioning the
#893530: Multi level (multirow) header for table FAPI element
here, though. I've linked to it in the issue summary here.
For views to produce multi-level headers, I think we'll have to wait until
#893530: Multi level (multirow) header for table FAPI element
is done first. Then we can open up a new issue to look at ways of letting a site-builder produce them with Views. Maybe we can extend the existing table views display in core, or it might need a separate views display plugin.
or
to post comments
Comment
#56
themusician
commented
10 April 2018 at 17:33
Status:
Needs review
» Reviewed & tested by the community
Status
File
Size
new
New view table setting.png
346.64 KB
new
th added on output.png
53.04 KB
The patch as re-rolled applies cleanly. The formatting appears to check out as well.
In this image you can see the new table setting appears when editing a table view. This is a clone of the watchdog view.
Then, the
output is appropriate as well when the view renders after selecting the row header for the type field.
or
to post comments
Comment
#57
star-szr
he/him
commented
10 April 2018 at 19:48
Status:
Reviewed & tested by the community
» Needs review
Sorry just drive-by Twig standards stuff for now.
+++ b/core/themes/classy/templates/views/views-view-table.html.twig
@@ -106,7 +107,11 @@
+ {% if (key) == (row_header) %}

@@ -118,7 +123,11 @@
+ {% if (key) == (row_header) %}
Are the parens needed?
+++ b/core/themes/classy/templates/views/views-view-table.html.twig
@@ -106,7 +107,11 @@
+
...
+
In both these cases, there should be no space before we print the attributes.
or
to post comments
Comment
#58
themusician
commented
10 April 2018 at 20:47
Status
File
Size
new
2770835-58.patch
8.25 KB
Thanks for the quick look @cottser.
It seems like the parenthes are not needed. It seems to work properly at least without them.
The spaces are removed prior to the attributes being printed.
Interdiff also attached.
or
to post comments
Comment
#59
themusician
commented
10 April 2018 at 20:47
Status
File
Size
new
interdiff_53_58.txt
1.19 KB
or
to post comments
Comment
#60
andrewmacpherson
commented
10 April 2018 at 22:26
@theMusician - thanks for working on this.
Review of patch #58:
The parentheses were removed from the template in Classy, but they're still present in the template in Views module.
--- a/core/modules/views/templates/views-view-table.html.twig
+++ b/core/modules/views/templates/views-view-table.html.twig
@@ -71,7 +72,7 @@
%}
{% endif %}
-
+
Unnecessary whitespace changes. I tested with Stark, and it has no effect on the output. I checked our
Twig coding standards
about this - there's no explicit opinion, but the examples don't have whitespace between element name and curly brace.
--- a/core/modules/views/templates/views-view-table.html.twig
+++ b/core/modules/views/templates/views-view-table.html.twig
@@ -94,7 +95,7 @@
{% endif %}

{% for row in rows %}
-
+
Same as previous point.
or
to post comments
Comment
#61
andrewmacpherson
commented
10 April 2018 at 22:27
Issue tags:
Needs tests
We'll need functional tests to check the output of the new option.
or
to post comments
Comment
#62
andrewmacpherson
commented
10 April 2018 at 22:45
@cottser - Where do we stand with Stable and Classy themes here? We're adding a new UI option which won't work unless they have the right template. The current patch changes templates in Views module and Classy theme, but what about sites using
base theme: false
and have the Stable template?
I think there's low risk to existing websites, because they won't have row-level headers configured for table views. Until a site-builder makes use of the new option, the output will be unaffected.
TODO:
Do we need to update existing views which use the table display plugin, to include the new config option (set to none)?
A change record with advice for themers.
If they already have a custom
views.view-table.html.twig
template, it will need updated to benefit from this feature.
Turning the option on may have an unexpected effect on how tables look, if their theme doesn't already anticipate row-level table headers.
or
to post comments
Comment
#63
themusician
commented
10 April 2018 at 23:39
Status
File
Size
new
2770835-63.patch
8.24 KB
I reviewed the twig coding standards as well and agree, nothing is explicit but the samples do not have a space.
Attached is a patch that includes the update to the standard view twig template as well.
or
to post comments
Comment
#64
themusician
commented
11 April 2018 at 00:40
Regarding tests, as views has a lot of them. Where would one start?
It seems that any test file that includes an option for "sortable" would need to have "row_header" added as well.
From there, what other test files need to either be edited or created?
This doc helps for a simple module,
, but the amount of tests in views has me a little perplexed.
or
to post comments
Comment
#65
andrewmacpherson
commented
12 April 2018 at 21:00
Issue tags:
Nashville2018
Can we get a 58-63 interdiff?
This one is really close, tests are the last part needed.
@theMusician - this is probably going to be functional tests rather than unit tests. Look at tests inside
core/modules/views/tests/src/Functional/
to find some. They typically make use of a test module inside
tests/src/modules
, and the test modules will have installation config like any other module. In views case, the
views_test_config
module has lots of specimen views.
Maybe we can add an extra test method to
core/modules/views/tests/src/Functional/Plugin/StyleTableTest.php
. The test process will be something like:
set up a test view which is configured with row-level headers
get the HTML output
assert the expected

is there for the chosen column,
assert it's just a

for the other columns,
assert there's only one column which uses row-level headers
... etc.
Also assert a table display with row-level headers configured as "none" does not have an TH inside the table body.
or
to post comments
Comment
#66
themusician
commented
13 April 2018 at 01:41
Status
File
Size
new
interdiff_58-63.txt
1.17 KB
Thanks @andrewmacpherson for the guidance on the tests.
The interdiff is attached.
Interdiff 58-63
If anyone is sprinting at DrupalCon and wants to tackle the tests please dive in!
or
to post comments
Comment
#67
lendude
Dutch
Amsterdam
commented
13 April 2018 at 06:07
Since we adding a new option we should really have an upgrade path for this to add the option to existing views. One problem with adding new settings to the table style is that existing settings are probably not up to date anyway, see
#2917814: Views table style info never gets updated when changing/adding fields
or
to post comments
Comment
#68
lendude
Dutch
Amsterdam
commented
13 April 2018 at 06:23
Nitpicks:
+++ b/core/modules/views/src/Plugin/views/style/Table.php
@@ -326,6 +332,23 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
+ // Because 'radio' doesn't fully support '#id' =(
This could be a more complete sentence explaining what's going on and why, and is there an open issue for that?
"We need to set the id using attributes because 'radio' doesn't fully support #id."
+++ b/core/modules/views/src/Plugin/views/style/Table.php
@@ -396,6 +419,17 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
+ // Provide a radio for no row header
Missing a trailing period.
+++ b/core/modules/views_ui/views_ui.theme.inc
@@ -462,13 +467,20 @@ function template_preprocess_views_ui_style_plugin_table(&$variables) {
+ array('data' => t('None'), 'colspan' => 2),
+ array('colspan' => 4),
+ array('align' => 'center', 'data' => $form['default'][-1]),
+ array('align' => 'center', 'data' => $form['row_header'][-1]),
+ array('colspan' => 2),
old skool array() notation needs to be []
+++ b/core/themes/classy/templates/views/views-view-table.html.twig
@@ -26,6 +26,7 @@
+ * - row_header: The field that should be marked up as a table header on each row.
> 80 characters
+++ b/core/modules/views/templates/views-view-table.html.twig
@@ -26,6 +26,7 @@
+ * - row_header: The field that should be marked up as a table header on each row.
> 80 characters
or
to post comments
Comment
#69
andrewmacpherson
commented
13 April 2018 at 07:47
Issue summary:
View changes
@lendude
Thanks for the related issue. DO you think it'll be a blocker for this one? I mentioned the upgrade path in #62, but didn't update the issue summary. I'll do that now.
I've reviewed the comments going all the way back, and updated the tasks in the issue summary to says what's been done.
One thing that isn't quite clear: are we happy about how row-level headers look in Seven? There's a screensnot in #21.
Later in #33 @yoroy says "For Seven we should apply the same styling as is used along the top", but I don't think we've acted on this. I think it means the row-level headers should get the beige background colour. I also think the link underlines look odd, in the screenshot example which made the node title links as the row headers.
Let's seek a design review again for Seven.
or
to post comments
Comment
#70
andrewmacpherson
commented
13 April 2018 at 07:59
Issue tags:
Novice
, +
Needs screenshots
Let's get some screenshots set up using the latest patch, and add them to the issue summary.
Here's a good novice task for the Nashville sprints...
Screenshot of the views table display settings. We want to show where the new setting is in the Views UI.
Update the "Content" view which powers the
admin/content
listing, make the node title field act as a row-level table header.
Then get a screenshot of the view, at
admin/content
. Make sure the mouse pointer is hovering over a node title, we want to confirm if the broad link underline from screenshot #21 is still an issue.
Switch the admin them to Bartik, and get a screenshot of
admin/content
, mouse over a node title.
Update the issue summary, put screenshots in proposed resolution.
Bonus level: find a member of the usability or design team, or a product manager, and get an opinion how it looks in Seven to clarify comment #33.
or
to post comments
Comment
#71
amklose
he/him
Wisconsin
commented
13 April 2018 at 15:35
I'll work on getting those screenshots (here at Drupalcon Nashville)
or
to post comments
Comment
#72
lendude
Dutch
Amsterdam
commented
13 April 2018 at 17:28
Status
File
Size
new
interdiff-2770835-63-72.txt
4.2 KB
new
2770835-72.patch
9.84 KB
DO you think it'll be a blocker for this one?
@andrewmacpherson probably not since we'd only add a setting that is not tied to the fields that exist on the view (well the default setting isn't anyway...), so I think we can workaround this.
Addressed my own nits.
Added the needed config schema changes and the most minimal of tests. If anybody wants to turn this into a proper test, go for it!
or
to post comments
Comment
#73
amklose
he/him
Wisconsin
commented
13 April 2018 at 19:30
Issue summary:
View changes
Status
File
Size
new
views-content-title_row_header_hover-bartik.png
22.09 KB
new
views-content-title_row_header_hover-seven.png
28.48 KB
new
views-table_format-row_header_column-title_selected.png
49.34 KB
new
views-content-title_row_header_hover-seven-DOM.png
31.49 KB
Added some screenshots to the proposed resolution section of the issue summary. I verified that the styling pointed out in #21 still applies for Seven and Bartik.
or
to post comments
Comment
#74
amklose
he/him
Wisconsin
commented
13 April 2018 at 20:59
Status
File
Size
new
seven-th-background.png
219 KB
new
seven-th-background-hover.png
92.51 KB
Regarding the bonus level task in #70, I spoke with Benji Fisher and dug into the CSS a little bit. It looks like the

tag background styles are specific to the

section in the Seven theme, but the link underline style is not that specific. I changed the CSS selector in my browser inspector so that the gray background was applied to all

elements but Benji and I agreed that this looked a little strange. See the following screenshot.
With Hover:
By making the styles more generic, it includes the following CSS on all

elements:
th {
background: #f5f5f2;
border: solid #bfbfba;
border-width: 1px 0;
color: #333;
text-transform: uppercase;
We also wondered whether that change might be out of scope for this issue since it would require a css change to the Seven theme.
or
to post comments
Comment
#75
lendude
Dutch
Amsterdam
commented
14 April 2018 at 05:33
Status:
Needs review
» Needs work
Something I noticed wile writing tests for this, when the view has no results the empty result area gets turned into a th while the setting is set to 'none'. That doesn't sound like the desired behaviour.
or
to post comments
Comment
#76
themusician
commented
25 April 2018 at 19:25
@Lendude, thanks for outlining what the tests would look like. For anyone wanting to wrap up tests take a look at drupal/core/modules/views/tests/src/Functional/Plugin/StyleTableTest.php starting on line 250 once the most recent patch has been applied.
In regards to no results returning from a table styled view, I cannot recreate that occurrence.
It seems a no results default for views in table format is to simply not display anything at all. I cannot get an empty table to render if there are results.
or
to post comments
Comment
#77
lendude
Dutch
Amsterdam
commented
2 May 2018 at 06:07
@theMusician it was the 'empty message' that got set to a

, but I can't reproduce it anymore, no idea how I got it that state, but thanks for checking!
or
to post comments
Comment
#78
rakesh.gectcr
he/him
Manchester
commented
5 May 2018 at 14:52
Issue tags:
Nwdug_may18
Adding tag for the sprint
or
to post comments
Comment
#79
andrewmacpherson
commented
6 May 2018 at 10:54
@rakesh.gectcr - I'm attending the NWDUG sprint too. The remaining work here is to finsih the tests, and it would be good to look at the core theme styles more closely.
or
to post comments
Comment
#80
andrewmacpherson
commented
8 May 2018 at 13:48
Issue tags:
Novice
, -
Needs screenshots
The novice tasks in #70 were done, not currently needed.
or
to post comments
Comment
#81
andrewmacpherson
commented
20 May 2018 at 04:06
Parent issue:
#2974071: [META] Improve context in admin UI tables for accessibility.
Gathering some issues up into a bigger-picture plan for improving tables.
or
to post comments
Comment
#82
20 May 2018 at 04:06
Version:
8.6.x-dev
» 8.7.x-dev
Drupal 8.6.0-alpha1
will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the
Drupal 8 minor version schedule
and the
Allowed changes during the Drupal 8 release cycle
or
to post comments
Comment
#83
20 May 2018 at 04:06
Version:
8.7.x-dev
» 8.8.x-dev
Drupal 8.7.0-alpha1
will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the
Drupal 8 minor version schedule
and the
Allowed changes during the Drupal 8 release cycle
or
to post comments
Comment
#84
20 May 2018 at 04:06
Version:
8.8.x-dev
» 8.9.x-dev
Drupal 8.8.0-alpha1
will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the
Drupal 8 and 9 minor version schedule
and the
Allowed changes during the Drupal 8 and 9 release cycles
or
to post comments
Comment
#85
20 May 2018 at 04:06
Version:
8.9.x-dev
» 9.1.x-dev
Drupal 8.9.0-beta1
was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the
Drupal 8 and 9 minor version schedule
and the
Allowed changes during the Drupal 8 and 9 release cycles
or
to post comments
Comment
#86
20 May 2018 at 04:06
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
or
to post comments
Comment
#87
themusician
commented
9 April 2021 at 18:43
Issue tags:
Status
File
Size
new
2770835-87.patch
10.46 KB
Rerolled the patch from #72.
or
to post comments
Comment
#88
themusician
commented
9 April 2021 at 21:22
Status
File
Size
new
2770835-88.patch
21.24 KB
Trying a new editor and apparently my formatter is not firing on save. Another attempt.
or
to post comments
Comment
#89
themusician
commented
12 April 2021 at 16:40
Status
File
Size
new
2770835-89.patch
33.8 KB
Ok. I think I finally have PHP code sniffer configured properly with Lando.
Attempt at the re-roll #3.
or
to post comments
Comment
#90
themusician
commented
12 April 2021 at 17:29
Status
File
Size
new
2770835-90.patch
35.14 KB
And one more fix with functions needing comments.
or
to post comments
Comment
#91
carlygerard
She/Her/Hers
commented
12 April 2021 at 23:11
Status
File
Size
new
2770835-table-settings.png
141.32 KB
new
2770835-th-markup.png
274.84 KB
Testing out the patch in
#90,
I could get the template to print correctly in the view, with a
,
if I didn't include the fixes with the views_ui.theme.inc.
Adding in the changes from the views_ui.theme.inc file caused the row header settings to no longer apply correctly. But, without adding the views_ui.theme.inc changes, the table settings is no longer usable:
Wondering if anyone else is running into this applying the patch from #90, or if it's just something I'm doing.
or
to post comments
Comment
#92
12 April 2021 at 23:11
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
or
to post comments
Comment
#93
12 April 2021 at 23:11
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
or
to post comments
Comment
#94
carlygerard
She/Her/Hers
commented
19 January 2022 at 23:58
Status
File
Size
new
2770835-92.patch
16.05 KB
Patch includes Olivero and Bartik view table templates, with row scope support. This still uses tests written by @lendude referenced in
#75
or
to post comments
Comment
#95
carlygerard
She/Her/Hers
commented
20 January 2022 at 16:04
Status
File
Size
new
2770835-95.patch
25.38 KB
Cleaning up PHP files through phpcbf based on previous test results.
or
to post comments
Comment
#96
ranjith_kumar_k_u
commented
24 January 2022 at 04:21
Status
File
Size
new
2770835-96.patch
17.56 KB
Re-rolled #95 for 9.4.
or
to post comments
Comment
#97
ranjith_kumar_k_u
commented
24 January 2022 at 12:30
Status
File
Size
new
2770835-97.patch
17.56 KB
or
to post comments
Comment
#98
carlygerard
She/Her/Hers
commented
26 January 2022 at 23:07
Status
File
Size
new
2770835-98-9.4.x.patch
20.71 KB
new
2770835-98-9.4.x.patch
20.71 KB
Including Umami views-view-table twig file to fix classy copy error. (Duplicate patch error--previewing comments made it seem the file was removed, had re-added just in case.)
or
to post comments
Comment
#99
vikashsoni
commented
28 January 2022 at 11:23
Status
File
Size
new
2770835--before--patch--pic.png
68 bytes
new
2770835--after--patch--pic.png
68 bytes
Applied patch #96 in drupal-9.3.x-dev applied successfully
After patch row header has been added
Thanks for the patch
for ref sharing screenshot ...
or
to post comments
Comment
#100
themusician
commented
2 February 2022 at 23:39
Status
File
Size
new
2770835-100.patch
21.92 KB
This patch updates the hash for the file views-view-table.html.twig which appears to have been causing the failed tests. This works locally running the test ConfirmClassyCopiesTest.php --filter=testClassyHashes with phpUnit.
or
to post comments
Comment
#101
carlygerard
She/Her/Hers
commented
3 February 2022 at 15:29
Status
File
Size
new
2770835-101.patch
24.24 KB
Updates classy view table templates in Claro and Seven to fix test results in #100.
or
to post comments
Comment
#102
themusician
commented
9 February 2022 at 22:51
Issue summary:
View changes
Issue tags:
Needs tests
Needs change record
Related issues:
#2917814: Views table style info never gets updated when changing/adding fields
The patch in #101 works for 9.4.x and prior recent rerolls work for earlier Drupal installs.
The issue summary has been updated to reflect where I believe this issue now stands.
In the remaining tasks, there exists
TODO: Update existing views which use the table display plugin, to include the new config option (set to none). See comment #67.
In reviewing that comment I am unsure if this is truly a blocker for improving the accessibility options available in Drupal views.
Change record coming for review.
or
to post comments
Comment
#103
themusician
commented
9 February 2022 at 23:26
Issue summary:
View changes
Status:
Needs work
» Needs review
Issue tags:
Needs change record
Please review the change record at
Primarily this was based on the prior recommendations and the work discovered while crafting the patches with everyone on this issue.
or
to post comments
Comment
#104
10 February 2022 at 01:23
The last submitted patch,
98: 2770835-98-9.4.x.patch
, failed testing.
View results
or
to post comments
Comment
#105
carlygerard
She/Her/Hers
commented
16 February 2022 at 22:50
Status
File
Size
new
interdiff_95-102.txt
596 bytes
Interdiff between failing 9.3.x #95 and passing #101 for 9.4.x. Patch couldn't apply to 9.3.x because of conflicting comment.
or
to post comments
Comment
#106
carlygerard
She/Her/Hers
commented
16 February 2022 at 22:50
Status
File
Size
new
2770835-101-9.3.x.patch
23.85 KB
Refactored #104 patch to cleanly apply to 9.3.x.
or
to post comments
Comment
#107
carlygerard
She/Her/Hers
commented
18 February 2022 at 19:04
Status
File
Size
new
2770835-107.patch
56.74 KB
Reroll #106 to add stable and starter-kit view templates.
or
to post comments
Comment
#108
carlygerard
She/Her/Hers
commented
18 February 2022 at 19:08
Status
File
Size
new
2770835-108.patch
30.85 KB
Reroll #101 to add stable and starter-kit view templates for 9.4.x.
or
to post comments
Comment
#109
carlygerard
She/Her/Hers
commented
18 February 2022 at 22:19
Status
File
Size
new
2770835-109.patch
29.17 KB
Reroll #107 to pass php formatting tests in 9.3.x.
or
to post comments
Comment
#110
wrd-oaitsd
commented
8 March 2022 at 15:45
#109 is working nicely for me.
or
to post comments
Comment
#111
themusician
commented
30 March 2022 at 22:15
Issue tags:
Portland2022
or
to post comments
Comment
#112
lendude
Dutch
Amsterdam
commented
30 April 2022 at 11:56
Status:
Needs review
» Needs work
Please don't add unrelated coding standards changes to these patches. It makes them bigger and so much harder to review because we now need to think about if a change is needed.
Setting back to needs work to limit this to the changes that are actually needed for this feature.
or
to post comments
Comment
#113
themusician
commented
4 May 2022 at 21:31
Thank you for the feedback.
Can you help me understand what is unrelated? In 9.4.x it seems like there are a lot of views-view-table twig files in Drupal core. Without including those files, the tests fail to pass I believe due to /Core/Theme/ConfirmClassyCopiesTest.php. As I understand it, if Bartik for example has a copy of the views-view-table twig file and it is not updated, the accessibility improvement in this patch would be ignored because that would override the template default.
Beyond updating those files and the hash to make the tests pass 9.x I am not sure what else is not related.
If those files do not need to be updated, what is the trick for making the tests ignore them?
More than happy to do the work, I just don't know what I don't know.
or
to post comments
Comment
#114
4 May 2022 at 21:31
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
or
to post comments
Comment
#115
andregp
commented
17 June 2022 at 19:38
Status:
Needs work
» Needs review
Status
File
Size
new
2770835-115.patch
24.12 KB
new
interdiff_2770835_109-115.txt
4.26 KB
@Lendude, Here I tried to remove all CS corrections that weren't necessary for the patch.
or
to post comments
Comment
#116
Charchil Khandelwal
commented
21 September 2022 at 10:28
Assigned:
Unassigned
Charchil Khandelwal
or
to post comments
Comment
#117
Charchil Khandelwal
commented
21 September 2022 at 11:43
Assigned:
Charchil Khandelwal
» Unassigned
Status:
Needs review
» Reviewed & tested by the community
Status
File
Size
new
img-after-patch(2).PNG
32.3 KB
new
img-after-patch(1).PNG
52.44 KB
new
img-before-patch(2).PNG
31.09 KB
new
img-before-patch(1).PNG
53.96 KB
Patch # 115 tested and applied successfully in drupal 9.5.x version.
or
to post comments
Comment
#118
alexpott
he/they
🇪🇺🌍
commented
4 October 2022 at 22:26
Version:
9.5.x-dev
» 10.1.x-dev
Status:
Reviewed & tested by the community
» Needs work
I think we need to target 10.1.x for this feature request as we're in beta for 9.5.x so it's closed to new features. Plus the patch doesn't apply to 10.1.x so we need to 10.x version anyway.
+++ b/core/modules/views/tests/src/Functional/Plugin/StyleTableTest.php
@@ -242,6 +242,23 @@ public function testGrouping() {
+ $this->assertNotEmpty($this->cssSelect('tbody th'));
Can't we assert on the actual content. Or something a bit more positive. Also shouldn't we be selecting on
scope="row"
Plus we have no test coverage that the UI actually works.
or
to post comments
Comment
#119
themusician
commented
17 November 2022 at 21:59
Status
File
Size
new
reroll_diff_115-119.txt
10.87 KB
new
2770835-119.patch
14.55 KB
This doesn't get us a UI test coverage fix but I believe this is now applying cleanly to 10.1.x-dev.
The original patch from 9.5.x trying to apply to 10.1.x-dev.
git apply --check ../2770835-115.patch
error: core/tests/Drupal/KernelTests/Core/Theme/ConfirmClassyCopiesTest.php: No such file or directory
error: core/themes/bartik/templates/classy/views/views-view-table.html.twig: No such file or directory
error: core/themes/classy/templates/views/views-view-table.html.twig: No such file or directory
error: core/themes/seven/templates/classy/views/views-view-table.html.twig: No such file or directory
error: core/themes/stable/templates/views/views-view-table.html.twig: No such file or directory
This makes sense as those files have been removed from 10.1.
Attached is a new patch and a diff of the reroll showing the files being removed from the new patch.
or
to post comments
Comment
#120
15 December 2022 at 20:04
theMusician
opened
merge request !3114
or
to post comments
Comment
#121
mgifford
he/him
commented
20 January 2023 at 20:23
Issue tags:
wcag111
This fits under WCAG SC 1.1.1.
or
to post comments
Comment
#122
pooja saraah
commented
24 January 2023 at 07:16
Status
File
Size
new
2770835-122.patch
14.43 KB
Fixed failed commands on #120
Attached patch against Drupal 10.1.x
or
to post comments
Comment
#123
themusician
commented
25 January 2023 at 18:09
Status:
Needs work
» Needs review
or
to post comments
Comment
#124
lendude
Dutch
Amsterdam
commented
23 February 2023 at 07:59
Status:
Needs review
» Needs work
Still needs test coverage for the Views UI working.
+++ b/core/modules/views/tests/src/Functional/Plugin/StyleTableTest.php
@@ -242,6 +242,24 @@ public function testGrouping() {
+ $this->assertNotEmpty($this->cssSelect('tbody th[scope="row"]'));
This is still too minimal, we should actually check what is in it
+++ b/core/themes/olivero/templates/views/views-view-table.html.twig
index a26eaeba85..a212ef8dcc 100644
--- a/core/themes/stable9/templates/views/views-view-table.html.twig

--- a/core/themes/stable9/templates/views/views-view-table.html.twig
+++ b/core/themes/stable9/templates/views/views-view-table.html.twig
I don't think we can change these here templates since they are 'stable'
And looking at this again, we should add an upgrade path to set the default value for existing Views so people don't get unrelated changes later when they change a View
or
to post comments
Comment
#125
themusician
commented
21 April 2023 at 17:56
Thank you for the feedback Lendude. Items 1 and 2 I believe I have knowledge of how to address moving forward.
Regarding an upgrade path for existing views, in the proposed patch this is set in Views/Style/Table.php,
$options['row_header'] = ['default' => ' '];
For existing views, where would I look to provide an upgrade path? In local testing, existing views seem to show the new row header option but not have it selected. I am guessing I am missing something obvious.
I appreciate any guidance you can offer.
Thank you
or
to post comments
Comment
#126
themusician
commented
5 May 2023 at 18:03
Status
File
Size
new
2770835-126.patch
13.29 KB
A new patch that does a more robust check for scope=row on output.
Removal of changes to stable9.
Thank you @CarlyGerard and @packern for team programming on this patch.
or
to post comments
Comment
#127
themusician
commented
5 May 2023 at 18:22
Status
File
Size
new
2770835-127.patch
13.3 KB
Missed the code formatting fixes. Trying again.
or
to post comments
Comment
#128
themusician
commented
12 May 2023 at 17:05
Status:
Needs work
» Needs review
I believe the new patch satisfies the concerns raised earlier in the discussion.
or
to post comments
Comment
#129
smustgrave
commented
12 May 2023 at 21:15
Status:
Needs review
» Needs work
Issue tags:
, -
govcon2016
, -
Seven
, -
Bartik
, -
Dublin2016
, -
CSKyiv18
, -
Nashville2018
, -
Nwdug_may18
, -
Portland2022
Needs accessibility review
, +
Needs Review Queue Initiative
Cleaned up the tags some.
Seems #124 still needs to be addressed.
For the upgrade path it will, I believe,
1. need to load all views
2. Check if they are using table
3. Add this setting even if it's unchecked
Idea being if this patch is applied and I open a view with a table. If I make 0 changes and click save and configuration is shown as changed because of this new setting then that would need upgrade path.
Upgrade test can be simple
Check a view table that this setting doesn't exist
Run updates
Check setting now exists.
or
to post comments
Comment
#130
12 May 2023 at 21:15
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
or
to post comments
Comment
#131
carlygerard
She/Her/Hers
commented
3 October 2023 at 17:43
Status
File
Size
new
2770835-130.patch
14.45 KB
new
interdiff_127-130.txt
3.61 KB
Adds upgrade path test to views where if not set, set header to "none." The interdiff showed a template also removed a classy template--I think in 11.x-dev the classy files are removed, unless someone can confirm otherwise?
or
to post comments
Comment
#132
carlygerard
She/Her/Hers
commented
5 October 2023 at 23:55
Status
File
Size
new
2770835-132.patch
14.57 KB
new
interdiff_127-132.txt
3.61 KB
Re-rolled patch from
comment #131
since it couldn't apply.
or
to post comments
Comment
#133
carlygerard
She/Her/Hers
commented
6 October 2023 at 21:22
Status
File
Size
new
2770835-133.patch
14.62 KB
new
interdiff_127-133.txt
6.31 KB
Fixing custom commands error from ViewsConfigUpdater.php in
#132
, where the bool value wasn't returned properly given the function parameter.
or
to post comments
Comment
#134
carlygerard
She/Her/Hers
commented
3 November 2023 at 18:20
Status
File
Size
new
2770835-134.patch
14.6 KB
new
interdiff_133_134.txt
774 bytes
Fixes undefined array key issue flagged from ViewsConfigUpdater.php in last CI testing.
or
to post comments
Comment
#135
themusician
commented
3 November 2023 at 21:28
Status:
Needs work
» Needs review
Hooray. Passing tests again and they are more comprehensive. I believe this checks all the tests described in comment #129.
or
to post comments
Comment
#136
themusician
commented
17 November 2023 at 18:13
The change record has been updated to reflect the current passing patch.
or
to post comments
Comment
#137
lendude
Dutch
Amsterdam
commented
7 December 2023 at 07:51
Status:
Needs review
» Needs work
+++ b/core/modules/views/views.post_update.php
@@ -137,3 +137,14 @@ function views_post_update_taxonomy_filter_user_context(?array &$sandbox = NULL)
+ return $view_config_updater->setRowHeaderOption($view);
This should follow the pattern of 'needsABC' and further changes to ConfigEntityUpdater so it will also update newly imported config when installing a module like we do in other Views updates.
Also we need a test for the upgrade path to make sure it actually works
or
to post comments
Comment
#138
themusician
commented
26 February 2024 at 21:30
As always, thank you for the feedback and guidance.
On my dev instance, there is an upgrade test in theory that operates but I can't get views actually to run an update hook.
A new file in tests/fixtures/update exists called set_row_header_option.php
It looks like
/**
* @file
* Test fixture.
*/

use Drupal\Core\Database\Database;
use Drupal\Core\Serialization\Yaml;

$connection = Database::getConnection();

$connection->insert('config')
->fields([
'collection' => '',
'name' => 'views.view.test_set_row_header_option',
'data' => serialize(Yaml::decode(file_get_contents('core/modules/views/tests/fixtures/update/views.view.test_set_row_header_option.yml'))),
])
->execute();
and then in tests/src/Functional/Update/ViewsSetRowHeaderUpdateTest.php I try to run the test which includes firing the updates using runUpdates(); which as I understand it runs update hooks. It is modeled after ViewsFixRevisionUpdateTest.php in the same directory.
/**
* Tests the upgrade path for adding row header option.
*/
public function testViewsPostUpdateSetRowHeaderOption() {
$view = View::load('test_set_row_header_option');
$data = $view->toArray();

// Assert no row header exists
$this->assertArrayNotHasKey('row_header', $data['display']['default']['display_options']['style']['options']);

$this->runUpdates();

$view = View::load('test_set_row_header_option');
$data = $view->toArray();

// Assert row header option exists, return values
$this->assertArrayHasKey('row_header', $data['display']['default']['display_options']['style']['options']);
which I think means the update hook needs to be in views.install. I have tried various iterations but when runUpdates() executes it doesn't apply any changes. Is there a different spot that updates go to when running tests? I can't find any views updates that would correspond to the other tests in /Tests/Src/Functional/Update.
I don't get any errors in the tests except that after running the updates, the new key cannot be found which tells me the hook_update I added into views.install is not running properly. If I introduce a syntax error into the hook_update, the test will fail and point out the syntax error, so it does seem like it wants the code to be in that spot.
Thank you for any hints.
or
to post comments
Comment
#139
themusician
commented
31 May 2024 at 22:16
I believe the code for ViewsConfigUpdater needs to be called
in updateAll of viewsConfigUpdater for the test runUpdates() function to work.
or
to post comments
Comment
#140
31 May 2024 at 22:16
Version:
11.x-dev
» main
Drupal core is now using the
main
branch as the primary development branch. New developments and disruptive changes should now be targeted to the
main
branch.
Read more in the announcement
or
to post comments
Contribution record
Change record drafts
Views table display now supports row headers
Parent issue
Child issues
#2770843: Add support for tables with two headers in views table display - D7 backport
Add child issue
clone issue
Related issues
Referenced by
#2792743: [D7] Views Table Row Headers
#2962585: Treat entity label as a row-level header in entity list tables
#3032312: Table style for row-level table headers in Claro
#3093249: Implement tables on Olivero proof of concept
#3566386: Accessibility: Add scope="col" attribute to payment list builder table headers
#3566687: Add scope attributes to table render element header cells for WCAG 1.3.1 compliance
Infrastructure management for Drupal.org provided by
Need a Drupal 7 extended support partner? Consider Tag1.
News items
News
Planet Drupal
Social media
Sign up for Drupal news
Security advisories
Jobs
Our community
Community
Services
Training
Hosting
Contributor guide
Groups & meetups
DrupalCon
Code of conduct
Documentation
Documentation
Drupal Guide
Drupal User Guide
Developer docs
API.Drupal.org
Drupal code base
Download & Extend
Drupal core
Modules
Themes
Distributions
Governance of community
About
Web accessibility
Drupal Association
About Drupal.org
Drupal is a
registered trademark
of
Dries Buytaert