Skip to content

Commit

Permalink
This is another attempt at fixing student_ans HTML escaping issues.
Browse files Browse the repository at this point in the history
This backs up one step from what was done in openwebwork#1004.  Instead of calling
`protectHTML` on `$ans->{student_ans}` in the `cmp_error` method, that
is instead called in the `cmp_parse` method in the case that the
`$ans->{student_value}` is undefined.

With openwebwork#1004, if `student_value` was defined, and then a problem author
calls `Value::Error` in a custom answer checker, it results in the
`student_ans` being escaped twice because it is escaped in `cmp_parse`
and then again after the custom answer checker is called.  With this
approach the `student_ans` is escaped when `cmp_parse` is called in
either case regardless if if `student_value` is definece, and then not
again later (if `student_value` is defined and thus the checker is
called)

This is also before the `format_matrix` method is called for array
answers.  This fixes an issue also caused by escaping those already HTML
formatted answers. In that case the `student_ans` is completely
redefined after this from the `student_formula`.  With openwebwork#1004, the
`student_ans` is first HTML formatted in this case, and then later
escaped which results in an incorrect display.

This of course still fixes the issue that openwebwork#1004 attempted to fix,
because the `student_value` is not defined, and so the `protectHTML` now
called in `cmp_parse` covers this case.

I am sure that this is not the end of this issue, but is better than
both before openwebwork#1004 and after.  I think that something along the lines of
@dpvc's suggestion in
openwebwork#990 (comment) will
be needed to fully resolve this issue.  I suggest that rather than
having `student_ans_text`, `student_ans_html`, `student_ans_latex`, and
`preview_ans_text` as suggested there, we use `student_ans`,
`preview_latex_string`, and `preview_text_string`.  `student_ans` would
always remain a text answer, and never be HTML formatted,
`preview_ans_latex` would stay the same and be LaTeX, and
`preview_text_string` would always be HTML.  The naming is not the best
since `preview_text_string` is actually HTML, but it is the closest to
the current usage, and so would take the least work.  Note that
currently `preview_text_string` is not actually used for anything.  Even
though it is documented in `lib/AnswerHash.pm` as being what is supposed
to be displayed for the preview.  So this proposal just aligns that
answer hash key with its intended use.
  • Loading branch information
drgrice1 committed Feb 29, 2024
1 parent cb1550b commit d832953
Showing 1 changed file with 1 addition and 2 deletions.
3 changes: 1 addition & 2 deletions lib/Value/AnswerChecker.pm
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,7 @@ sub cmp_parse {
$self->cmp_diagnostics($ans);
}
} else {
$ans->{student_ans} = protectHTML($ans->{student_ans});
$self->cmp_collect($ans);
$self->cmp_error($ans);
}
Expand Down Expand Up @@ -337,8 +338,6 @@ sub cmp_error {
. protectHTML(substr($string, $s, $e - $s))
. '</SPAN>'
. protectHTML(substr($string, $e));
} else {
$ans->{student_ans} = protectHTML($ans->{student_ans});
}
$self->cmp_Error($ans, $message);
}
Expand Down

0 comments on commit d832953

Please sign in to comment.