Task

Convert the following theme functions to use the new table #type:

Module Theme function name Where in Code What is it really?
node theme_node_search_admin function form table

Changes in this patch

- theme_node_search_admin() is removed (along with the theme hook "node_search_admin").
- This theme function was only used for theming the content ranking section of the config page for the NodeSearch plugin in NodeSearch::buildConfigurationForm(). That function is refactored to use a regular #type = table render array instead of #theme = node_search_admin.
- In the process of doing that, the form elements for the weights of the rankings changed their HTML element names in the markup, so NodeSearch::submitConfigurationForm() also changed how it reads the ranking values for saving in the config, and SearchRankingTest also had to change how it submitted these forms.
- An Aria label is added to the ranking HTML selects, for accessibility.

Manual testing steps

  1. Enable the Search module
  2. Visit admin/config/search/pages/manage/node_search to review the generated HTML.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because we only change the way the markup for an admin page is generated.
Issue priority Major because parent meta issue #1876712: [meta] Convert all tables in core to new #type 'table' is major.
Unfrozen changes Unfrozen because it only changes the way markup is being printed
Prioritized changes This is not a prioritized change for the beta phase.
Disruption Non disruptive for core because we only change way markup is being printed. It also improves accessibility by providing aria-labels instead of hidden

Related issues

CommentFileSizeAuthor
#86 interdiff.txt741 byteslauriii
#86 1938920-convert-node-tables-85.patch7 KBlauriii
#81 interdiff-1938920-68-81.txt739 bytesakalata
#81 1938920-convert-node-tables-81.patch7 KBakalata
#69 Kaleidoscope_–_1938920-before-whitespaceless_txt___1938920-after-whitespaceless_txt.png231.17 KBjoelpittet
#69 1938920-before-whitespaceless.txt7.81 KBjoelpittet
#69 1938920-after-whitespaceless.txt6.54 KBjoelpittet
#68 interdiff-1938920-67-68.txt706 bytesakalata
#68 1938920-convert-node-tables-68.patch7 KBakalata
#67 interdiff-1938920-62-67.txt2.24 KBakalata
#67 1938920-convert-node-tables-67.patch6.97 KBakalata
#62 interdiff-1938920-60-62.txt550 bytesakalata
#62 1938920-convert-node-tables-62.patch6.53 KBakalata
#60 interdiff-1938920-56-60.txt2.08 KBakalata
#60 1938920-convert-node-tables-60.patch6.56 KBakalata
#59 1938920-convert-node-tables-59.patch6.58 KBakalata
#56 1938920-56.patch6.57 KBakalata
#44 interdiff-1938920-42-44.txt880 bytesakalata
#44 1938920-node-convert-tables-44.patch6.57 KBakalata
#43 interdiff-40-42.txt2.9 KBmartin107
#42 1938920-node-convert-tables-42.patch6.02 KBakalata
#40 1938920-node-convert-tables-reroll-40.patch6.77 KBakalata
#36 convert-node-tables-1938920-33.patch6.87 KBnicholasruunu
#32 convert-node-tables-1938920-32.patch6.14 KBJon Pugh
#29 convert-node-tables-1938920-29-do-not-test.patch3.69 KBJon Pugh
#28 convert-node-tables-1938920-28.patch3.72 KBJon Pugh
#25 convert-node-tables-1938920-25.patch2.08 KBJon Pugh
#22 convert-node-tables-1938920-22.patch3.06 KBJon Pugh
#4 convert-node-tables-1938920-3.patch3.47 KBshanethehat
#3 convert-node-tables-1938920-3.patch3.47 KBshanethehat
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

shanethehat’s picture

Assigned: Unassigned » shanethehat
shanethehat’s picture

I'm not sure if this one needs converting, at least not to the table type. The form is already using type 'detail', so converting it to table removes the detail markup. Is there a way to add the form as a child to the detail type that I'm missing?

shanethehat’s picture

I've done an initial conversion, and the markup seems to match, especially the name attributes of the form elements. The form does not save the content ranking values, but I have tested the original form on a clean install, and that does not seem to save the values either.

The only differences I've seen in the markup are:

  1. The table now has an id attribute.
  2. The help text now appears, it was incorrectly keyed as #info instead of #markup in the original theme function.

Before:

<details class="form-wrapper collapse-processed" open="open" id="edit-content-ranking"><summary role="button" aria-controls="edit-content-ranking" aria-expanded=""><a href="#edit-content-ranking" class="details-title"><span class="details-summary-prefix element-invisible">Show</span> Content ranking</a><span class="summary"></span></summary><div class="details-wrapper"><div class="tableresponsive-toggle-columns"><button style="display: none;" title="Show table cells that were hidden to make the table fit within a small screen." type="button" class="link tableresponsive-toggle">Hide unimportant columns</button></div><table style="visibility: hidden; position: fixed; top: 79px; left: 53px; width: 1198px;" class="sticky-header"><thead><tr><th style="width: 915px; display: table-cell;">Factor</th><th style="width: 240px; display: table-cell;">Weight</th> </tr></thead></table><table class="sticky-enabled responsive-enabled tableresponsive-processed tableheader-processed sticky-table">
 <thead><tr><th>Factor</th><th>Weight</th> </tr></thead>
<tbody>
 <tr class="odd"><td>Number of comments</td><td><div class="form-item form-type-select form-item-node-rank-comments">
 <label class="element-invisible" for="edit-node-rank-comments">Number of comments</label> <select id="edit-node-rank-comments" name="node_rank_comments" class="form-select"><option value="0" selected="selected">0</option><option value="1">1</option><option value="2">2</option><option value="3">3</option><option value="4">4</option><option value="5">5</option><option value="6">6</option><option value="7">7</option><option value="8">8</option><option value="9">9</option><option value="10">10</option></select>
</div>
</td> </tr>
 <tr class="even"><td>Keyword relevance</td><td><div class="form-item form-type-select form-item-node-rank-relevance">
 <label class="element-invisible" for="edit-node-rank-relevance">Keyword relevance</label> <select id="edit-node-rank-relevance" name="node_rank_relevance" class="form-select"><option value="0" selected="selected">0</option><option value="1">1</option><option value="2">2</option><option value="3">3</option><option value="4">4</option><option value="5">5</option><option value="6">6</option><option value="7">7</option><option value="8">8</option><option value="9">9</option><option value="10">10</option></select>
</div>
</td> </tr>
 <tr class="odd"><td>Content is sticky at top of lists</td><td><div class="form-item form-type-select form-item-node-rank-sticky">
 <label class="element-invisible" for="edit-node-rank-sticky">Content is sticky at top of lists</label> <select id="edit-node-rank-sticky" name="node_rank_sticky" class="form-select"><option value="0" selected="selected">0</option><option value="1">1</option><option value="2">2</option><option value="3">3</option><option value="4">4</option><option value="5">5</option><option value="6">6</option><option value="7">7</option><option value="8">8</option><option value="9">9</option><option value="10">10</option></select>
</div>
</td> </tr>
 <tr class="even"><td>Content is promoted to the front page</td><td><div class="form-item form-type-select form-item-node-rank-promote">
 <label class="element-invisible" for="edit-node-rank-promote">Content is promoted to the front page</label> <select id="edit-node-rank-promote" name="node_rank_promote" class="form-select"><option value="0" selected="selected">0</option><option value="1">1</option><option value="2">2</option><option value="3">3</option><option value="4">4</option><option value="5">5</option><option value="6">6</option><option value="7">7</option><option value="8">8</option><option value="9">9</option><option value="10">10</option></select>
</div>
</td> </tr>
</tbody>
</table>
</div></details>

After:

