Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add feedback in the post processing phase which replaces the webwork2 attempts table. #939

Merged
merged 11 commits into from
Oct 30, 2023

Conversation

drgrice1
Copy link
Member

@drgrice1 drgrice1 commented Oct 7, 2023

A feedback button is placed somewhere in the DOM for each response group. Default locations that should work well in most cases are determined in post processing. However, there is a ww-feedback-container css class that is provided in problem.scss, and a problem author can use it to override the default location by wrapping the responses in a div or span with that class. The classes "align-middle" and "align-bottom" can be added to the container in addition to align the feedback button. By default the button is aligned at the top. See the documentation added in the POD for PG.pl for more details.

The result summary is also generated by PG and is returned in the result summary. The caller can decide to use it or not, and where to place it.

The input background icon was removed, and instead those icons are used for the feedback button icon. Also, an info icon is used for preview, and an exclamation mark in a triangle for partially correct answers.

The correct, incorrect, and (new) partially-correct styles were tweaked to make them a bit darker.

A red dot is added to the top right corner of the feedback button if a message is present.

The ResultsWithError and ResultsWithoutError classes were dispensed with, and instead bootstrap alerts are used with the alert-danger, alert-success, and alert-warning classes for the summary. This is done in the last commit, and we can revert it if we aren't happy with it.

Note that this includes both #897 and #936 which are both used to make this happen.

This is also paired with the webwork2 pull request openwebwork/webwork2#2228.

@drgrice1 drgrice1 force-pushed the attempts-table-replace branch 4 times, most recently from dfbf1b9 to ab6012a Compare October 11, 2023 21:46
Macros or problems can add a hook by calling
`add_content_post_processor` with a subroutine that will be called after
the translator has processed answers.  For all display modes except TeX,
the subroutine will be passed the problem text parsed into a Mojo::DOM
object and a reference to the header text.  For the TeX display mode a
reference to the problem text is passed.  The subroutine can then modify
the problem DOM or text, and header text.  The resulting Mojo::DOM
object will be converted back to a string and the problem text reference
returned by the translator set to point to that string.

Note that the `$PG_PROBLEM_TEXT_ARRAY_REF` has been removed from the
translator code.  It is never used and is redundant with the
`$PG_PROBLEM_TEXT_REF` which is all that is used.  It was becoming
annoying to maintain both in a compatible manner.

The hidden MathQuill inputs for the latex string is added in this post
processing stage (see the ENDDOCUMENT method in PG.pl).
This means that answers are no longer evaluated by the macro.  Instead
the scaffold sections are finalized after the answers are processed
normally by the translator.
This is so that the content post processor methods have access to the
full answer objects.  This gives this approach a whole lot more power
than the (soon to be eliminated) results table has.
@Alex-Jordan
Copy link
Contributor

Following comments I added at openwebwork/webwork2#2228 that belong here...

The padding on the card headers in the popover is reduced some.

Thanks for that. I think it looks better now in ways I can't put into words well. Maybe it's that it's more clear that the headers are headers?

I made clicking on the popover header close it (assuming the popover has a header). I realized that focus and interactive semantics are probably not needed or even desirable in this case. Keyboard users can already open and close the popover, and if the header is focusable, it would probably just be an annoying tab stop.

Yeah I was thinking that for a mouse user, it helps to have a larger area that can be targeted with a click. And the keyboard user doesn't care. I did wonder if automated accessibility checkers will have a problem with this. And it seems that Firefox's accessibility inspector does not like that it's clickable but not focusable. But I would argue as you did, that in this case the letter of the law is not a good thing to follow. And since it's only in the popover, maybe we can count on automated accessiblity inspections to miss that.

The popover should now be in the DOM immediately after the feedback button in most cases (all cases I know of, but if someone places the feedback button in a different place it may not work).

Thanks for working out how to do that. This one could have been a significant accessibility issue. It works as described in the few examples I just looked at.

And thanks for explaining about ESC with the MQ menu. That alone is good for this. (And it makes me realize we need to update the accessibility guide for using WeBWorK.)

I'm using a MacBookPro and quick Googling suggests there is no keyboard way to activate a context menu. Or rather, there is a way to make something like shift + F12 be the same as a right click, but then you still have to get the mouse pointer over to where you would want to activate that menu (focus or cursor position is not enough). Maybe there is a way to pull the mouse pointer to wherever is focus or a text cursor, but I did not explore. Anyway, good to know about the Windows/Linux option.

Copy link
Contributor

@Alex-Jordan Alex-Jordan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested the feature and it is nice and works well. I did not inspect the code itself.

@drgrice1 drgrice1 force-pushed the attempts-table-replace branch 2 times, most recently from 32d7db2 to 9be06a6 Compare October 25, 2023 10:18
@drgrice1
Copy link
Member Author

