buttons for reordering problems in a set details page

[Alex-Jordan]

When in the Set Details page, there will now be buttons on each problem to move a problem up/down in sequence. The mouse click-and-drag tool is still there, but these buttons give a keyboard accessible way to rearrange the problems.

If the set is a JITAR set, there are also buttons for indenting/outdenting problems.

Buttons are disabled when appropriate (like you can't move problem #1 up in the list).

[drgrice1]

Conceptually I think that this is okay. However, I have been thinking about working on this, and this is not how I would implement this. There are several problems with this approach.

One issue is that it involves numerous buttons, which there really isn't room for. This is what I see on a narrow window. I took it to the extreme to really emphasize the point in this screenshot, but it happens already even when the window is not nearly this narrow.

Another issue is that when you click on one of the up/down buttons for a problem the switch of problems is almost imperceivable. In most sets the settings are usually all the same except for the source file. So since the numbering is automatically updated (and that is correct to do), the only way you can see the change is by watching the source file.

Another issue that I am seeing has to do with the tooltips. I think that this can be fixed with this pull request and its design though. The issue is that if you click on one of the up/down buttons the tooltip is revealed and the problem is switched with the one above or below. The keyboard focus goes with the switch to where the original problem now is, but the tooltip stays where it was. This seems to confuse Bootstrap's javascript, and the tooltip never seems to get closed. In the extreme you can end up with numerous tooltips open at a time as in the following picture.

I have also noticed that one of those tooltips gets completely stuck, and there seems to be no way to actually get it to go away. It seems to be the one on the first button used. I believe this can be fixed by taking care to close the tooltips in the javascript when the button is clicked.

What I envisioned, and had planned to implement when I got around to it, was an approach similar to what is now implemented for the PG drag and drop in the dragndrop.js file for the DragNDrop.pm module. The existing move buttons would serve as the focus point. The mouse could still be used to drag those around as it currently is, but the arrow keys could also be used to move the problems around when the move button is focused. The up and down arrows clearly would move a problem up or down, and in a JITAR set the left and right arrows increase or decrease the nesting level. This would all be aria announce as it is for the PG drag and drop. One advantage of this approach over the implementation in this pull request is that if you use the down arrow repeatedly, you would be able to quickly move a single problem down several positions. If you want to move a problem all the way from the top of the list to bottom, you could just hold the down arrow down, and keyboard repeat would take the problem down quickly. Also, with the css transitions similar to those used in the PG drag and drop it would make the movement of the problem more clear.

[Alex-Jordan]

I'm happy to defer to your recommendations here. I could withdraw this, or feed what you have described here to Claude. I probably won't directly intervene with coding changes myself on this one though.

Whatever things here (and with the other PRs I've made today) that we can complete, it will affect the Accessibility Guide I'm trying to complete before the release. It's getting down to a pretty short list of known issues (as far as webwork2 goes, that is).

[drgrice1]

We can perhaps go with this for now, and then change to what I was thinking of later when I have time to work on that.

[Alex-Jordan]

OK. I'll see what Claude can do for the smaller tweaks you identified.

[Alex-Jordan]

The issues you found are at least mostly addressed now. The method I directed for the first issue (about overcrowding) is maybe not the best though.

[drgrice1]

I find the "indent"/"outdent" terminology a bit ugly. Also, in a JITAR set it isn't really indentation. It is nesting, and in fact the wording everywhere else calls it nesting.

[Alex-Jordan]

There is another problem with this. Say problem 1 and 2 use the same file. Problem 2 will have an alert "This problem uses the same source file as number 1.". Then if I use the buttons to swap #1 with #2, now the new #1 still says "This problem uses the same source file as number 1.". I'm working on that.

[Alex-Jordan]

OK, the language (both user-facing and internal) is now about nest/denest instead of indent/outdent.

Also I moved the alerts for "This problem uses the same source file as number 1." from the template to the javascript, so it can be recalculated as things move. This is probably already an issue with the current mouse-driven rearrangement tool.

[drgrice1]

This is going to take some time to review and test properly. But here are some things I see at this point.

[drgrice1]

Please change this to

Suggested change

	container.querySelectorAll('.psd_list_item').forEach((item) => {
	for (const item of container.querySelectorAll('.psd_list_item')) {

and remove the ); on line 76 below.

Make the same change for the forEach uses on line 89 and 117.

The similar forEach calls in this file that already existed should be updated at some point. The for (const ...) construct is generally clearer for multi-line for loops. The forEach method is largely a remnant of the old days when a var automatically hoisted. With let and const that is no longer an issue. The forEach usage is still nice for named functions such as on line 112 below, and brief one liners such as line 80 below (although that returns the result of num.textContent = '' which is really not correct, and eslint would complain about that).

[drgrice1]

Please change this to

Suggested change

	items.forEach((item, index) => {
	for (const [index, item] of items.entries()) {

and delete the ); on line 103.

I originally included this line in the previous comment, but realized this one might need more explanation because it works differently to iterate over the index and element.

[drgrice1]

Anytime that prototype hacking is being done, generally, you should question the code. There are advanced techniques that require accessing the prototype, but hasOwnProperty usage by itself is not one of them. It is highly questionable if the hasOwnProperty check is needed for a basic object that has no other properties or methods than the ones set locally. There is always the possibility of a conflict with a builtin method or property, and so it is good to check. Although, why would you use Object.prototype.hasOwnProperty.call for a locally defined object that can have no overrides that required the safety of that method. Calling showYet.hasOwnProperty is perfectly safe in this instance.

Although, the real problem is that using a bare object is the wrong thing to do, and the reason for the need of such mysterious incantations as hasOwnProperty at all. showYet should be a Map. A Map was introduced into JavaScript for exactly this purpose and has been around in all browsers for more than 10 years. So instead change line 56 above to const shownYet = new Map();, and change this to

Suggested change

	if (Object.prototype.hasOwnProperty.call(shownYet, sourceFile)) {
	const alert = document.createElement('div');
	alert.className = 'alert alert-danger p-1 mb-2 fw-bold';
	alert.textContent = repeatFileText.replace('[_1]', shownYet[sourceFile]);
	alertContainer.append(alert);
	} else {
	shownYet[sourceFile] = row.querySelector('.pdr_problem_number')?.textContent ?? '';
	}
	if (shownYet.has(sourceFile)) {
	const alert = document.createElement('div');
	alert.className = 'alert alert-danger p-1 mb-2 fw-bold';
	alert.textContent = container.dataset.repeatFileText.replace('[_1]', shownYet.get(sourceFile));
	alertContainer.append(alert);
	} else {
	shownYet.set(sourceFile, row.querySelector('.pdr_problem_number')?.textContent ?? '');
	}