<details class="form-wrapper collapse-processed" open="open" id="edit-content-ranking"><summary role="button" aria-controls="edit-content-ranking" aria-expanded=""><a href="#edit-content-ranking" class="details-title"><span class="details-summary-prefix element-invisible">Show</span> Content ranking</a><span class="summary"></span></summary><div class="details-wrapper"><em>The following numbers control which properties the content search should favor when ordering the results. Higher numbers mean more influence, zero means the property is ignored. Changing these numbers does not require the search index to be rebuilt. Changes take effect immediately.</em><div class="tableresponsive-toggle-columns"><button style="display: none;" title="Show table cells that were hidden to make the table fit within a small screen." type="button" class="link tableresponsive-toggle">Hide unimportant columns</button></div><table style="visibility: hidden; position: fixed; top: 79px; left: 53px; width: 1198px;" class="sticky-header"><thead><tr><th style="width: 915px; display: table-cell;">Factor</th><th style="width: 240px; display: table-cell;">Weight</th> </tr></thead></table><table id="edit-factors" class="sticky-enabled responsive-enabled tableresponsive-processed tableheader-processed sticky-table">
 <thead><tr><th>Factor</th><th>Weight</th> </tr></thead>
<tbody>
 <tr class="odd"><td>Number of comments</td><td><div class="form-item form-type-select form-item-node-rank-comments">
 <label class="element-invisible" for="edit-node-rank-comments">Number of comments</label> <select id="edit-node-rank-comments" name="node_rank_comments" class="form-select"><option value="0" selected="selected">0</option><option value="1">1</option><option value="2">2</option><option value="3">3</option><option value="4">4</option><option value="5">5</option><option value="6">6</option><option value="7">7</option><option value="8">8</option><option value="9">9</option><option value="10">10</option></select>
</div>
</td> </tr>
 <tr class="even"><td>Keyword relevance</td><td><div class="form-item form-type-select form-item-node-rank-relevance">
 <label class="element-invisible" for="edit-node-rank-relevance">Keyword relevance</label> <select id="edit-node-rank-relevance" name="node_rank_relevance" class="form-select"><option value="0" selected="selected">0</option><option value="1">1</option><option value="2">2</option><option value="3">3</option><option value="4">4</option><option value="5">5</option><option value="6">6</option><option value="7">7</option><option value="8">8</option><option value="9">9</option><option value="10">10</option></select>
</div>
</td> </tr>
 <tr class="odd"><td>Content is sticky at top of lists</td><td><div class="form-item form-type-select form-item-node-rank-sticky">
 <label class="element-invisible" for="edit-node-rank-sticky">Content is sticky at top of lists</label> <select id="edit-node-rank-sticky" name="node_rank_sticky" class="form-select"><option value="0" selected="selected">0</option><option value="1">1</option><option value="2">2</option><option value="3">3</option><option value="4">4</option><option value="5">5</option><option value="6">6</option><option value="7">7</option><option value="8">8</option><option value="9">9</option><option value="10">10</option></select>
</div>
</td> </tr>
 <tr class="even"><td>Content is promoted to the front page</td><td><div class="form-item form-type-select form-item-node-rank-promote">
 <label class="element-invisible" for="edit-node-rank-promote">Content is promoted to the front page</label> <select id="edit-node-rank-promote" name="node_rank_promote" class="form-select"><option value="0" selected="selected">0</option><option value="1">1</option><option value="2">2</option><option value="3">3</option><option value="4">4</option><option value="5">5</option><option value="6">6</option><option value="7">7</option><option value="8">8</option><option value="9">9</option><option value="10">10</option></select>
</div>
</td> </tr>
</tbody>
</table>
</div></details>
shanethehat’s picture

Status: Active » Needs review
FileSize
3.47 KB

Same patch, with the right status

Status: Needs review » Needs work

The last submitted patch, convert-node-tables-1938920-3.patch, failed testing.

shanethehat’s picture

Status: Needs work » Needs review

#4: convert-node-tables-1938920-3.patch queued for re-testing.

shanethehat’s picture

Status: Needs review » Postponed

Hmm, so maybe this should be postponed until #1831632: Convert node_rank_ $rank variables to use configuration system lands, since that will change the way the rankings are retrieved in node_search_admin().

jibran’s picture

