Make test selection page work with non-JS [#308668]

When JavaScript is disabled, the test selection page is not accessible. (Neither is the test runner page, but that's a different beast.)

This small & handsome 5-liner does the following things:
- Makes all tests appear with JavaScript disabled, by using system.css's "js-hide" class instead of a naughty "display: none".
- Makes the expand/collapse button disappear for non-JS, loading the images only in simpletest.js (leaving them out in the original HTML).

Makes things a bit nicer, and is a precondition for my "test select box preservation" patch (upcoming soon).

Comment	File	Size	Author
#8	simpletest-selection-page-no-js-try2.patch	1.89 KB	jpetso
#8
#1	simpletest-selection-page-no-js.patch	2.11 KB	jpetso
#1
	simpletest-selection-page-no-js.patch	2.05 KB	jpetso

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

jpetso CreditAttribution: jpetso commented 15 September 2008 at 15:02

File	Size
simpletest-selection-page-no-js.patch	2.11 KB

Ahem. Wrong patch, the $collapsed variable will only appear later on. Here's the correct one.

Comment #2

boombatower CreditAttribution: boombatower commented 16 September 2008 at 04:05

Status:

Needs review

» Needs work

I would much prefer that #308272: Improve test selection page gets in first as this will be a much simpler re-roll.

This patch makes the checkboxes show up, but the tests don't run when selected. This was fixed in #250047: Rework the SimpleTest results interface and clean-up of backend code (at least I remember fixing it).

Please find the appropriate code and add it, I'm tired of all the related issues to breaking up patches, as stated in original issues. Too many wasted hours after people break my code.

Comment #3

jpetso CreditAttribution: jpetso commented 16 September 2008 at 07:50

Status:

Needs work

» Needs review

I have no problem with #308272: Improve test selection page going in first, but I respectfully disagree with your request to rescope this issue. The attached patch does one thing and does it well, in a small, easily committable way. While it doesn't solve all JavaScript related problems in SimpleTest (which was never my objective, this patch is just a side effect of #305150: Keep running tests checked), it's an obvious improvement to the current state. Incorporating the code from the rework/clean-up issue would make it multiple times as large, and while it would be nice to have, I don't see why the test runner page can't be fixed in a separate issue - the effort of including that code in here or in a separate patch is the same really.

I understand that splitting up and rerolling patches (like happened in the mentioned issue) is tiring, but that's a fact of life and affects me just as well as you. This is just five lines of code changes, any reroll incorporating that piece of change can't really take longer than 5 minutes. I can see that the work you put in your patch goes a long way and it's frustrating to see it not (yet) going in because of scope issues rather than quality ones. Still, saying that my patch should not be committed because it might affect your (also yet uncommitted) patch is unfair in my opinion, especially since I've also taken quite some time to split up a perfectly working patch into three separate pieces.

Please allow me to switch this issue back to "code needs review" as I'd like to hear what someone with less personal stakes involved (webchick, flobruit, ...) thinks of the proposed scope creep.

Comment #4

boombatower CreditAttribution: boombatower commented 16 September 2008 at 20:02

All I'm asking is that after causing all the time I've spent in addition to writing original code is that we keep this sensible. If the other goes in first (which it should soon) it is much less work to re-roll this. Since this portion of the original patch is not vital that would seem fair, but do with it as you wish.

I marked it as code needs work due to the fact that the title reads "Make test selection page work with non-JS", but it doesn't work with the patch applied and js turned off. As it is missing a bit of code, I believe it is 1 line (very small cause I remember adding it to original).

That would seem within the scope, at least according to the title, but once again I don't appear to have much say anymore since my patch has been split apart into 6 pieces and slowly made into things I have no say in.

At this point I have no idea if all the original changes in the patch are still in existence in other issues, which is exactly what happened before and why I was against all this. Now in addition to writing the original code I have to check after these are committed to see if all the changes end up getting in.

Comment #5

jpetso CreditAttribution: jpetso commented 16 September 2008 at 20:36

Assigned:	Unassigned	» jpetso
Status:	Needs review	» Needs work

Just one line? Ok, let me have a look.

I marked it as code needs work due to the fact that the title reads "Make test selection page work with non-JS", but it doesn't work with the patch applied and js turned off.

That's why I called it "Make test *selection* page work with non-JS", not "Make test pages work with non-JS" :P

Comment #6

boombatower CreditAttribution: boombatower commented 16 September 2008 at 21:25

Status:

Needs work

» Reviewed & tested by the community

The other issue will be delt with later as it would conflict with the "MASSIVE PATCH". :)

Comment #7

boombatower CreditAttribution: boombatower commented 1 October 2008 at 21:30

Status:

Reviewed & tested by the community

» Needs work

This needs a re-role then I would like to see this get in.

Comment #8

jpetso CreditAttribution: jpetso commented 2 October 2008 at 07:40

Status:

Needs work

» Needs review

File	Size
simpletest-selection-page-no-js-try2.patch	1.89 KB

Your wish is my command. Here's the re-rolled patch, applies to current HEAD, otherwise unchanged. Do I need to set the status to CNR again or can I go straight to RTBC like before?