Skip to content

Commit 4ce5cd9

Browse files
dmolsdmols
authored
Merge pull request ucfopen#1063 from mbusch3/quick-bug-fixes
Several small bug fixes...
2 parents 3250f09 + 8f2ac4f commit 4ce5cd9

File tree

11 files changed

+102
-90
lines changed

11 files changed

+102
-90
lines changed

‎assets/js/Components/FixIssuesPage.js‎

Lines changed: 62 additions & 69 deletions
Original file line numberDiff line numberDiff line change
@@ -463,7 +463,10 @@ export default function FixIssuesPage({
463463

464464
setFilteredIssues(tempFilteredContent)
465465
setGroupedList(groupList(tempFilteredContent))
466-
setActiveIssue(holdoverActiveIssue)
466+
if(holdoverActiveIssue) {
467+
setActiveIssue(holdoverActiveIssue)
468+
}
469+
467470
}, [report])
468471

469472

@@ -702,7 +705,7 @@ export default function FixIssuesPage({
702705
return tempDoc.body.innerHTML
703706
}
704707

705-
const handleIssueSave = (issue, markAsReviewed = false) => {
708+
const handleIssueSave = async (issue, markAsReviewed = false) => {
706709

707710
if(!activeContentItem || !activeContentItem?.body || !issue) {
708711
return
@@ -734,83 +737,73 @@ export default function FixIssuesPage({
734737
let fullPageDoc = new DOMParser().parseFromString(fullPageHtml, 'text/html')
735738
let newElement = Html.findElementWithError(fullPageDoc, issue?.newHtml)
736739
let newXpath = Html.findXpathFromElement(newElement)
737-
if(newXpath) {
738-
issue.xpath = newXpath
739-
}
740-
else {
741-
issue.xpath = ""
742-
}
743-
activeContentItem.body = fullPageHtml
744-
740+
issue.xpath = newXpath || ''
741+
745742
// Save the updated issue using the LMS API
746743
let api = new Api(settings)
747744
try {
748-
api.saveIssue(issue, fullPageHtml, markAsReviewed)
749-
.then((responseStr) => {
750-
// Check for HTTP errors before parsing JSON
751-
if (!responseStr.ok) {
752-
processServerError(responseStr)
753-
updateActiveSessionIssue(issue.id, settings.ISSUE_STATE.ERROR)
754-
removeItemFromBeingScanned(issue.contentItemId)
755-
return null
756-
}
757-
return responseStr.json()
758-
})
759-
.then((response) => {
745+
const saveResponse = await api.saveIssue(issue, fullPageHtml, markAsReviewed)
746+
if(!saveResponse.ok){
747+
processServerError(saveResponse)
748+
updateActiveSessionIssue(issue.id, settings.ISSUE_STATE.ERROR)
749+
removeItemFromBeingScanned(issue.contentItemId)
750+
throw Error('Save returned invalid server response.')
751+
}
760752

761-
// If the save falied, show the relevant error message
762-
if (response.data.failed) {
763-
updateActiveSessionIssue(issue.id, settings.ISSUE_STATE.ERROR)
764-
removeItemFromBeingScanned(issue.contentItemId)
765-
response.messages.forEach((msg) => addMessage(msg))
766-
767-
if (Array.isArray(response.data.issues)) {
768-
response.data.issues.forEach((issue) => {
769-
addMessage({
770-
severity: 'error',
771-
message: t(`form.error.${issue.ruleId}`)
772-
})
773-
})
774-
}
753+
const saveResponseJson = await saveResponse.json()
775754

776-
if (Array.isArray(response.data.errors)) {
777-
response.data.errors.forEach((error) => {
778-
addMessage({
779-
severity: 'error',
780-
message: error
781-
})
755+
if(saveResponseJson?.errors && saveResponseJson.errors.length > 0) {
756+
updateActiveSessionIssue(issue.id, settings.ISSUE_STATE.ERROR)
757+
removeItemFromBeingScanned(issue.contentItemId)
758+
saveResponseJson.messages.forEach((msg) => addMessage(msg))
759+
760+
if (Array.isArray(saveResponseJson.errors)) {
761+
saveResponseJson.errors.forEach((error) => {
762+
addMessage({
763+
severity: 'error',
764+
message: t(error)
782765
})
783-
}
766+
})
784767
}
785-
else {
786-
787-
// If the save was successful, show the success message
788-
response.messages.forEach((msg) => addMessage(msg))
789-
790-
if (response.data.issue) {
791-
// Update the report object by rescanning the content
792-
const newIssue = Object.assign({}, issue, response.data.issue)
793-
// const formattedData = formatIssueData(newIssue)
794-
// setActiveIssue(formattedData)
795-
updateActiveSessionIssue(newIssue.id, settings.ISSUE_STATE.SAVED)
796-
797-
api.scanContent(newIssue.contentItemId)
798-
.then((responseStr) => responseStr.json())
799-
.then((res) => {
800-
const tempReport = Object.assign({}, res?.data)
801-
processNewReport(tempReport)
802-
removeItemFromBeingScanned(newIssue.contentItemId)
803-
})
804-
}
805-
else {
806-
// setActiveIssue(formatIssueData(issue))
807-
updateActiveSessionIssue(issue.id, settings.ISSUE_STATE.SAVED)
808-
removeItemFromBeingScanned(issue.contentItemId)
768+
throw Error('Save failed.')
769+
}
770+
771+
// Successful save!
772+
saveResponseJson.messages.forEach((msg) => addMessage(msg))
773+
if(issue.contentItemId === activeContentItem.id) {
774+
activeContentItem.body = fullPageHtml
775+
}
776+
777+
// If there isn't a new issue created, we're done.
778+
if(!saveResponseJson?.data?.issue) {
779+
updateActiveSessionIssue(issue.id, settings.ISSUE_STATE.SAVED)
780+
removeItemFromBeingScanned(issue.contentItemId)
781+
return
782+
}
783+
784+
if(saveResponseJson?.data?.issue) {
785+
// Update the report object by rescanning the content
786+
const newIssue = Object.assign({}, issue, saveResponseJson.data.issue)
787+
updateActiveSessionIssue(newIssue.id, settings.ISSUE_STATE.SAVED)
788+
789+
const scanResponse = await api.scanContent(newIssue.contentItemId)
790+
791+
if(scanResponse.ok) {
792+
const scanResponseJson = await scanResponse.json()
793+
if(scanResponseJson.data) {
794+
const tempReport = Object.assign({}, scanResponseJson.data)
795+
processNewReport(tempReport)
796+
removeItemFromBeingScanned(newIssue.contentItemId)
797+
return
809798
}
810799
}
811-
})
800+
}
801+
802+
updateActiveSessionIssue(issue.id, settings.ISSUE_STATE.SAVED)
803+
removeItemFromBeingScanned(issue.contentItemId)
804+
812805
} catch (error) {
813-
console.error(error)
806+
console.warn(error)
814807
updateActiveSessionIssue(issue.id, settings.ISSUE_STATE.ERROR)
815808
}
816809
}

‎assets/js/Components/Forms/AltText.js‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -54,11 +54,18 @@ export default function AltText ({
5454
let element = Html.toElement(html)
5555

5656
if (isDecorative) {
57-
element = Html.setAttribute(element, "role", "presentation")
57+
58+
// <canvas> elements may have special roles, like "img", and we don't want to overwrite or remove those.
59+
// <input> elements with type="image" also need to keep their role.
60+
if(issue.scanRuleId !== 'canvas_content_described' && issue.scanRuleId !== 'imagebutton_alt_exists') {
61+
element = Html.setAttribute(element, "role", "presentation")
62+
}
5863
element = Html.setAttribute(element, 'alt', '')
5964
element = Html.setAttribute(element, 'title', '')
6065
} else {
61-
element = Html.removeAttribute(element, "role")
66+
if(issue.scanRuleId !== 'canvas_content_described' && issue.scanRuleId !== 'imagebutton_alt_exists') {
67+
element = Html.removeAttribute(element, "role")
68+
}
6269
element = Html.setAttribute(element, "alt", textInputValue)
6370
element = Html.setAttribute(element, "title", textInputValue)
6471
}

‎assets/js/Components/Forms/SensoryMisuseForm.js‎

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@ export default function SensoryMisuseForm({
4848
const includedAttributes = ['alt', 'title', 'aria-label']
4949

5050
const editorRef = useRef(null)
51+
5152
const [sensoryErrors, setSensoryErrors] = useState([])
5253

5354
useEffect(() => {
@@ -107,12 +108,7 @@ export default function SensoryMisuseForm({
107108
}
108109

109110
const traverseNode = (node) => {
110-
111-
// Editor HTML is empty so node ends up empty
112-
if(node === null)
113-
return
114-
115-
if (node.nodeType === Node.TEXT_NODE) {
111+
if (node?.nodeType === Node.TEXT_NODE) {
116112
// Check if the text node contains any sensory words
117113
checkText(node.textContent);
118114
}

‎assets/js/Components/Forms/TableHeadersForm.js‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -74,11 +74,24 @@ export default function TableHeadersForm({
7474
}
7575

7676
const removeHeaders = (table) => {
77+
78+
// The table_structure_misuse rule means that when a table is marked as decorative,
79+
// it should not contain any data-related elements, like <th> or <caption>. All 'summary',
80+
// 'scope', and 'headers' attributes should also be removed.
81+
if(decorationOnly) {
82+
table.removeAttribute('summary')
83+
let caption = table.querySelector('caption')
84+
if(caption) {
85+
caption.remove()
86+
}
87+
}
88+
7789
for (let row of table.rows) {
7890
for (let cell of row.cells) {
91+
cell.removeAttribute('scope')
92+
cell.removeAttribute('headers')
7993
if (cell.tagName === 'TH') {
8094
let newCell = Html.renameElement(cell, 'td')
81-
newCell.removeAttribute('scope')
8295
row.replaceChild(newCell, cell)
8396
}
8497
}

‎assets/js/Components/Widgets/Combobox.css‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,6 @@
2828

2929
.combo-input {
3030
background-color: var(--white);
31-
display: block;
3231
font-size: .85em;
3332
padding: .4em .15em .4em .5em;
3433
border: 1px solid var(--border-color);

‎assets/js/Components/Widgets/FixIssuesContentPreview.js‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -89,12 +89,12 @@ export default function FixIssuesContentPreview({
8989
}
9090

9191
const addPreviewHelperElements = (doc, errorElement) => {
92-
if(!activeIssue || !doc || !errorElement) {
92+
if(!activeIssue || !doc) {
9393
return doc
9494
}
9595

9696
// If the issue edits the alt text, we need to show the auto-updating alt text preview
97-
if (ALT_TEXT_RELATED.includes(formNameFromRule(activeIssue.scanRuleId))) {
97+
if (ALT_TEXT_RELATED.includes(formNameFromRule(activeIssue.scanRuleId)) && errorElement) {
9898
let altText = Html.getAccessibleName(errorElement)
9999

100100
// If there is alt text to show...

‎assets/js/Services/Html.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -481,7 +481,7 @@ export function findElementWithIssue(content, issue) {
481481
else {
482482
let errorHtml = issue?.sourceHtml || undefined
483483
if(issue.status.toString() === '1') {
484-
errorHtml = issue?.newHtml || errorHtml
484+
errorHtml = issue?.newHtml || undefined
485485
}
486486

487487
if(errorHtml === undefined || errorHtml === '') {

‎jest.setup.js‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@ import 'regenerator-runtime/runtime'
22
import '@testing-library/jest-dom'
33

44
import { server } from './assets/js/__tests__/mocks/server.js'
5-
import 'whatwg-fetch';
65

76
// Establish API mocking before all tests.
87
beforeAll(() => server.listen())

‎package.json‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,12 +17,10 @@
1717
"@testing-library/user-event": "^14.6.1",
1818
"copy-webpack-plugin": "13.0.0",
1919
"jest": "^30.0.4",
20-
"jest-environment-jsdom": "^30.0.4",
2120
"msw": "^2.10.4",
2221
"regenerator-runtime": "^0.14.1",
2322
"webpack-cli": "^5.1.4",
24-
"webpack-notifier": "^1.15.0",
25-
"whatwg-fetch": "^3.6.20"
23+
"webpack-notifier": "^1.15.0"
2624
},
2725
"license": "GPL-3.0-only",
2826
"private": true,

‎src/Controller/IssuesController.php‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -61,13 +61,16 @@ public function saveIssue(
6161

6262
$contentUpdated = true;
6363
// Check if new HTML is different from original HTML
64-
if ($newHtml !== '' && ($sourceHtml === $newHtml || $issue->getPreviewHtml() === $newHtml || $issue->getNewHtml() === $newHtml)) {
64+
if ($newHtml !== '' &&
65+
(
66+
$sourceHtml === $newHtml ||
67+
$issue->getPreviewHtml() === $newHtml ||
68+
$issue->getNewHtml() === $newHtml
69+
)
70+
) {
6571
$contentUpdated = false;
6672
}
6773

68-
$output->writeln("Review updated: ".($reviewUpdated ? 'true' : 'false'));
69-
$output->writeln("Content updated: ".($contentUpdated ? 'true' : 'false'));
70-
7174
if(!$reviewUpdated && !$contentUpdated) {
7275
throw new \Exception('form.error.same_html');
7376
}

0 commit comments

Comments
 (0)