Issue tags: +Novice

Tagging.

Jon Pugh’s picture

The data does not with patch in #4.

Jon Pugh’s picture

Status: Postponed » Needs work
Jon Pugh’s picture

Title: Convert node theme tables to table #type » Convert node_search_admin theme tables to table #type
Assigned: shanethehat » Jon Pugh

I am re-rolling #1831632: Convert node_rank_ $rank variables to use configuration system with this patch at the same time, since they depend on one another.

Jon Pugh’s picture

Status: Needs work » Postponed

Patch queued for testing over at #1831632: Convert node_rank_ $rank variables to use configuration system contains the work done here. They are so closely tied it was much easier to include both in the same patch.

tstoeckler’s picture

Status: Postponed » Closed (duplicate)

So this should be duplicate then?

Petr Illek’s picture

Status: Closed (duplicate) » Needs work
ianthomas_uk’s picture

Status: Needs work » Postponed

I'm working on #1831632: Convert node_rank_ $rank variables to use configuration system and hope to have a patch ready to commit within the next week or so. To avoid conflicting patches, please check the status of that issue before working on this one.

ianthomas_uk’s picture

Issue summary: View changes

Removing theme_node_recent_block()

xjm’s picture

Component: node.module » node system
Issue summary: View changes

(Merging "node system" and "node.module" components for 8.x; disregard.)

star-szr’s picture

Assigned: Jon Pugh » Unassigned
Status: Postponed » Active

Looks like this can probably be re-activated and that possibly this has been partially converted, current code:

  $table = array(
    '#type' => 'table',
    '#header' => $header,
    '#rows' => $rows,
  );

So this might just be a matter of moving this #type table out into \Drupal\node\Plugin\Search\NodeSearch::buildConfigurationForm() and removing the theme function.

star-szr’s picture

Issue tags: +Twig

And tagging so we can keep track of this on http://drupaltwig.org.

martin107’s picture

Issue tags: +Needs reroll

Patch no longer applies.

star-szr’s picture

Issue tags: -Needs reroll

I don't think a reroll is applicable here.

Jon Pugh’s picture

Assigned: Unassigned » Jon Pugh

I'll start on this one again. #NYCcamp

Jon Pugh’s picture

Done!

So simple... $type table assumes all element_children are rows, and element_children of that are columns.

Jon Pugh’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 22: convert-node-tables-1938920-22.patch, failed testing.

Jon Pugh’s picture

Ok, should be all fixed up.

I refactored the naming of the forms to remain consistent with the config.

martin107’s picture

Status: Needs work » Needs review

triggering testbot

Status: Needs review » Needs work

The last submitted patch, 25: convert-node-tables-1938920-25.patch, failed testing.

Jon Pugh’s picture

Status: Needs work » Needs review
FileSize
3.72 KB

Rerolled.

Jon Pugh’s picture

Adding newline at end of file.

Status: Needs review » Needs work

The last submitted patch, 28: convert-node-tables-1938920-28.patch, failed testing.

Jon Pugh’s picture

Adjusting the test now.

Stand by for patch.

Jon Pugh’s picture

Updated the field identifiers, but there's an error happening that I can't figure out:

Fatal error: Call to a member function id() on a non-object in /home/jon/Repos/drupal/core/modules/search/lib/Drupal/search/Tests/SearchRankingTest.php on line 133
 $this->assertEqual($set[0]['node']->id(), $nodes[$node_rank][1]->id(), 'Search ranking "' . $node_rank . '" order.');
Jon Pugh’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 32: convert-node-tables-1938920-32.patch, failed testing.

nicholasruunu’s picture

Assigned: Jon Pugh » nicholasruunu

I'll try to make the patch work.

nicholasruunu’s picture

Reroll patch

martin107’s picture

Status: Needs work » Needs review

Triggering testbot

Status: Needs review » Needs work

The last submitted patch, 36: convert-node-tables-1938920-33.patch, failed testing.

akalata’s picture

Assigned: nicholasruunu » akalata
Issue tags: +TCDrupal 2014, +needs re-roll
akalata’s picture