I think this is a good start at least. Clearly there is more to be done.

The tab order thing with the MathQuill toolbar is something that has been bugging me even without this. I know some have said that they could care less about the toolbar, but in my experience (and with the students I deal with) the toolbar is essential. The average student in the courses I teach does not have a clue what to type to achieve the things that the toolbar provides. It is unfortunate that there is not a more uniform way to open the context menu in browsers to disable/enable it.

Yeah, the Firefox accessibility checker does not like that the popover header is clickable but not focusable. I noticed that the WAVE Evaluation Tool in Chrome does not complain about it though.

@drgrice1 drgrice1 force-pushed the attempts-table-replace branch from 9be06a6 to 577a79c Compare October 28, 2023 03:17
… attempts table.

A feedback button is placed somewhere in the DOM for each response
group.  Default locations that should work well in most cases are
determined in post processing.  However, there is a
ww-feedback-container css class that is provided in problem.scss, and a
problem author can use it to override the default location by wrapping
the responses in a div or span with that class.

The classes "align-middle" and "align-bottom" can be added to the
container in addition to align the feedback button.  By default the
button is aligned at the top.

The result summary is also generated by PG and is returned in the result
summary. The caller can decide to use it or not, and where to place it.
…button icon.

An info icon is used for preview, and an exclamation mark in a triangle
for partially correct answers.
…em a bit darker.

Also, make them only apply when the element is not focused so that focus
styling is not overridden..

Opacity is simply set to 1.

The inset box shadow also has an additial 1 pixel blur radius.
Dispense with the ResultsWithErrors, ResultsWithoutErrors, and
ResultsAlert classes, and use bootstrap alerts instead.
Reduce the padding on the card headers in the results popover.

Make the popover container the parent element of the feedback button.
In most cases this will result in the popover being in the DOM
immediately after the feedback button, and thus also immediately after
the button in the tab order.

Make clicking on the popover header close it.  Note that this does not
have interactive semantics other than a pointer cursor when the mouse is
over it, and is not focusable.  That is probably okay since this would
be an annoying tab stop for keyboard users, and keyboard users can
already open and close the popover with the button.
@drgrice1 drgrice1 force-pushed the attempts-table-replace branch from 577a79c to 8b76324 Compare October 29, 2023 02:51
@pstaabp pstaabp merged commit 6775fff into openwebwork:develop Oct 30, 2023
2 checks passed
@drgrice1 drgrice1 deleted the attempts-table-replace branch October 30, 2023 21:08
@Alex-Jordan
Copy link
Contributor

The little badge that indicates there is feedback is always a red circle, even if the answer is correct or partially correct. Do we want that? For a partially correct answer, it seems fine that way. For a correct answer that has a feedback message, it seems like maybe we would not want that to be red. That is, if red indicates incorrect. In this place, maybe red indicates an alert that there is something to look at...

Screenshot 2023-11-30 at 10 17 36 PM

@somiaj
Copy link
Contributor

somiaj commented Dec 1, 2023

The messages have that yellow background, maybe make the dot match that in all cases?

@drgrice1
Copy link
Member Author

drgrice1 commented Dec 1, 2023

The idea here was that "red" indicates an alert that there is something (a message) to look at, and was what was suggested when we discussed this in our meetings. It does seem a bit jarring and perhaps confusing to have the red dot on the green correct button though. It would be easy to change this to always be yellow, and that would probably be more appropriate. It would also not be hard to make the color be dependent on the status of the answer, but I think that always using yellow is more appropriate for this.

drgrice1 added a commit to drgrice1/pg that referenced this pull request Dec 1, 2023
drgrice1 added a commit to drgrice1/pg that referenced this pull request Dec 1, 2023
@Alex-Jordan
Copy link
Contributor

I was looking at this tonight and I wonder if it would be better to put any feedback message immediately under the "Correct/Incorrect" heading, like this:

Screenshot 2023-12-02 at 12 04 21 AM

Currently the feedback message comes at the bottom, under a heading "Message". I'm guessing this is mainly because the message came at the far right of the results table. And then once it is down there, you need a "Message" heading to make it clear this is not part of the "Correct Answer" or whatever was above. As a student user, I think I would want to read the feedback message right away before the other information that might be here. And I think this would let us drop the "Message" header in a natural way.

Does anyone feel that the message belongs at the bottom?

@drgrice1
Copy link
Member Author

drgrice1 commented Dec 2, 2023

Yeah, I just order the feedback in the same order that it was in the old attempts table. It could be ordered differently. I think the reordering you propose sounds good.

@dlglin
Copy link
Member

dlglin commented Dec 4, 2023

I like this better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants