From 240eaed52e8b740dc2f96c8563902c0c862f9f3f Mon Sep 17 00:00:00 2001 From: Maximilian Haye Date: Tue, 19 Sep 2023 18:00:23 +0200 Subject: [PATCH] feat: replace HTML validation attrs with soft validation We mustn't prevent form submission, but we still want to give some feedback to the user. Closes #57 --- amd/build/view_question.min.js | 3 + amd/build/view_question.min.js.map | 1 + amd/src/view_question.js | 146 ++++++++++++++++++ classes/question_metadata.php | 12 +- classes/question_ui_renderer.php | 55 +++++++ question.php | 12 +- renderer.php | 13 +- tests/question_ui_renderer_test.php | 70 +++++++-- tests/question_uis/ids_and_names.xhtml | 27 ++++ .../{inputs.xhtml => metadata.xhtml} | 4 +- tests/question_uis/parameters.xhtml | 17 -- tests/question_uis/validations.xhtml | 11 ++ 12 files changed, 335 insertions(+), 36 deletions(-) create mode 100644 amd/build/view_question.min.js create mode 100644 amd/build/view_question.min.js.map create mode 100644 amd/src/view_question.js create mode 100644 tests/question_uis/ids_and_names.xhtml rename tests/question_uis/{inputs.xhtml => metadata.xhtml} (79%) create mode 100644 tests/question_uis/validations.xhtml diff --git a/amd/build/view_question.min.js b/amd/build/view_question.min.js new file mode 100644 index 00000000..326cad5c --- /dev/null +++ b/amd/build/view_question.min.js @@ -0,0 +1,3 @@ +define("qtype_questionpy/view_question",["exports","jquery","theme_boost/bootstrap/popover"],(function(_exports,_jquery,_popover){var obj;function getLabelFor(input){const id=input.id;if(""!==id){const label=document.querySelector("label[for='".concat(id,"']"));if(label)return label}const label=input.closest("label");return label||null}async function checkConstraints(element){try{"qpy_required"in element.dataset&&element.setAttribute("required","required");for(const attr of["pattern","minlength","maxlength","min","max"])"qpy_".concat(attr)in element.dataset&&element.setAttribute(attr,element.dataset["qpy_".concat(attr)]);element.checkValidity()?function(element){element.classList.remove("is-invalid"),element.removeAttribute("aria-invalid"),(0,_jquery.default)([element,getLabelFor(element)]).popover("dispose")}(element):function(element,message){let ariaInvalid=!(arguments.length>2&&void 0!==arguments[2])||arguments[2];element.classList.add("is-invalid"),ariaInvalid?element.setAttribute("aria-invalid","true"):element.removeAttribute("aria-invalid");let popoverTarget=element;if("checkbox"===element.type||"radio"===element.type){const label=getLabelFor(element);popoverTarget=label?label.contains(element)?label:[element,label]:element}(0,_jquery.default)(popoverTarget).popover({toggle:"popover",trigger:"hover",content:message})}(element,element.validationMessage,!element.validity.valueMissing)}finally{for(const attr of["required","pattern","minlength","maxlength","min","max"])element.removeAttribute(attr)}}Object.defineProperty(_exports,"__esModule",{value:!0}),_exports.init=async function(){for(const element of document.querySelectorAll("\n [data-qpy_required], [data-qpy_pattern], \n [data-qpy_minlength], [data-qpy_maxlength], \n [data-qpy_min], [data-qpy_max]\n "))await checkConstraints(element),element.addEventListener("change",(event=>checkConstraints(event.target)))},_jquery=(obj=_jquery)&&obj.__esModule?obj:{default:obj}})); + +//# sourceMappingURL=view_question.min.js.map \ No newline at end of file diff --git a/amd/build/view_question.min.js.map b/amd/build/view_question.min.js.map new file mode 100644 index 00000000..9b21e081 --- /dev/null +++ b/amd/build/view_question.min.js.map @@ -0,0 +1 @@ +{"version":3,"file":"view_question.min.js","sources":["../src/view_question.js"],"sourcesContent":["/*\n * This file is part of the QuestionPy Moodle plugin - https://questionpy.org\n *\n * Moodle is free software: you can redistribute it and/or modify\n * it under the terms of the GNU General Public License as published by\n * the Free Software Foundation, either version 3 of the License, or\n * (at your option) any later version.\n *\n * Moodle is distributed in the hope that it will be useful,\n * but WITHOUT ANY WARRANTY; without even the implied warranty of\n * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the\n * GNU General Public License for more details.\n *\n * You should have received a copy of the GNU General Public License\n * along with Moodle. If not, see .\n */\n\nimport $ from \"jquery\";\nimport \"theme_boost/bootstrap/popover\";\n\n/**\n * If the given input(-like) element is labelled, returns the label element. Returns null otherwise.\n *\n * @param {HTMLElement} input\n * @return {HTMLLabelElement | null}\n * @see {@link https://html.spec.whatwg.org/multipage/forms.html#the-label-element}\n */\nfunction getLabelFor(input) {\n // A label can reference its labeled control in its for attribute.\n const id = input.id;\n if (id !== \"\") {\n const label = document.querySelector(`label[for='${id}']`);\n if (label) {\n return label;\n }\n }\n\n // Or the labeled control can be a descendant of the label.\n const label = input.closest(\"label\");\n if (label) {\n return label;\n }\n\n return null;\n}\n\n/**\n * Marks the given input element as invalid.\n *\n * @param {HTMLElement} element\n * @param {string} message validation message to show\n * @param {boolean} ariaInvalid\n */\nfunction markInvalid(element, message, ariaInvalid = true) {\n element.classList.add(\"is-invalid\");\n if (ariaInvalid) {\n element.setAttribute(\"aria-invalid\", \"true\");\n } else {\n element.removeAttribute(\"aria-invalid\");\n }\n\n let popoverTarget = element;\n if (element.type === \"checkbox\" || element.type === \"radio\") {\n // Checkboxes and radios make for a very small hit area for the popover, so we attach the popover to the label.\n const label = getLabelFor(element);\n if (!label) {\n // No label -> Add the popover just to the checkbox.\n popoverTarget = element;\n } else if (label.contains(element)) {\n // Label contains checkbox -> Add the popover just to the label.\n popoverTarget = label;\n } else {\n // Separate label and checkbox -> Add the popover to both.\n popoverTarget = [element, label];\n }\n }\n\n $(popoverTarget).popover({\n toggle: \"popover\",\n trigger: \"hover\",\n content: message\n });\n}\n\n/**\n * Undoes what {@link markInvalid} did.\n *\n * @param {HTMLInputElement} element\n */\nfunction unmarkInvalid(element) {\n element.classList.remove(\"is-invalid\");\n element.removeAttribute(\"aria-invalid\");\n\n $([element, getLabelFor(element)]).popover(\"dispose\");\n}\n\n/**\n * Softly (i.e. without preventing form submission) validates constraints on the given element.\n *\n * @param {HTMLInputElement} element\n */\nasync function checkConstraints(element) {\n /* Our goal here is to show helpful localised validation messages without actually preventing form submission.\n One way to achieve this would be to add the attribute \"novalidate\" to the form element, but that might interfere\n with other questions (since they share the same form).\n We also don't want to reimplement the validation logic already implemented by browsers.\n Instead, the standard validation attributes are added, their validity checked, the message used to create a\n popover, and the attributes removed. */\n try {\n if (\"qpy_required\" in element.dataset) {\n element.setAttribute(\"required\", \"required\");\n }\n for (const attr of [\"pattern\", \"minlength\", \"maxlength\", \"min\", \"max\"]) {\n if (`qpy_${attr}` in element.dataset) {\n element.setAttribute(attr, element.dataset[`qpy_${attr}`]);\n }\n }\n\n const isValid = element.checkValidity();\n if (isValid) {\n unmarkInvalid(element);\n } else {\n // Aria-invalid shouldn't be set for missing inputs until the user has tried to submit them.\n // https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-invalid\n markInvalid(element, element.validationMessage, !element.validity.valueMissing);\n }\n } finally {\n for (const attr of [\"required\", \"pattern\", \"minlength\", \"maxlength\", \"min\", \"max\"]) {\n element.removeAttribute(attr);\n }\n }\n}\n\n/**\n * Adds change event handlers for soft validation.\n */\nexport async function init() {\n for (const element of document.querySelectorAll(`\n [data-qpy_required], [data-qpy_pattern], \n [data-qpy_minlength], [data-qpy_maxlength], \n [data-qpy_min], [data-qpy_max]\n `)) {\n await checkConstraints(element);\n element.addEventListener(\"change\", event => checkConstraints(event.target));\n }\n}\n"],"names":["getLabelFor","input","id","label","document","querySelector","closest","checkConstraints","element","dataset","setAttribute","attr","checkValidity","classList","remove","removeAttribute","popover","unmarkInvalid","message","ariaInvalid","add","popoverTarget","type","contains","toggle","trigger","content","markInvalid","validationMessage","validity","valueMissing","querySelectorAll","addEventListener","event","target"],"mappings":"mJA2BSA,YAAYC,aAEXC,GAAKD,MAAMC,MACN,KAAPA,GAAW,OACLC,MAAQC,SAASC,mCAA4BH,aAC/CC,aACOA,YAKTA,MAAQF,MAAMK,QAAQ,gBACxBH,OAIG,oBA0DII,iBAAiBC,aAQpB,iBAAkBA,QAAQC,SAC1BD,QAAQE,aAAa,WAAY,gBAEhC,MAAMC,OAAQ,CAAC,UAAW,YAAa,YAAa,MAAO,OACxD,cAAOA,QAAUH,QAAQC,SACzBD,QAAQE,aAAaC,KAAMH,QAAQC,sBAAeE,QAI1CH,QAAQI,yBA7BTJ,SACnBA,QAAQK,UAAUC,OAAO,cACzBN,QAAQO,gBAAgB,oCAEtB,CAACP,QAASR,YAAYQ,WAAWQ,QAAQ,WA2BnCC,CAAcT,kBAnELA,QAASU,aAASC,uEACnCX,QAAQK,UAAUO,IAAI,cAClBD,YACAX,QAAQE,aAAa,eAAgB,QAErCF,QAAQO,gBAAgB,oBAGxBM,cAAgBb,WACC,aAAjBA,QAAQc,MAAwC,UAAjBd,QAAQc,KAAkB,OAEnDnB,MAAQH,YAAYQ,SAMtBa,cALClB,MAGMA,MAAMoB,SAASf,SAENL,MAGA,CAACK,QAASL,OANVK,4BAUtBa,eAAeL,QAAQ,CACrBQ,OAAQ,UACRC,QAAS,QACTC,QAASR,UA4CLS,CAAYnB,QAASA,QAAQoB,mBAAoBpB,QAAQqB,SAASC,0BAGjE,MAAMnB,OAAQ,CAAC,WAAY,UAAW,YAAa,YAAa,MAAO,OACxEH,QAAQO,gBAAgBJ,kGAS3B,MAAMH,WAAWJ,SAAS2B,kLAKrBxB,iBAAiBC,SACvBA,QAAQwB,iBAAiB,UAAUC,OAAS1B,iBAAiB0B,MAAMC"} \ No newline at end of file diff --git a/amd/src/view_question.js b/amd/src/view_question.js new file mode 100644 index 00000000..11a99717 --- /dev/null +++ b/amd/src/view_question.js @@ -0,0 +1,146 @@ +/* + * This file is part of the QuestionPy Moodle plugin - https://questionpy.org + * + * Moodle is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * Moodle is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with Moodle. If not, see . + */ + +import $ from "jquery"; +import "theme_boost/bootstrap/popover"; + +/** + * If the given input(-like) element is labelled, returns the label element. Returns null otherwise. + * + * @param {HTMLElement} input + * @return {HTMLLabelElement | null} + * @see {@link https://html.spec.whatwg.org/multipage/forms.html#the-label-element} + */ +function getLabelFor(input) { + // A label can reference its labeled control in its for attribute. + const id = input.id; + if (id !== "") { + const label = document.querySelector(`label[for='${id}']`); + if (label) { + return label; + } + } + + // Or the labeled control can be a descendant of the label. + const label = input.closest("label"); + if (label) { + return label; + } + + return null; +} + +/** + * Marks the given input element as invalid. + * + * @param {HTMLElement} element + * @param {string} message validation message to show + * @param {boolean} ariaInvalid + */ +function markInvalid(element, message, ariaInvalid = true) { + element.classList.add("is-invalid"); + if (ariaInvalid) { + element.setAttribute("aria-invalid", "true"); + } else { + element.removeAttribute("aria-invalid"); + } + + let popoverTarget = element; + if (element.type === "checkbox" || element.type === "radio") { + // Checkboxes and radios make for a very small hit area for the popover, so we attach the popover to the label. + const label = getLabelFor(element); + if (!label) { + // No label -> Add the popover just to the checkbox. + popoverTarget = element; + } else if (label.contains(element)) { + // Label contains checkbox -> Add the popover just to the label. + popoverTarget = label; + } else { + // Separate label and checkbox -> Add the popover to both. + popoverTarget = [element, label]; + } + } + + $(popoverTarget).popover({ + toggle: "popover", + trigger: "hover", + content: message + }); +} + +/** + * Undoes what {@link markInvalid} did. + * + * @param {HTMLInputElement} element + */ +function unmarkInvalid(element) { + element.classList.remove("is-invalid"); + element.removeAttribute("aria-invalid"); + + $([element, getLabelFor(element)]).popover("dispose"); +} + +/** + * Softly (i.e. without preventing form submission) validates constraints on the given element. + * + * @param {HTMLInputElement} element + */ +async function checkConstraints(element) { + /* Our goal here is to show helpful localised validation messages without actually preventing form submission. + One way to achieve this would be to add the attribute "novalidate" to the form element, but that might interfere + with other questions (since they share the same form). + We also don't want to reimplement the validation logic already implemented by browsers. + Instead, the standard validation attributes are added, their validity checked, the message used to create a + popover, and the attributes removed. */ + try { + if ("qpy_required" in element.dataset) { + element.setAttribute("required", "required"); + } + for (const attr of ["pattern", "minlength", "maxlength", "min", "max"]) { + if (`qpy_${attr}` in element.dataset) { + element.setAttribute(attr, element.dataset[`qpy_${attr}`]); + } + } + + const isValid = element.checkValidity(); + if (isValid) { + unmarkInvalid(element); + } else { + // Aria-invalid shouldn't be set for missing inputs until the user has tried to submit them. + // https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-invalid + markInvalid(element, element.validationMessage, !element.validity.valueMissing); + } + } finally { + for (const attr of ["required", "pattern", "minlength", "maxlength", "min", "max"]) { + element.removeAttribute(attr); + } + } +} + +/** + * Adds change event handlers for soft validation. + */ +export async function init() { + for (const element of document.querySelectorAll(` + [data-qpy_required], [data-qpy_pattern], + [data-qpy_minlength], [data-qpy_maxlength], + [data-qpy_min], [data-qpy_max] + `)) { + await checkConstraints(element); + element.addEventListener("change", event => checkConstraints(event.target)); + } +} diff --git a/classes/question_metadata.php b/classes/question_metadata.php index 0737b26c..57e26ca6 100644 --- a/classes/question_metadata.php +++ b/classes/question_metadata.php @@ -38,15 +38,25 @@ class question_metadata { */ public array $expecteddata = []; + /** + * @var string[] an array of required field names + * @see \question_manually_gradable::is_complete_response() + * @see \question_manually_gradable::is_gradable_response() + */ + public array $requiredfields = []; + /** * Initializes a new instance. * * @param array|null $correctresponse if known, an array of `name => correct_value` entries for the expected * response fields * @param array $expecteddata an array of `name => PARAM_X` entries for the expected response fields + * @param string[] $requiredfields an array of required field names */ - public function __construct(?array $correctresponse = null, array $expecteddata = []) { + public function __construct(?array $correctresponse = null, array $expecteddata = [], + array $requiredfields = []) { $this->correctresponse = $correctresponse; $this->expecteddata = $expecteddata; + $this->requiredfields = $requiredfields; } } diff --git a/classes/question_ui_renderer.php b/classes/question_ui_renderer.php index bb0afa4b..157be78c 100644 --- a/classes/question_ui_renderer.php +++ b/classes/question_ui_renderer.php @@ -182,6 +182,10 @@ public function get_metadata(): question_metadata { $name = $element->getAttribute("name"); if ($name) { $this->metadata->expecteddata[$name] = PARAM_RAW; + + if ($element->hasAttribute("required")) { + $this->metadata->requiredfields[] = $name; + } } } } @@ -215,6 +219,7 @@ private function render_part(DOMNode $part, question_attempt $qa, ?question_disp try { $this->hide_unwanted_feedback($xpath, $options); $this->set_input_values_and_readonly($xpath, $qa, $options); + $this->soften_validation($xpath); $this->shuffle_contents($xpath); $this->add_styles($xpath); $this->mangle_ids_and_names($xpath, $qa); @@ -451,6 +456,56 @@ private function resolve_placeholders(DOMXPath $xpath): void { } } + /** + * Replaces the HTML attributes `pattern`, `required`, `minlength`, `maxlength` so that submission is not prevented. + * + * The standard attributes are replaced with `data-qpy_X`, which are then evaluated in JS. + * Ideally we'd also want to handle min and max here, but their evaluation in JS would be quite complicated. + * + * @param DOMXPath $xpath + * @return void + */ + private function soften_validation(DOMXPath $xpath): void { + /** @var DOMElement $element */ + foreach ($xpath->query("//xhtml:input[@pattern]") as $element) { + $pattern = $element->getAttribute("pattern"); + $element->removeAttribute("pattern"); + $element->setAttribute("data-qpy_pattern", $pattern); + } + + foreach ($xpath->query("(//xhtml:input | //xhtml:select | //xhtml:textarea)[@required]") as $element) { + $element->removeAttribute("required"); + $element->setAttribute("data-qpy_required", "data-qpy_required"); + $element->setAttribute("aria-required", "true"); + } + + foreach ($xpath->query("(//xhtml:input | //xhtml:textarea)[@minlength]") as $element) { + $minlength = $element->getAttribute("minlength"); + $element->removeAttribute("minlength"); + $element->setAttribute("data-qpy_minlength", $minlength); + } + + foreach ($xpath->query("(//xhtml:input | //xhtml:textarea)[@maxlength]") as $element) { + $maxlength = $element->getAttribute("maxlength"); + $element->removeAttribute("maxlength"); + $element->setAttribute("data-qpy_maxlength", $maxlength); + } + + foreach ($xpath->query("//xhtml:input[@min]") as $element) { + $min = $element->getAttribute("min"); + $element->removeAttribute("min"); + $element->setAttribute("data-qpy_min", $min); + $element->setAttribute("aria-valuemin", $min); + } + + foreach ($xpath->query("//xhtml:input[@max]") as $element) { + $max = $element->getAttribute("max"); + $element->removeAttribute("max"); + $element->setAttribute("data-qpy_max", $max); + $element->setAttribute("aria-valuemax", $max); + } + } + /** * Adds CSS classes to various elements to style them similarly to Moodle's own question types. * diff --git a/question.php b/question.php index e3b4609b..49ff985c 100644 --- a/question.php +++ b/question.php @@ -83,12 +83,13 @@ public function __construct(string $packagehash, string $questionstate) { public function start_attempt(question_attempt_step $step, $variant): void { $attempt = $this->api->start_attempt($this->packagehash, $this->questionstate, $variant); + $step->set_qt_var(self::QT_VAR_ATTEMPT_STATE, $attempt->attemptstate); + // We generate a fixed seed to be used during every render of the attempt, to keep shuffles deterministic. $mtseed = mt_rand(); $step->set_qt_var(self::QT_VAR_MT_SEED, $mtseed); $this->ui = new question_ui_renderer($attempt->ui->content, $attempt->ui->parameters, $mtseed); - $step->set_qt_var(self::QT_VAR_ATTEMPT_STATE, $attempt->attemptstate); } /** @@ -154,7 +155,12 @@ public function get_correct_response(): ?array { * {@see question_attempt_step::get_qt_data()}. * @return bool whether this response is a complete answer to this question. */ - public function is_complete_response(array $response) { + public function is_complete_response(array $response): bool { + foreach ($this->ui->get_metadata()->requiredfields as $requiredfield) { + if (!isset($response[$requiredfield]) || $response[$requiredfield] === "") { + return false; + } + } return true; } @@ -165,7 +171,7 @@ public function is_complete_response(array $response) { * * @param array $prevresponse the responses previously recorded for this question, * as returned by {@see question_attempt_step::get_qt_data()} - * @param array $newresponse the new responses, in the same format. + * @param array $newresponse the new responses, in the same format. * @return bool whether the two sets of responses are the same - that is * whether the new set of responses can safely be discarded. */ diff --git a/renderer.php b/renderer.php index ca5aa416..a5a275ee 100644 --- a/renderer.php +++ b/renderer.php @@ -30,13 +30,24 @@ */ class qtype_questionpy_renderer extends qtype_renderer { + /** + * Return any HTML that needs to be included in the page's when this + * question is used. + * @param question_attempt $qa the question attempt that will be displayed on the page. + * @return string HTML fragment. + */ + public function head_code(question_attempt $qa) { + $this->page->requires->js_call_amd("qtype_questionpy/view_question", "init"); + return parent::head_code($qa); + } + /** * Generate the display of the formulation part of the question. This is the * area that contains the quetsion text, and the controls for students to * input their answers. Some question types also embed bits of feedback, for * example ticks and crosses, in this area. * - * @param question_attempt $qa the question attempt to display. + * @param question_attempt $qa the question attempt to display. * @param question_display_options $options controls what should and should not be displayed. * @return string HTML fragment. * @throws coding_exception diff --git a/tests/question_ui_renderer_test.php b/tests/question_ui_renderer_test.php index ccdcda18..1741c964 100644 --- a/tests/question_ui_renderer_test.php +++ b/tests/question_ui_renderer_test.php @@ -36,7 +36,7 @@ class question_ui_renderer_test extends \advanced_testcase { * @covers \qtype_questionpy\question_metadata */ public function test_should_extract_correct_metadata() { - $input = file_get_contents(__DIR__ . "/question_uis/inputs.xhtml"); + $input = file_get_contents(__DIR__ . "/question_uis/metadata.xhtml"); $ui = new question_ui_renderer($input, [], mt_rand()); $metadata = $ui->get_metadata(); @@ -51,8 +51,10 @@ public function test_should_extract_correct_metadata() { "my_select" => PARAM_RAW, "my_radio" => PARAM_RAW, "my_text" => PARAM_RAW, - "my_button" => PARAM_RAW - ]), $metadata); + "my_button" => PARAM_RAW, + "only_lowercase_letters" => PARAM_RAW, + "between_5_and_10_chars" => PARAM_RAW + ], ["my_number"]), $metadata); } /** @@ -211,7 +213,7 @@ public function test_should_throw_when_formulation_is_missing() { * @covers \qtype_questionpy\question_ui_renderer */ public function test_should_mangle_names() { - $input = file_get_contents(__DIR__ . "/question_uis/inputs.xhtml"); + $input = file_get_contents(__DIR__ . "/question_uis/ids_and_names.xhtml"); $ui = new question_ui_renderer($input, [], mt_rand()); $qa = $this->createStub(\question_attempt::class); @@ -224,15 +226,26 @@ public function test_should_mangle_names() { $this->assertXmlStringEqualsXmlString(<< - - - One - Two -