Status: Needs work » Needs review
FileSize
6.77 KB

attempted re-roll due to PSR-4 filepath changes (I think?) #2083547: PSR-4: Putting it all together

Status: Needs review » Needs work

The last submitted patch, 40: 1938920-node-convert-tables-reroll-40.patch, failed testing.

akalata’s picture

Status: Needs work » Needs review
FileSize
6.02 KB

Updating tests for search settings path change; also fixed error in reroll exposed by one of the failed tests.

martin107’s picture

FileSize
2.9 KB

I wanted to see the interdiff....

So here it is... I'm glad to see so many issues being updated at TCDrupal

akalata’s picture

Thanks martin107! I thought I posted the interdiff , must've gotten lost from all of us posting at once. :)

I've got a few questions about why SearchRankingTest.php has hardcoded rankings to test for on admin/config/search/pages/manage/node_search, when 2 of those items aren't enabled in the Standard install profile, but I'd like to post this patch and interdiff to get the 4 other items to hopefully pass.

The last submitted patch, 42: 1938920-node-convert-tables-42.patch, failed testing.

The last submitted patch, 42: 1938920-node-convert-tables-42.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 44: 1938920-node-convert-tables-44.patch, failed testing.

The last submitted patch, 44: 1938920-node-convert-tables-44.patch, failed testing.

akalata’s picture

In an attempt to stop uploading fixes to testbot to see if they pass, I tried running the SearchRankingsTest testRankings locally. I'm getting the error below, even on a fresh HEAD:

$php core/scripts/run-tests.sh --class 'Drupal\search\Tests\SearchRankingTest'

Drupal test run
---------------

Tests to be run:
  - Drupal\search\Tests\SearchRankingTest

Test run started:
  Sunday, August 10, 2014 - 20:42

Test summary
------------

Fatal error: Call to a member function getId() on a non-object in /.../core/modules/search/src/Tests/SearchRankingTest.php on line 127

Call Stack:
    0.0015     397720   1. {main}() /.../core/scripts/run-tests.sh:0
    0.5346   14255520   2. simpletest_script_run_one_test() /.../core/scripts/run-tests.sh:36
    0.5439   15589704   3. Drupal\simpletest\TestBase->run() /.../core/scripts/run-tests.sh:626
   16.5377   44440232   4. Drupal\search\Tests\SearchRankingTest->testRankings() /.../core/modules/simpletest/src/TestBase.php:860

FATAL Drupal\search\Tests\SearchRankingTest: test runner returned a non-zero error code (255).
- Found database prefix 'simpletest774566' for test ID 2.
[10-Aug-2014 20:42:46 America/Chicago] PHP Fatal error:  Call to a member function getId() on a non-object in /.../core/modules/search/src/Tests/SearchRankingTest.php on line 127
[10-Aug-2014 20:42:46 America/Chicago] PHP Stack trace:
[10-Aug-2014 20:42:46 America/Chicago] PHP   1. {main}() /.../core/scripts/run-tests.sh:0
[10-Aug-2014 20:42:46 America/Chicago] PHP   2. simpletest_script_run_one_test() /.../core/scripts/run-tests.sh:36
[10-Aug-2014 20:42:46 America/Chicago] PHP   3. Drupal\simpletest\TestBase->run() /.../core/scripts/run-tests.sh:626
[10-Aug-2014 20:42:46 America/Chicago] PHP   4. Drupal\search\Tests\SearchRankingTest->testRankings() /.../core/modules/simpletest/src/TestBase.php:860

- Removed test site directory.
- Removed 43 leftover tables.

Test run duration: 17 sec

I looked at #1919358: Tests failing on local dev environment with 8.x HEAD and clean DB. and checked my file permissions, but that doesn't appear to have any affect.

tim.plunkett’s picture

Issue tags: -needs re-roll +Needs reroll
martin107’s picture

Issue tags: -Needs reroll

reroll tag was stale

patch applied cleanly against head :-

