Conversation
…ut groups, and custom field icons - Reorganize form into logical sections: Name & Identity, Birth & Family, Contact Information, Social Media, Church Membership - Add Bootstrap 4 input groups with appropriate icons for all form fields - Update formCustomField() function with Bootstrap 4 patterns and icons for all 12 field types - Add FAB buttons on PersonEditor (Save/Cancel) and PersonView (Edit/Note) - Improve form layout for better data entry workflow
There was a problem hiding this comment.
Pull request overview
This PR modernizes the PersonEditor and PersonView pages with a comprehensive UX overhaul focused on improved form organization, visual consistency, and user workflow. The changes introduce a Floating Action Button (FAB) pattern for primary actions and reorganize the PersonEditor form into five logical sections (Name & Identity, Birth & Family, Contact Information, Social Media, Church Membership) with Bootstrap 4 input groups and icons throughout.
Key changes:
- Implemented FAB buttons for quick access to primary actions (Edit/Note on PersonView, Save/Cancel on PersonEditor)
- Updated all 12 custom field types in
formCustomField()to use Bootstrap 4input-group-prependpattern with appropriate FontAwesome icons - Enhanced "Save and Add" workflow to preserve family context when adding multiple people to the same family
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| src/PersonView.php | Removed duplicate Edit/Note buttons from sidebar and button area; added FAB container with Edit Person and Add Note buttons |
| src/PersonEditor.php | Reorganized form into 5 logical card sections; migrated all input fields to Bootstrap 4 input groups; added FAB container with Save/Cancel/Save and Add buttons; improved redirect logic for "Save and Add" workflow |
| src/Include/Functions.php | Updated formCustomField() function to use Bootstrap 4 input-group-prepend pattern with icons for all 12 field types (boolean, date, text, textarea, year, season, number, person selector, money, phone, custom list) |
Comments suppressed due to low confidence (1)
src/Include/Functions.php:667
- Security: Use
ENT_QUOTESinstead ofENT_NOQUOTESwhen usinghtmlentities(). TheENT_NOQUOTESflag does not escape single or double quotes, which could lead to XSS vulnerabilities when values are used in HTML attributes.
'<textarea class="form-control" id="' . $fieldname . '" name="' . $fieldname . '" rows="4" maxlength="65535">' . htmlentities(stripslashes($data), ENT_NOQUOTES, 'UTF-8') . '</textarea>' .
src/Include/Functions.php
Outdated
| echo '</div>'; | ||
| echo '<input class="form-control" type="text" Name="' . $fieldname . '" maxlength="30" size="30" value="' . htmlentities(stripslashes($data), ENT_NOQUOTES, 'UTF-8') . '" data-inputmask=\'"mask": "' . SystemConfig::getValue('sPhoneFormat') . '"\' data-mask>'; | ||
| echo '<br><input type="checkbox" name="' . $fieldname . 'noformat" value="1"'; | ||
| echo '<input class="form-control" type="text" id="' . $fieldname . '" name="' . $fieldname . '" maxlength="30" value="' . htmlentities(stripslashes($data), ENT_NOQUOTES, 'UTF-8') . '" data-inputmask=\'"mask": "' . SystemConfig::getValue('sPhoneFormat') . '"\' data-mask>'; |
There was a problem hiding this comment.
Security: Use ENT_QUOTES instead of ENT_NOQUOTES when using htmlentities(). The ENT_NOQUOTES flag does not escape single or double quotes, which could lead to XSS vulnerabilities when values are used in HTML attributes.
| echo '<input class="form-control" type="text" id="' . $fieldname . '" name="' . $fieldname . '" maxlength="30" value="' . htmlentities(stripslashes($data), ENT_NOQUOTES, 'UTF-8') . '" data-inputmask=\'"mask": "' . SystemConfig::getValue('sPhoneFormat') . '"\' data-mask>'; | |
| echo '<input class="form-control" type="text" id="' . $fieldname . '" name="' . $fieldname . '" maxlength="30" value="' . htmlentities(stripslashes($data), ENT_QUOTES, 'UTF-8') . '" data-inputmask=\'"mask": "' . SystemConfig::getValue('sPhoneFormat') . '"\' data-mask>'; |
src/PersonEditor.php
Outdated
| <a href="#" class="fab-button fab-save-add" onclick="document.getElementById('PersonSaveAndAddButton').click(); return false;"> | ||
| <span class="fab-label"><?= gettext('Save and Add') ?></span> | ||
| <div class="fab-icon"> | ||
| <i class="fa-solid fa-user-plus"></i> | ||
| </div> | ||
| </a> | ||
| <?php } ?> | ||
| <a href="#" class="fab-button fab-save" onclick="document.getElementById('PersonSaveButton').click(); return false;"> | ||
| <span class="fab-label"><?= gettext('Save') ?></span> | ||
| <div class="fab-icon"> | ||
| <i class="fa-solid fa-check"></i> | ||
| </div> | ||
| </a> |
There was a problem hiding this comment.
Accessibility: The FAB buttons with href="#" should include role="button" and proper ARIA labels. Additionally, they lack title attributes that the PersonView FAB buttons have. Add title attributes for consistency and better accessibility.
src/PersonEditor.php
Outdated
| <input type="hidden" name="Address2" value="<?= InputUtils::escapeAttribute(stripslashes($sAddress2)) ?>"> | ||
| <input type="hidden" name="City" value="<?= InputUtils::escapeAttribute(stripslashes($sCity)) ?>"> | ||
| <input type="hidden" name="State" value="<?= htmlentities(stripslashes($sState), ENT_NOQUOTES, 'UTF-8') ?>"> | ||
| <input type="hidden" name="StateTextbox" value="<?= htmlentities(stripslashes($sState), ENT_NOQUOTES, 'UTF-8') ?>"> |
There was a problem hiding this comment.
Security: The ENT_NOQUOTES flag in htmlentities() is unsafe for HTML attributes. Use InputUtils::escapeAttribute() instead (which internally uses ENT_QUOTES), consistent with the other hidden fields on lines 977-979, 982-983.
| <input type="hidden" name="StateTextbox" value="<?= htmlentities(stripslashes($sState), ENT_NOQUOTES, 'UTF-8') ?>"> | |
| <input type="hidden" name="StateTextbox" value="<?= InputUtils::escapeAttribute(stripslashes($sState)) ?>"> |
src/Include/Functions.php
Outdated
| '<div class="input-group-prepend">' . | ||
| '<span class="input-group-text"><i class="fa-solid fa-font"></i></span>' . | ||
| '</div>' . | ||
| '<input class="form-control" type="text" id="' . $fieldname . '" name="' . $fieldname . '" maxlength="50" value="' . htmlentities(stripslashes($data), ENT_NOQUOTES, 'UTF-8') . '">' . |
There was a problem hiding this comment.
Security: Use ENT_QUOTES instead of ENT_NOQUOTES when using htmlentities(). The ENT_NOQUOTES flag does not escape single or double quotes, which could lead to XSS vulnerabilities when values are used in HTML attributes. This affects multiple field types (3, 4, 5, 11).
src/Include/Functions.php
Outdated
| '<div class="input-group-prepend">' . | ||
| '<span class="input-group-text"><i class="fa-solid fa-align-left"></i></span>' . | ||
| '</div>' . | ||
| '<textarea class="form-control" id="' . $fieldname . '" name="' . $fieldname . '" rows="2" maxlength="100">' . htmlentities(stripslashes($data), ENT_NOQUOTES, 'UTF-8') . '</textarea>' . |
There was a problem hiding this comment.
Security: Use ENT_QUOTES instead of ENT_NOQUOTES when using htmlentities(). The ENT_NOQUOTES flag does not escape single or double quotes, which could lead to XSS vulnerabilities when values are used in HTML attributes.
src/Include/Functions.php
Outdated
| echo '<div class="input-group-prepend">'; | ||
| echo '<span class="input-group-text"><i class="fa-solid fa-dollar-sign"></i></span>'; | ||
| echo '</div>'; | ||
| echo '<input class="form-control" type="text" id="' . $fieldname . '" name="' . $fieldname . '" maxlength="13" value="' . $data . '">'; |
There was a problem hiding this comment.
The money field (type 10) prints $data directly into the value attribute without escaping: value="' . $data . '". If $data contains quotes or HTML, it can break the attribute and inject script (stored XSS). Escape attribute values, e.g., InputUtils::escapeAttribute($data):
echo '<input class="form-control" type="text" id="' . $fieldname . '" name="' . $fieldname . '" maxlength="13" value="' . InputUtils::escapeAttribute($data) . '">';| echo '<input class="form-control" type="text" id="' . $fieldname . '" name="' . $fieldname . '" maxlength="13" value="' . $data . '">'; | |
| echo '<input class="form-control" type="text" id="' . $fieldname . '" name="' . $fieldname . '" maxlength="13" value="' . InputUtils::escapeAttribute($data) . '">'; |
src/Include/Functions.php
Outdated
| '<div class="input-group-prepend">' . | ||
| '<span class="input-group-text"><i class="fa-solid fa-calendar-days"></i></span>' . | ||
| '</div>' . | ||
| '<input class="form-control" type="text" id="' . $fieldname . '" name="' . $fieldname . '" maxlength="4" value="' . $data . '" placeholder="YYYY">' . |
There was a problem hiding this comment.
The year field (type 6) writes $data directly into the value attribute without escaping: value="' . $data . '". This allows attribute-breaking and stored XSS if the value contains quotes/HTML. Use attribute-safe escaping, e.g., InputUtils::escapeAttribute($data):
echo '<input class="form-control" type="text" id="' . $fieldname . '" name="' . $fieldname . '" maxlength="4" value="' . InputUtils::escapeAttribute($data) . '" placeholder="YYYY">';| '<input class="form-control" type="text" id="' . $fieldname . '" name="' . $fieldname . '" maxlength="4" value="' . $data . '" placeholder="YYYY">' . | |
| '<input class="form-control" type="text" id="' . $fieldname . '" name="' . $fieldname . '" maxlength="4" value="' . InputUtils::escapeAttribute($data) . '" placeholder="YYYY">' . |
src/Include/Functions.php
Outdated
| '<div class="input-group-prepend">' . | ||
| '<span class="input-group-text"><i class="fa-solid fa-hashtag"></i></span>' . | ||
| '</div>' . | ||
| '<input class="form-control" type="text" id="' . $fieldname . '" name="' . $fieldname . '" maxlength="11" value="' . $data . '">' . |
There was a problem hiding this comment.
The integer field (type 8) echoes $data directly into the value attribute without escaping: value="' . $data . '". If $data contains quotes or special characters, this can lead to attribute injection and stored XSS. Escape with InputUtils::escapeAttribute($data):
echo '<input class="form-control" type="text" id="' . $fieldname . '" name="' . $fieldname . '" maxlength="11" value="' . InputUtils::escapeAttribute($data) . '">';| '<input class="form-control" type="text" id="' . $fieldname . '" name="' . $fieldname . '" maxlength="11" value="' . $data . '">' . | |
| '<input class="form-control" type="text" id="' . $fieldname . '" name="' . $fieldname . '" maxlength="11" value="' . InputUtils::escapeAttribute($data) . '">' . |
| echo '<option value="' . $fam_ID . '"'; | ||
| if ($iFamily === $fam_ID || $queryParamFamilyId === $fam_ID) { | ||
| echo ' selected'; | ||
| } | ||
|
|
||
| echo 'value="' . InputUtils::escapeAttribute(stripslashes($sZip)) . '" '; ?> | ||
| maxlength="10" size="8"> | ||
| </div> | ||
| <div class="form-group col-md-2"> | ||
| <label for="Zip"> | ||
| <?php if ($bFamilyCountry) { | ||
| echo '<span class="text-danger">'; | ||
| echo '>' . $fam_Name . ' ' . FormatAddressLine($fam_Address1, $fam_City, $fam_State); | ||
| } ?> |
There was a problem hiding this comment.
The option label is rendered using unescaped variables $fam_Name, FormatAddressLine($fam_Address1, $fam_City, $fam_State). If any of these database fields contain HTML or quotes, they can inject markup/JS into the page (stored XSS). Escape the output before concatenation, e.g., use InputUtils::escapeHtml() (or htmlentities) for each field:
echo '>' . InputUtils::escapeHtml($fam_Name) . ' ' . InputUtils::escapeHtml(FormatAddressLine($fam_Address1, $fam_City, $fam_State));Security fixes in Functions.php: - Replace ENT_NOQUOTES with ENT_QUOTES in htmlentities() calls (types 3,4,5,11) - Add InputUtils::escapeAttribute() for year field (type 6) - Add InputUtils::escapeAttribute() for integer field (type 8) - Add InputUtils::escapeAttribute() for money field (type 10) Security fixes in PersonEditor.php: - Use InputUtils::escapeAttribute() for hidden State/StateTextbox fields - Escape family name and address in dropdown with InputUtils::escapeHTML() Accessibility fixes in PersonEditor.php: - Add title attributes to all FAB buttons for consistency - Add role="button" to FAB buttons with href="#"
- standard.person.new.spec.js: Replace #PersonSaveButton and #EditPerson with .fab-save and .fab-edit - standard.person.profile.spec.js: Replace #addNote with .fab-note - standard.person.search.spec.js: Replace #PersonSaveButton with .fab-save
What Changed
Type
Screenshots
New
Security Check
Code Quality
Pre-Merge