[#70190] Add missing accessibility semantics for Border Box Table

app/components/op_primer/border_box_row_component.html.erb

@@ -1,4 +1,4 @@
-<%#-- copyright
+<%# -- copyright
 OpenProject is an open source project management software.
 Copyright (C) the OpenProject GmbH
 
@@ -25,31 +25,35 @@ Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
 
 See COPYRIGHT and LICENSE files for more details.
 
-++#%>
-
-<%=
-  render Primer::BaseComponent.new(
-    tag: :div,
-    id: row_css_id,
-    classes: "#{table.grid_class} #{row_css_class}"
-  ) do
-%>
-  <% columns.each do |column| %>
-    <%= render(Primer::BaseComponent.new(tag: :div, classes: grid_column_classes(column), **column_args(column))) do %>
-      <% label = mobile_label(column) %>
-      <% if label.present? %>
-        <%= render(Primer::Beta::Text.new(classes: "op-border-box-grid--row-label")) { "#{label}: " } %>
-      <% end %>
-      <%= column_value(column) %>
+++# %>
+
+<% columns.each.with_index do |column, index| %>
+  <%=
+    render(
+      Primer::BaseComponent.new(
+        tag: :div,
+        classes: cell_classes(column),
+        role: cell_role(column, index),
+        aria: { colindex: index + 1 }
+      )
+    ) do %>
+    <% label = mobile_label(column) %>
+    <% if label.present? %>
+      <%= render(Primer::Beta::Text.new(classes: "op-border-box-grid__row-label")) { "#{label}: " } %>
     <% end %>
+    <%= column_value(column) %>
   <% end %>
+<% end %>
 
-  <% if table.has_actions? %>
-    <%= flex_layout(classes: "op-border-box-grid--row-action") do |flex| %>
-      <% button_links.each_with_index do |link, i| %>
-        <% args = i == (button_links.count - 1) ? {} : { mr: 1 } %>
-        <% flex.with_column(**args) { link } %>
-      <% end %>
+<% if table.has_actions? %>
+  <%=
+    flex_layout(
+      classes: "op-border-box-grid__row-action gap-1",
+      role: :cell,
+      aria: { colindex: columns.size + 1 }
+    ) do |flex| %>
+    <% button_links.each do |link| %>
+      <% flex.with_column { link } %>
     <% end %>
   <% end %>
 <% end %>

app/components/op_primer/border_box_row_component.rb

@@ -42,18 +42,20 @@ def visible_on_mobile?(column)
       table.mobile_columns.include?(column)
     end
 
-    def grid_column_classes(column)
-      classes = ["op-border-box-grid--row-item"]
-      classes << column_css_class(column)
-      classes << "op-border-box-grid--main-column" if table.main_column?(column)
-      classes << "ellipsis" unless table.main_column?(column)
-      classes << "op-border-box-grid--no-mobile" unless visible_on_mobile?(column)
-
-      classes.compact.join(" ")
+    def cell_classes(column)
+      class_names(
+        "op-border-box-grid__row-item",
+        column_css_class(column),
+        {
+          "op-border-box-grid__row-item--main-column": table.main_column?(column),
+          ellipsis: !table.main_column?(column),
+          "op-border-box-grid__row-item--no-mobile": !visible_on_mobile?(column)
+        }
+      )
     end
 
-    def column_args(_column)
-      {}
+    def cell_role(column, colindex)
+      table.main_column?(column) && colindex.zero? ? :rowheader : :cell
     end
 
     def checkmark(condition)

app/components/op_primer/border_box_table_component.html.erb

@@ -1,4 +1,4 @@
-<%#-- copyright
+<%# -- copyright
 OpenProject is an open source project management software.
 Copyright (C) the OpenProject GmbH
 
@@ -25,62 +25,118 @@ Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
 
 See COPYRIGHT and LICENSE files for more details.
 
-++#%>
+++# %>
 
 <%=
   render(
-    border_box_container(
+    OpPrimer::BorderBoxTableComponent::InternalBoxComponent.new(
       id: container_id,
       classes: container_class,
+      role: :table,
+      aria: { label: mobile_title, colcount: column_count },
+      list_arguments: { tag: :div, role: :rowgroup },
       test_selector:
     )
   ) do |component|
-    component.with_header(classes: grid_class, color: :muted) do
-      headers.each do |name, args|
-        concat render(
+%>
+  <% component.with_header(role: :rowgroup) do %>
+    <%=
+      render(
+        Primer::BaseComponent.new(
+          tag: :div,
+          color: :muted,
+          classes: [grid_class, "#{grid_class}--has-mobile-header"],
+          role: :row
+        )
+      ) do
+        render(
           Primer::Beta::Text.new(
-            classes: header_classes(name),
+            classes: "#{grid_class}__mobile-header",
             font_weight: :semibold,
-            **header_args(name)
+            role: :columnheader,
+            aria: { colspan: column_count }
           )
-        ) { args[:caption] }
+        ) { mobile_title }
       end
-
-      concat render(
-        Primer::Beta::Text.new(
-          classes: "op-border-box-grid--mobile-heading",
-          font_weight: :semibold
+    %>
+    <%=
+      render(
+        Primer::BaseComponent.new(
+          tag: :div,
+          color: :muted,
+          classes: [grid_class, "#{grid_class}--has-headers"],
+          role: :row
         )
-      ) { mobile_title }
+      ) do
+    %>
+      <% headers.each.with_index do |(name, args), index| %>
+        <%=
+          render(
+            Primer::Beta::Text.new(
+              classes: header_classes(name),
+              font_weight: :semibold,
+              role: :columnheader,
+              aria: { colindex: index + 1 }
+            )
+          ) { args[:caption] }
+        %>
+      <% end %>
 
-      if has_actions?
-        concat render(
-          Primer::BaseComponent.new(
-            classes: header_classes(:actions),
-            tag: :span
-          )
-        ) { action_row_header_content }
-      end
-    end
+      <% if has_actions? %>
+        <%=
+          render(
+            Primer::BaseComponent.new(
+              classes: header_action_class,
+              tag: :span,
+              role: :columnheader,
+              aria: { colindex: column_count }
+            )
+          ) do
+        %>
+          <%= render(Primer::Beta::Text.new(classes: "sr-only")) { t(:label_actions) } %>
+          <%= action_row_header_content %>
+        <% end %>
+      <% end %>
+    <% end %>
+   <% end %>
 
-    if rows.empty?
-      component.with_row(scheme: :default) { render_blank_slate }
-    else
+  <% if rows.empty? %>
+    <%
+      component.with_row(scheme: :default, tag: :div, role: :row) do
+        render(Primer::BaseComponent.new(tag: :div, role: :cell, aria: { colspan: column_count })) do
+          render_blank_slate
+        end
+      end
+    %>
+  <% else %>
+    <%
       rows.each do |row|
         row_instance = row_class.new(row: row, table: self)
-        component.with_row(scheme: row_instance.scheme) do
+        component.with_row(
+          scheme: row_instance.scheme,
+          tag: :div,
+          id: row_instance.row_css_id,
+          classes: [grid_class, row_instance.row_css_class],
+          role: :row
+        ) do
           render(row_instance)
         end
       end
-    end
+    %>
+  <% end %>
 
-    if has_footer?
-      component.with_footer(classes: grid_class, color: :muted) do
-        footer
-      end
-    end
-  end
-%>
+    <% if has_footer? %>
+      <%
+        component.with_footer(role: :rowgroup) do
+          render(Primer::BaseComponent.new(tag: :div, color: :muted, classes: grid_class, role: :row)) do
+            render(Primer::BaseComponent.new(tag: :div, role: :cell, aria: { colspan: column_count })) do
+              footer
+            end
+          end
+        end
+      %>
+    <% end %>
+  <% end %>
 
 <% if paginated? %>
   <%= helpers.pagination_links_full rows %>

app/components/op_primer/border_box_table_component.rb

@@ -77,25 +77,24 @@ def main_column?(column)
       self.class.main_column.include?(column)
     end
 
-    def header_args(_column)
-      {}
-    end
-
     def column_title(name)
       _, header_options = headers.assoc(name)
       header_options&.dig(:caption)
     end
 
     def header_classes(column)
-      classes = [heading_class]
-      classes << "op-border-box-grid--main-column" if main_column?(column)
-      classes << "op-border-box-grid--heading-action" if column == :actions
+      class_names(
+        header_class,
+        "op-border-box-grid__header--main-column": main_column?(column)
+      )
+    end
 
-      classes.join(" ")
+    def header_action_class
+      "op-border-box-grid__header-action"
     end
 
-    def heading_class
-      "op-border-box-grid--heading"
+    def header_class
+      "op-border-box-grid__header"
     end
 
     # Default grid class with equal weights
@@ -146,5 +145,11 @@ def action_row_header_content
     def footer
       raise ArgumentError, "Need to provide footer content"
     end
+
+    private
+
+    def column_count
+      @column_count ||= columns.size + (has_actions? ? 1 : 0)
+    end
   end
 end

app/components/op_primer/border_box_table_component.sass

@@ -3,19 +3,19 @@
 .op-border-box-grid
   display: grid
 
-  &--row-item
-    word-break: break-word
-
-  &--heading
+  &__header
     align-self: center
 
-  &--row-action,
-  &--heading-action
+  &__row-item
+    word-break: break-word
+
+  &__header-action,
+  &__row-action
     display: flex
     align-items: flex-start
     justify-content: flex-end
 
-  &--row-action
+  &__row-action
     margin-top: -6px // Moving the button slightly to the top, so that the icon aligns visually with the other columns
 
 @media screen and (min-width: $breakpoint-md)
@@ -26,33 +26,41 @@
     justify-content: flex-start
     align-items: flex-start
 
-    &--heading,
-    &--row-item
+    &--has-mobile-header
+      display: none
+
+    &__header,
+    &__row-item
       &:not(:first-child)
         padding-left: 6px
 
       &:not(:last-child)
         padding-right: 6px
 
-    &--mobile-heading,
-    &--row-label
-      display: none
+      &--main-column
+        grid-column: span 2
 
-    &--main-column
-      grid-column: span 2
+    &__mobile-header,
+    &__row-label
+      display: none
 
 @media screen and (max-width: $breakpoint-md)
   .op-border-box-grid
     grid-template-columns: 1fr auto
     grid-auto-flow: row
 
-    &--heading,
-    &--no-mobile
+    &--has-headers
       display: none
 
-    &--row-item
+    &__header
+      display: none
+
+    &__row-item
       grid-column: 1
 
-    &--row-action
+      &--no-mobile
+        display: none
+
+    &__row-action
       grid-column: 2
       grid-row: 1

app/components/op_primer/border_box_table_component/internal_box_component.html.erb

@@ -0,0 +1,12 @@
+<%= render Primer::BaseComponent.new(**@system_arguments) do %>
+  <%= header %>
+  <%= body %>
+  <% if rows.any? %>
+    <%= render Primer::BaseComponent.new(**@list_arguments) do %>
+      <% rows.each do |row| %>
+        <%= row %>
+      <% end %>
+    <% end %>
+  <% end %>
+  <%= footer %>
+<% end %>