commit d21c9a4e2e95477865cec3da1d3faad18d7cd5ae
Author: Alex Pott
Date: Sun Aug 10 17:38:02 2014 -0500

Issue #2313615 by tim.plunkett: Clarify the deprecated-ness of ArrayAccess usage in FormState.

martin107’s picture

@akalata

When I repeat your command

php core/scripts/run-tests.sh --class 'Drupal\search\Tests\SearchRankingTest'

I get the same result as you, but I suspect the problem is the command. Most notably when I include the --url to my site everything passes, no fails. :)

php core/scripts/run-tests.sh --php /Applications/MAMP/bin/php/php5.5.10/bin/php --color --die-on-fail --url http://dev.drupal.co.uk --verbose --file ./core/modules/search/src/Tests/SearchRankingTest.php

All the other options are a distraction, I think but am including them just for reference.

akalata’s picture

Assigned: akalata » Unassigned
star-szr’s picture

Assigned: Unassigned » star-szr
Issue tags: -Novice

No promises, but I'm going to try and figure this one out.

akalata’s picture

Assigned: star-szr » akalata

Taking this over @ Fox Valley Drupal Camp sprint. I'm determined to get this done! Still no luck with running tests via command line, but I found the GUI. :)

akalata’s picture

FileSize
6.57 KB

Reroll #44.

While trying to fix testRankings() I realized that there's actually something wrong in submitConfigurationForm() from some earlier point. Working with @YesCT to debug that as well.

martin107’s picture

Status: Needs work » Needs review

Triggering testbot.

Status: Needs review » Needs work

The last submitted patch, 56: 1938920-56.patch, failed testing.

akalata’s picture

Thanks @martin, I knew it would fail which is why I didn't trigger it. :) Was just sharing my progress up to that point.

Here's another progress marker -- fixing the form submission (at admin/config/search/pages/manage/node_search), which was an error I made in an earlier re-roll. Now working on fixing the test.

akalata’s picture

Status: Needs work » Needs review
FileSize
6.56 KB
2.08 KB

And I didn't think I'd get the tests working this quickly! It's amazing what hours spent figuring out docs and getting the debugger working will help you get done! :)

star-szr’s picture

Woohoo green! Awesome work.

