Convert simpletest theme tables to table #type

Seem to have the same issue as

http://drupal.org/node/1876712#comment-7156742

There's some issues converting some of the tables that relay on cell attributes (like the permissions table, where the module name column needs a class and a colspan), and it doesn't appear that table form types don't allow columns to have attributes. Maybe a separate issue needs to be opened?

#1948374: #type 'table' allow attributes on table cells is in so I think we can unpostpone this.

Going to see if I can something rolling on this over the next week or so.

@Cottser, headstart... maybe. The following patch doesn't work in it's current state but most of the pieces are there for this table to work.

I ran into trouble because the checkbox fields weren't rendered yet so they don't have an #id to add to the JS so I started trying to mimic the way they are created. I think because of the JS, also the input fields for the groups don't show.

Hope that helps.

@joelpittet - Yeah, I didn't get as far as you did working on this tonight but I did figure out that the #id thing would be an issue and had things moving in roughly the same direction as you did :)

Thanks, will see if I can bounce it back in a day or two and push things along further.

I think this should get the sorting working without a second loop. And something I did here made the checkboxes pop-in and toggle correctly as well. Though I have a feeling without the FAPI processing things won't act the same on form submit/validate for which items were previously open.

Should have time for this again over the next few days.

First a reroll, small conflict with #1846172: Replace the actions API.

#8 contained some additional changes, here is the actual reroll of #6 with -do-not-test, and an interdiff for the few changes between #6 and #8.

Edit: Okay the patch here is the same as #8 but the interdiff is legit so leaving it at that.

Sigh. I was working from old local branches and somehow was basing my work on #4, not #6. So please ignore everything from #8 to #10 :)

Here is a proper reroll of #6 which seems to be working pretty well locally. The only thing I've noticed so far is that the for attributes on the labels do not match up, so clicking on the test names does not toggle the checkboxes. For now let's see what testbot thinks though.

Getting markup and functionality a bit closer. Will be back to this in a day or two.

Thanks to @joelpittet for the help on IRC tonight :)

Getting markup a bit closer :)

#14: 1938926-14.patch queued for re-testing.

Hell yeah green! Wicked @Cottser!

New patch, separated into two interdiffs:

• First interdiff shows getting the markup matching up.
• Second interdiff is docs cleanup and simplifying things.

(And provided a combined interdiff as well.)

I did markup comparison via view source and running a visual diff and couldn't spot any differences.

Had some trouble during markup comparison with memory running out so I decided to do some profiling. Not sure how much of this is the patch and how much is #type table conversion overhead in general but this 35.9% wt increase looks very high to me, particularly because before the patch the page already took over 2s to render on my machine. Lots of calls to drupal_html_id() that add up quickly. I tried a preg_replace() and str_replace() version and it didn't fare much better. Suggestions and patches welcome, not sure when I will get back to this and IMO this should not be committed with such a large performance regression.

=== 8.x..8.x compared (51ba929760d1b..51ba93c3db76f):

ct  : 516,988|516,988|0|0.0%
wt  : 2,127,045|2,127,755|710|0.0%
cpu : 2,106,514|2,107,777|1,263|0.1%
mu  : 41,202,152|41,202,152|0|0.0%
pmu : 45,592,176|45,592,176|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51ba929760d1b&...

=== 8.x..simpletest-1938926-18 compared (51ba929760d1b..51ba946b609ed):

ct  : 516,988|587,842|70,854|13.7%
wt  : 2,127,045|2,890,020|762,975|35.9%
cpu : 2,106,514|2,870,813|764,299|36.3%
mu  : 41,202,152|57,840,232|16,638,080|40.4%
pmu : 45,592,176|69,531,112|23,938,936|52.5%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51ba929760d1b&...

#18: 1938926-18.patch queued for re-testing.

#18: 1938926-18.patch queued for re-testing.

Tagging, needs reroll

Reroll

@pplantinga, thanks for working on this! I think the reroll is missing some changes. I suggest a fresh reroll from #18.

#18: 3 files changed, 128 insertions, 128 deletions.
#24: 2 files changed, 128 insertions, 9 deletions.

Oops, thanks for catching that.

Re-rolled.

Some minor issues.

1. +++ b/core/modules/simpletest/lib/Drupal/simpletest/Form/SimpletestTestForm.php
   @@ -52,9 +52,40 @@ public function buildForm(array $form, array &$form_state) {
   +        theme('image', array(
   ...
   +        theme('image', array(
   

   This should be render array.

2. +++ b/core/modules/simpletest/lib/Drupal/simpletest/Form/SimpletestTestForm.php
   @@ -63,17 +94,94 @@ public function buildForm(array $form, array &$form_state) {
   +    drupal_add_js(array('simpleTest' => $js), 'setting');
   

   This should be converted to attach.

Confirming the results from #18

Postponing this issue until we find a way to get type #table faster.

=== 8.x..8.x compared (523e49a66dc20..523e4a8c453b2):

ct  : 449,459|449,459|0|0.0%
wt  : 1,837,295|1,846,190|8,895|0.5%
cpu : 1,813,053|1,817,550|4,497|0.2%
mu  : 21,150,224|21,150,224|0|0.0%
pmu : 27,270,752|27,270,752|0|0.0%

Profiler output

=== 8.x..1938926-simpletest-type-table compared (523e49a66dc20..523e4c6056bfd):

ct  : 449,459|501,386|51,927|11.6%
wt  : 1,837,295|2,593,922|756,627|41.2%
cpu : 1,813,053|2,564,378|751,325|41.4%
mu  : 21,150,224|38,555,696|17,405,472|82.3%
pmu : 27,270,752|50,184,984|22,914,232|84.0%

Profiler output

Is there an issue where we are improving #table performance? Seems like a fixable problem - we aren't trying to do anything different from the old way.

@moshe weitzman there is not, but I was going to open one from #2152193: Discuss the fate of #type table

Re-rolled against latest HEAD + revamped to properly use #type table.

Doing so revealed a bug in form_process_table(), which does not ensure to use a custom #id for the tableselect checkbox, and thus, FormBuilder creates a second #id via drupal_html_id(), which then has a '--2' suffix.

Manually tested the test overview page, tableselect + custom group select, as well as instantfilter.

Let's move forward here as quickly as possible. The performance of #type table can only be fixed after all instances of #theme table are gone.

ok, ok, then the other way around :)

Let's move forward here as quickly as possible. The performance of #type table can only be fixed after all instances of #theme table are gone.

That's understandable but I don't think we can just push through the huge performance regression here. While it's great that we have so many tests that it's becoming a performance issue, we can't expect everyone to run tests through the CLI. If this is committed we're going to see a lot of folks reporting out of memory errors (check the peak memory usage increase) on /admin/config/development/testing and I can only see that turning in a revert.

One possible way forward would be to convert everything in a sandbox and do performance optimization there, then bring it back to the core queues once performance is acceptable.

Sorry, this one should finally pass.

form_process_table() did not account for the case of #type table that wants to use #tree FALSE. (it defaults to TRUE for #type table, since that is the most common case)

Thanks for giving this a crack @sun

Re #32

+++ b/core/modules/simpletest/lib/Drupal/simpletest/Form/SimpletestTestForm.php
@@ -53,39 +55,44 @@
-      '#header' => array(
-        array('class' => array('select-all')),
-        array('data' => t('Test'), 'class' => array('simpletest_test')),
-        array('data' => t('Description'), 'class' => array('simpletest_description')),
...
+      '#header' => array($this->t('Test'), $this->t('Description')),

Seems like this removes the first column and each th class as well. Was this intended?

Seems like this removes the first column and each th class as well. Was this intended?

Absolutely yes :)

One of the first and foremost reasons for introducing #type table was to combine several other, super-hard-to-grok alternative concepts into a single API.

Just specify '#tableselect' => TRUE for the #type table element + for the form submit buttons that need a selection. → Done.

All magic happens under the hood. The table gets a first column containing form controls, and your form receives the values that you'd naturally expect. :)

→ We're not only going to reduce #theme table to the max, we will also #1876714: Remove #type 'tableselect'

So regarding #type table and performance concerns, there really is only one direction to accomplish the mission: (1) Convert everything. (2) Refactor/revamp #type table as one and only entry point for structured tables to gain optimal performance.

It is no surprise at all to me that individual conversions to #type table are showing a substantial performance decrease. → Just have a look at what that funky #theme table is actually doing. #theme table is one of the oldest theme functions and call chains in Drupal core since... Drupal 4.7?

There's no way to optimize that as long as there are two completely different entry points to the API for building and rendering a table via structured data. Kill the legacy crap first. Then refactor the modern way.

Lastly, those 'simpletest_' table header classes are completely obsolete and unused, hence removed.

Well if #type=>table is meant to replace #theme=>table couldn't we just remove the theme_table call back and provide the proposed performance improvements directly on to #type table. That way we'd avoid the problem where people use #rows and bypass the element restructuring and the duplicate looping/restructuring preprocess?

Lastly, those 'simpletest_' table header classes are completely obsolete and unused, hence removed.

They aren't unused: @see simpletest.module.css

#simpletest-form-table th.select-all {
  width: 1em;
}
th.simpletest_test {
  width: 16em;
}

Thanks for catching that. I swear I had grepped the CSS for "simpletest_" before already, but somehow missed that instance.

Attached patch should pass all tests.

I agree that we have to proceed here in order to unblock theme system DX. Once this goes in, I will track the reported performance regression at #1744302: [meta] Resolve known performance regressions in Drupal 8

The good news: Testbot passes, JS filtering still works as it did, and form submits correctly. So this is pretty close to RTBC but not quite there.

Markup changes that need some work:

1. +++ b/core/modules/simpletest/lib/Drupal/simpletest/Form/SimpletestTestForm.php
   @@ -120,9 +230,6 @@ public function submitForm(array &$form, array &$form_state) {
   -    else {
   -      drupal_set_message($this->t('No test(s) selected.'), 'error');
   -    }
   

   Removed on purpose? Seems to be user feedback if tests aren't selected on submit.

2. +++ b/core/modules/simpletest/simpletest.module
   @@ -79,10 +79,6 @@ function simpletest_permission() {
   -    'simpletest_test_table' => array(
   -      'render element' => 'table',
   -      'file' => 'simpletest.theme.inc',
   -    ),
   

   Love these:)

Performance

Without the patch:

Requests per second:    0.45 [#/sec] (mean)
Connection Times (ms)
              min  mean[+/-sd] median   max
Total:       2137 2237  78.5   2217    2623

With the patch:

Requests per second:    0.31 [#/sec] (mean)
Connection Times (ms)
              min  mean[+/-sd] median   max
Total:       3090 3234  80.5   3226    3415

And just a visual difference, the page loads exactly 1s slower in yslow, which matches up to the difference in the ab test.

re: #45:

1. Yes, removed on purpose. Form validation of tableselect is handled by #type table. In addition, that code was wrong to begin with, since it is pointless to throw a form validation error in a form submission handler.
2. Yes.

re: annotated diff screenshot:

1. Yes.
2. Adjusted the #tableselect processing to add a .table-select class to the TD.
3. Label: see below.
4. Aria: see below.
5. The changed value of checkbox input elements is expected and caused by #tableselect. Doesn't matter, because Form API processes the POST input data internally anyway.
6. The div.description was removed on purpose, because it is superfluous markup in a HTML page that is large enough already, and because div.description has no styling in the first place. The wrapping table cell has a class already.

Now, attached patch additionally fixes an accessibility problem with #type table:

The tableselect form elements (checkboxes/radios) are currently not associated with their actual/proper labels.

In a #type table with #tableselect, usually the first (second) table column holds the title (label) for the selectable option. However, that label is not associated with the form control. Instead, the form control is rendered with an invisible label:

<tr>
<td>
  <div class="form-item">
  <label class="invisible" for="edit-checkbox">
    Select this item
  </label>
  <input type="checkbox" id="edit-checkbox" />
  </div>
</td>
<td>
  Actual item label here
</td>
</tr>

To fix this, I'm introducing a new #type label, which essentially just wraps theme_form_element_label(), and which is used by the #tableselect processing to automatically associate the proper label for each item in the table, so that the result is this:

<tr>
<td>
  <div class="form-item">
  <input type="checkbox" id="edit-checkbox" aria-labelledby="edit-checkbox--label" />
  </div>
</td>
<td>
  <label id="edit-checkbox--label" for="edit-checkbox">
    Actual item label here
  </label>
</td>
</tr>

→ Works like a charm, fixes accessibility of #tableselect, and even reduces page size.

Note that "aria-labelledby" is not misspelled:
http://www.w3.org/TR/wai-aria/states_and_properties#aria-labelledby

This is RTBC for me, but perhaps @joelpittet has some final input.

@joelpittet - awesome annotated screenshot. I love the red/green numbered comments, and the highlighted source code. How did you make it?

re #46 I'm good with all the changes and rational except for these two:

Yes, removed on purpose. Form validation of tableselect is handled by #type table. In addition, that code was wrong to begin with, since it is pointless to throw a form validation error in a form submission handler.

I'm positive this should still be in there, the implementation may need to change/move and I'm fine with that but the end result should likely be similar and not lost.

The div.description was removed on purpose, because it is superfluous markup in a HTML page that is large enough already, and because div.description has no styling in the first place. The wrapping table cell has a class already.

I agree the div is superflous but the class provides a UI heirarchy change. The .description text is used to demote in the visual hierarchy, in this case the text is lighter and slightly smaller. Can we just add that class to the wrapper?

div.description {
  margin: 5px 0;
  line-height: 1.231em;
  font-size: 0.923em;
  color: #555;
}

Also, I like that the label changes! Makes much more sense now thank you for that. I'm not super happy with that whole themeable #types implementation with $element = $variables['#element'] being littered through out the theme functions but that's a different problem. (read ranty, rant rant). Nevertheless I'll split my consolidation of form_element_label theme as it is probably the path of least resistance right now to allow for this #type label bit. Thanks for bringing up this use-case for it... tables FTW? :P

@moshe weitzman thanks, it's just Skitch 2 + Sublime 3 (itg.flat.dark.green.sublime-theme at the moment).

Also, just realized crap! that .description class is targeting the div selector... well maybe we just add those styles to the description selector class that is already on it? the css class selector shouldn't be targetting like that but needs cleanup. Maybe just:

.simpletest-test-description {
  font-size: 0.923em;
  color: #555;
}

The margin & line-height don't help.

removed on purpose. Form validation of tableselect is handled by #type table. In addition, that code was wrong to begin with, since it is pointless to throw a form validation error in a form submission handler.

I'm positive this should still be in there, the implementation may need to change/move and I'm fine with that but the end result should likely be similar and not lost.

Could you please elaborate on what you're missing? I'm happy to explain how #type table + #tableselect + Form API works, but I don't know what your technical experience level on these topics is, so I don't know where to start.

The end result is the same as before: If no items are selected, Form API throws a form validation error that informs the user that no items were selected and no tests are run.

Also, just realized crap! that .description class is targeting the div selector...

well maybe we just add those styles to the description selector class that is already on it? the css class selector shouldn't be targetting like that but needs cleanup.

A few thoughts on that:

1. I agree that div.description could/should most probably just be .description → Let's create an issue to explore that.
   (would have the nice potential of possibly getting rid of .fieldset-description and .details-description, too)
2. Especially when looking at the manually crafted screenshot provided in #49, it is weird that the content of one table cell in a de-facto two-column table is styled differently than the first/other column. To my knowledge, no other table in Drupal core uses such styling. → Why adjust/override the style in the first place?
3. This is a developer-only-facing UI. In fact, the Simpletest module UI is so much pure developer material, it shouldn't even be in core in the first place. → We shouldn't spend too much time on this. If necessary, this UI can even be improved after the 8.0.0 stable release.

Re #50

1. For the <div class="description> is to be dropped, the style for the developer experience's UX should not lose out on the deal. So the end result is either: add the styles in #49 in the css files, or add the <div class="description> back in.
2. ('No test(s) selected.'), 'error'); Right now, the current patch in #46 no longer has user feedback when no tests have been selected after submit.

Sorry, I finally understood now that you were trying to report a bug "When I do not select any items, I do not get an error message." - That wasn't clear to me; I thought you were talking about the message string of the error message.

→ Fixed now. I had previously tested that and it worked, until I adjusted #type table to account for #tree FALSE to fix the Simpletest module test failures... However, the tableselect form elements cannot live at the top-level of a form, because the form validation handler of the parent #type table element is not able to determine whether any input was submitted otherwise. Added corresponding docs for that.

1. Reverted #tree FALSE changes. Not supported for #tableselect, because validation handler is not able to detect missing input otherwise.
2. Added docs for #for property handling to theme_form_element_label().
3. Restored div.description markup and CSS.

Sorry, forgot to clean up one detail:

Moved stale help text into an actual hook_help() text.

54: table.simpletest.53.patch queued for re-testing.

Random testbot OS failure. The new/final patch is green.

Great!

1. Manually tested and selected tests run.
2. The styles look good and haven't changed.
3. "No items selected. " vs "No test(s) selected. " changed but the wording on that is less important than the fact that there is feedback!
4. The accessibility is much better with the aria and label placement!
5. Performance is still horrid but I'll leave that up to the committers to decide on that bit see rational in #41 and results in #45.

Sorry, one more thing I noticed the collapsible fieldset got dropped. Though this usually wouldn't be a huge issue, it's kinda nice to collapse the set from the top so you can get at that annoyingly placed "Clean environment" button. If possible it would be nice to leave that in.

As briefly mentioned in IRC:

1. While it is possible to revert to HEAD, that details element was an interface design disorder in the first place: The list of tests is the primary interaction control on that page, they are not details. Details are reserved for optional, well, details. ;)
2. In terms of UX, I never collapsed the details in order to reach the "Clean environment" button — that possibility never even occurred to me. In the rare cases I had to, I always scrolled down to the end of the page. Did you get a habit of collapsing the details element?
3. In any case, I agree that this button should be located somewhere else; possibly somewhere at the top - though ideally, it should not exist in the first place, because I never understood the point of retaining the results of a fatal'ed test to begin with, since whatever you get is data garbage. The test runner should automatically clean up after itself. Only after #2171683: Remove all Simpletest overrides and rely on native multi-site functionality instead, there might be a point in doing so via #2176023: Allow to keep artifacts after a test run (test site + database), but that will introduce an explicit configuration option for doing so.

In short, I think the current patch not only converts to #type table, it also fixes the accessibility of #type table in general, and on top, it fixes the UX/usability of the main interaction of this page in accordance to our usability guidelines. Let's move forward :)

Further improvements to Simpletest's UI are very well possible and if you have any ideas, I'd be more than eager to hear + help!

Regarding front-end and total RTT performance, the next best step for this page is to get rid of 99% of the JavaScript settings, which are apparently duplicating ~40% of the page content in JSON, whereas there is absolutely no reason for why the JS on this page should need any kind of settings. Eliminating that would be a wonderful contribution. :)

I think we need two core committers in this issue for UX changes @webchick and for performance regression @catch.

I can suck it up and use the "end" key on my keyboard for now, but maybe we can get that button in a better position in a follow-up? I didn't really notice that toggle "feature" until I tested it anyway, always just gripped in my head at it being at the bottom as I abused my scroll wheel.

Re: the 'details' tag, I agree that may be the intended use-case but if that's how we do the collapsible UI then, it doesn't matter the tag (unless there is another UI element that does collapsible elements that isn't details tag, then we can just use that) It's not the tag's fault we used it for that UI feature...

Also agree with @jibran two committers would be nice:)

This is back to RTBC, thanks for addressing the questions and hopefully we can do that UI regression/improvement in a follow-up.

Thanks! :)

And, sorry, as the original author, I can't leave that statement without correction:

Re: the 'details' tag, … that's how we do the collapsible UI then, it doesn't matter the tag … It's not the tag's fault we used it for that UI feature.

Support for the HTML5 Details element was explicitly added in Drupal 8 to replace our previous "collapsible fieldsets" with a clear and concise user interface design component and concept (which isn't invented by Drupal):

#1168246: Freedom For Fieldsets! Long Live The DETAILS.

This particular HTML element has a (semantic) meaning. Any usage that does not comply with its meaning not only violates the Drupal user interface and usability guidelines, it also violates HTML5 standards.

The situation that Drupal core is still using details elements in (too) many places is solely caused by the fact that it was not possible to evaluate every single instance and usage of all former collapsible fieldsets in the conversion. → Just because a form in core happens to contain a details element right now does not mean that its usage is correct.

In fact, as far as I can see, progress on the full conversion + individual UI evaluations still appears to be in the same state it was when I took a break from Drupal core a year ago:

#1852104: #type details: Remove #collapsible property
#1892182: #type details: Rename #collapsed to #open
#1839158: Replace collapse.js with a proper polyfill for <details>

Not sure if/when I'll find time to work on that, but thanks for the reminder! :-)

Thanks for pointing me to that, another issue to lurk on, I like that #open. Still stand by my statement. If there is a need for collapsible elements and details is not the correct tag for the job, then maybe we need a div based one. This is not a good example as the UI just needs to change... but I'm sure there is a case out there, anyway contrib can deal with that or they will just abuse details as we did here:)

table.simpletest.53.patch no longer applies.

error: patch failed: core/modules/simpletest/lib/Drupal/simpletest/Tests/BrokenSetUpTest.php:77
error: core/modules/simpletest/lib/Drupal/simpletest/Tests/BrokenSetUpTest.php: patch does not apply
error: patch failed: core/modules/simpletest/lib/Drupal/simpletest/Tests/MissingCheckedRequirementsTest.php:55
error: core/modules/simpletest/lib/Drupal/simpletest/Tests/MissingCheckedRequirementsTest.php: patch does not apply

Simple diff context conflicts, clean merge.

table.simpletest.66.patch no longer applies.

error: patch failed: core/modules/simpletest/simpletest.module:26
error: core/modules/simpletest/simpletest.module: patch does not apply

Simple diff context conflicts, clean merge.

68: table.simpletest.68.patch queued for re-testing.

68: table.simpletest.68.patch queued for re-testing.

This blocks some key theme DX issues. Up priority.

Merged 8.x.

72: table.simpletest.72.patch queued for re-testing.

Good little clean-up to bring things more inline. Committed to 8.x.

Sorry, follow-up: #2207341: "Re-run tests" form action broken