The page listing all tests ( /admin/config/development/testing ) has duplicated id's in the HTML sent, which is against W3C spec. I do not know of any existing problems caused by this violation, but they are possible as the return of the function document.getElementById is undefined by spec when more than one id is present (currently I think all browsers return the first instance on the page). So this could cause problems for the JavaScript behaviors of the page. This was discovered by the unique assertion in AttributeNamedId in #2444003: Optimize Drupal\Core\Template\Attribute.
This occurs because we have test groups with different capitalization cases. The first example on the page is "Action", which is out of the Drupal\system\Tests\Action namespace, and "action" which is from the action module at Drupal\action. So having the collision is natural - the keys don't collide when the modules are pulled because PHP keys are case sensitive.
CSS identifiers however are not case sensitive and so when the code prior to this patch creates the group_class name it uses strtolower. This creates the id collision and the bug.
The output of $group_class is never user visible - it's a reference used by JavaScript only - so there is no reason to build it using strtolower(preg_replace()) in the first place. Therefore I set an increment variable before the for/each which builds the array of modules, insuring all modules will have a unique id. This is also has the advantage of being less computationally expensive than the regex.
Final reviews
None
None
| Issue category | Bug |
|---|---|
| Issue priority | Major - one of the goals of 8 is to be have fully valid output HTML. This bug prevents this. |
| Unfrozen changes | Unfrozen because it only changes the HTML markup of the offending page. |
| Comment | File | Size | Author |
|---|---|---|---|
| #11 | 2477717-11.diff | 1.3 KB | Aki Tendo |
| #8 | 2477717-7.diff | 2.43 KB | Aki Tendo |
| #5 | 2477717--5--do-not-test.diff | 1.12 KB | Aki Tendo |
| #4 | 2477717--4.diff | 1.12 KB | Aki Tendo |
| simpletestpage.diff | 1.58 KB | Aki Tendo |
Comments
Comment #1
joelpittet@Aki Tendo thanks for the report. Could you explain a bit more what the problems JS is having or which IDs are duplicating? Maybe we should fix the collisions instead of masking them?
Here's a quick review on the patch:
Replace "Insure" with "Ensure" and sentence needs to end in a period.
There should be no need to cast this to a string, it will implicitly cast it through the concatenation.
Example:
http://3v4l.org/6XaB5
And in general core mostly casts with (string) instead of the function from a cursory search as I haven't seen that function used much. 4 times strval() vs 402 for (string), so if you think it needs an explicit cast let's use (string).
Comment #2
Aki Tendo commentedExpanded both the problem explanation and the reason the solution is the way it is as requested in IRC with Joel.
Comment #3
Aki Tendo commentedComment #4
Aki Tendo commentedChanges above applied. Also, since $group_class is never end user visible, just drop the whole strtolower(preg_replace()) operation in favor of just using $i to keep each module group section unique.
Comment #5
Aki Tendo commentedForgot to add the period. This patch marked do not test since it is otherwise identical to #4 with the only change occurring in comment text.
Comment #6
Aki Tendo commentedIssue updated and properly formatted. Since the last patch passes it should be safe to just do away with the regex entirely.
Comment #7
joelpittetWow there is a bunch of cleanup on that test, so many duplicate names because of Titlecase or lower case group names.
The names were kinda useful before as a "group class", but useless as pointed out in the issue summary as an
#id.Since I very highly doubt anybody is going to go out of their way to style test groups differently in an admin theme. I see no problem with doing the $i increment identifier. Though it would make much more sense if the variable was changed to
$group_id, or something because it's no longer a CSS class or Test PHP class.Comment #8
Aki Tendo commentedChange applied. Forgoing uploading an interdiff since the main patch is so small anyway.
Comment #9
alexpottWhat are the duplicate IDs?
Comment #10
Aki Tendo commentedProbably over 100 - examples: module_action, module_block, module_config, module_datetime, module_field, module_file, module_image, etc. Basically any system test that also has tests in a core module. The culprit is the strtolower() call. While we could remove it, that would only solve the problem for Java Script -- CSS would still be affected since it is case insensitive. Also, having uppercase characters in id's violates code standards.
Comment #11
Aki Tendo commentedAs an alternative to an incrementor prefix the group as a module if its label is all lower case (which modules are supposed to be and all the core modules will be), otherwise prefix as 'core'
Comment #12
alexpottThis situation is caused by #2301481: Mark test groups as belonging to modules in UI
Comment #13
mile23Solution in #2301481-20: Mark test groups as belonging to modules in UI adds ' module' to module groups, maybe solving this issue, though it would be great to have a test.
Comment #17
Aki Tendo commentedComment #23
quietone commentedTriaging issues in simpletest.module as part of the Bug Smash Initiative to determine if they should be in the Simpletest Project or core.
This looks like it belongs in the Simpletest project.