+++ b/core/modules/node/src/Plugin/Search/NodeSearch.php
@@ -554,17 +554,25 @@ public function buildConfigurationForm(array $form, FormStateInterface $form_sta
+      $form['content_ranking']['rankings'][$var]['name'] = array(
+        '#markup' => $values['title'],
+        '#type' => 'markup',
+      );

I don't think #type markup is needed. See https://api.drupal.org/api/drupal/developer%21topics%21forms_api_referen....

akalata’s picture

A better link might be #2125043: Remove '#type' => 'markup' where all other instances were removed from D8 core. :) So in that case...

akalata’s picture

Assigned: akalata » Unassigned
akalata’s picture

Issue tags: +Needs manual testing
joelpittet’s picture

Here's a code review from @a-fro and me. We are going to manual test this now!

  1. +++ b/core/modules/node/src/Plugin/Search/NodeSearch.php
    @@ -554,17 +554,24 @@ public function buildConfigurationForm(array $form, FormStateInterface $form_sta
    +    $header = array(t('Factor'), t('Influence'));
    

    This should be the injected t method because we can:) $this->t(). Also maybe we can use the short array syntax? []

  2. +++ b/core/modules/node/src/Plugin/Search/NodeSearch.php
    @@ -578,8 +585,8 @@ public function buildConfigurationForm(array $form, FormStateInterface $form_sta
    +      if (!$form_state->isValueEmpty(array('rankings', $var, 'value'))) {
    

    Can we change these to the new array syntax too, not required obvious, but seems to be the preference.

joelpittet’s picture

After we did manually testing we discovered the missing visually hidden label for the select element.

+++ b/core/modules/node/src/Plugin/Search/NodeSearch.php
@@ -554,17 +554,25 @@ public function buildConfigurationForm(array $form, FormStateInterface $form_sta
+        '#markup' => $values['title'],
...
+      );
+      $form['content_ranking']['rankings'][$var]['value'] = array(
         '#type' => 'select',
         '#options' => $options,
         '#default_value' => isset($this->configuration['rankings'][$var]) ? $this->configuration['rankings'][$var] : 0,

There was a hidden label for accessibility here, we can replace this with an 'aria-label' which @jessebeach suggested and I'd prefer(less markup).

akalata’s picture

Yay feedback!

Here are edits based on #65 and #66, plus the removal of node_search_admin from node_theme that I discovered after upgrading my local machine to PHP 5.5.

akalata’s picture

Based on in-person feedback from @joelpittet, making the aria-label we added translatable.

joelpittet’s picture

Thank you @akalata and @a-fro, this looks great and still accessible!

YesCT’s picture

akalata’s picture

Issue summary: View changes
akalata’s picture

Issue summary: View changes
akalata’s picture

Issue tags: +Accessibility

YesCT suggesting tagging with accessibility, and to mention that dawehner suggests that the conversion to #type=table is a security improvement, though I'm not familiar enough with the existing theme function to confirm.

mgifford’s picture

Just looking to the standards first for reference:
http://www.w3.org/TR/wai-aria/roles#tac_example2

Also other references:
http://msdn.microsoft.com/en-us/library/windows/desktop/jj658627%28v=vs....

This looks fine when comparing it to those references.

Note: Tested in VoiceOver & ChromeVox. Works as expected.

joelpittet’s picture

Thank you @mgifford for the accessibility test!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

the aria-label of "Influence of Number of comments" has interesting capitalisation.

+++ b/core/modules/node/src/Plugin/Search/NodeSearch.php
@@ -554,19 +554,27 @@ public function buildConfigurationForm(array $form, FormStateInterface $form_sta
+        '#attributes' => ['aria-label' => $this->t('Influence of @title', ['@title' => $values['title']])],

Also I don't think that this will work for translation.

joelpittet’s picture

@mgifford what do you think, should we lowercase the title or just remove the prefix extra 'Influence of '?

jhodgdon’s picture

I was asked to comment here... since I just came to this issue, it's a bit confusing. The issue summary just says it's trying to get rid of this custom theme function, but the patch seems to be doing a number of other changes that are not explained in the summary or code comments. Maybe an issue summary update would be useful?

Regarding the aria label... It seems like the right fix would be to make the labels something like:

Influence of "Number of comments"

rather than removing the "Influence of" part.

joelpittet’s picture

Thank you @jhodgdon, that sounds like a good idea

Influence of "Number of comments"

That change is so we don't have to create a label and have lighter markup and better accessibility.

The other changes that seem evident are just key changes due to the structure of the #type table. It's kind of hacky either way, you change the keys or you change #parents to fake the keys you expect... it's a toss up in my opinion, do you have a preference?

jhodgdon’s picture

It seems like less markup and fewer levels of key parents is a good idea.

akalata’s picture

I've added single quotes around the ranking variable name, since I was worried that double quotes would affect the aria-label="".

I've also tested these labels in multiple interface languages, and I'm not seeing any issues with the @title substitution.

I didn't quite follow the conversation in 79/80, so I didn't change any keys.

akalata’s picture

Status: Needs work » Needs review

The last submitted patch, 59: 1938920-convert-node-tables-59.patch, failed testing.

jhodgdon’s picture

Issue summary: View changes
Status: Needs review » Needs work

I updated the issue summary and added a section explaining the changes I found in this patch. Please check it over. After looking at this, I think I'd actually be more comfortable with leaving the HTML markup for the input elements as it was. There is at least one contrib module that extends this plugin (Display Suite) and I think it would just be better at this stage not to change the HTML markup for the form element used for rankings. This could affect contrib modules and their tests, and I don't think the change is necessary for this patch.

Also, one small nitpick:

+        '#attributes' => ['aria-label' => $this->t('Influence of \'@title\'', ['@title' => $values['title']])],

When you make a string with ' inside it, it is much better to use "... 'something' ..." than ' ... \'something\'.... So this line should be changed. It's more readable and also that's our coding standard.

jhodgdon’s picture

Component: node system » search.module

I'm also changing the component to search.module. Normally everything related to Core Search goes in that component. As you can see, all the tests are in that module, and people check that component for search-related stuff.

lauriii’s picture

Tested this manually and everything seems to work. This looks good to me.

jhodgdon’s picture

Status: Reviewed & tested by the community » Needs review

I disagree with the RTBC. I do not think at this stage we should change the element names in the HTML markup for the select lists. This patch could achieve its aims without making that change. See #84.

star-szr’s picture

From what I recall working on/reviewing other #type table issues there is little to no control over what name and id attributes you end up with, they are automatically generated based on the array structure and the -value is automatically appended to the ID. So the slight changes to markup have usually been accepted as part of the conversion process. I can't find the code (or chain of code) right now that is responsible for those changes.

This is RTBC in my opinion but I don't want to start that game. It's worth mentioning that markup is not frozen until RC1: https://www.drupal.org/core/beta-changes#unfrozen

star-szr’s picture

Or put another way: the markup here is not being changed intentionally, it's changing as a result of converting to #type table.

jhodgdon’s picture

Really?

-      $form['content_ranking']['factors']["rankings_$var"] = array(
-        '#title' => $values['title'],
+      $form['content_ranking']['rankings'][$var]['name'] = array(
+        '#markup' => $values['title'],
+      );
+      $form['content_ranking']['rankings'][$var]['value'] = array(
         '#type' => 'select',
         '#options' => $options,
+        '#attributes' => ['aria-label' => $this->t("Influence of '@title'", ['@title' => $values['title']])],
         '#default_value' => isset($this->configuration['rankings'][$var]) ? $this->configuration['rankings'][$var] : 0,
       );
     }

It looks to me as though it used to be $form['content_ranking']['factors']['rankings_$var'] and now it's $form['content_ranking']['rankings'][$var]['value'] . Both are hard-wired, it's just hard-wired differently now.

This is not just markup; it's also affecting the value structure of $form_state.

star-szr’s picture

I'm not sure if it's possible to preserve the $form_state structure within the constraints of #type table but I'm happy to be proven wrong :)

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Oh, I see... I was not understanding the reason for the markup change. OK. I'm OK with it now. Sorry for that!

So, before this can be committed:
- We need a draft change record
- We need the "beta policy" section in the issue summary updated. See https://www.drupal.org/contribute/core/beta-changes -- and I don't think it's actually true that there is no disruption, as that section currently states, because the markup has changed and $form_state, so potentially modules extending the NodeSearch class could be affected.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record

This issue is a normal task so we need to outline how it fits within the allowable Drupal 8 beta criteria. Can someone add Drupal 8 beta phase evaluation template to the issue summary.

Also #92 points out we need a CR.

jhodgdon’s picture

Oh sorry, I should not have set it to RTBC without those items that are outlined in #93/#93 being present.

lauriii’s picture

Priority: Normal » Major
Issue summary: View changes
lauriii’s picture

Issue summary: View changes
YesCT’s picture

Issue summary: View changes

adding remaining tasks section to the summary (with link to how to draft a change record)

jhodgdon’s picture

Issue summary: View changes

Fixed missing end of phrase in beta eval section in summary.

Still needs draft change record.

joelpittet’s picture

Status: Needs work » Needs review
Issue tags: -Needs change record

Added a draft change record. Hopefully that covers enough?

https://www.drupal.org/node/2385285

jhodgdon’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Looks pretty good. I tweaked the wording a bit.

All tasks have been done now, so back to RTBC. Thanks!

joelpittet’s picture

Very nice tweaking:) Thanks.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed d07e48a and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation for to the issue summary.

  • alexpott committed d07e48a on 8.0.x
    Issue #1938920 by akalata, Jon Pugh, joelpittet, shanethehat, lauriii,...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.