Email styler for Lite

[truongwp]

Handoff: https://linear.app/strategy11/document/feature-handoff-9558d9fdc708

Pro PR: https://github.com/Strategy11/formidable-pro/pull/5965

This PR adds new email styles settings in Global settings and Send mail action.

[coderabbitai]

Actionable comments posted: 0

♻️ Duplicate comments (1)

classes/models/FrmTableHTMLGenerator.php (1)

92-98: Sanitize and validate cell_padding and width to prevent style injection.

Both properties are interpolated into HTML attributes without validation, which allows style injection. While esc_attr() is applied at output, it doesn't prevent malicious CSS values (e.g., expressions, data URIs, or invalid values that break layout).

Apply this diff to validate both properties:

 if ( isset( $atts['cell_padding'] ) ) {
-  $this->cell_padding = $atts['cell_padding'];
+  $padding = sanitize_text_field( $atts['cell_padding'] );
+  // Accept 1–4 CSS lengths (px|em|rem|%).
+  if ( preg_match( '/^\s*\d+(\.\d+)?(px|em|rem|%)' .
+                   '(\s+\d+(\.\d+)?(px|em|rem|%)){0,3}\s*$/', $padding ) ) {
+    $this->cell_padding = $padding;
+  }
 }

 if ( isset( $atts['width'] ) ) {
-  $this->width = $atts['width'];
+  $width = sanitize_text_field( $atts['width'] );
+  // Allow percentages or pixels only.
+  if ( preg_match( '/^\s*(\d+(\.\d+)?%|\d+px)\s*$/', $width ) ) {
+    $this->width = $width;
+  }
 }

🧹 Nitpick comments (2)

classes/models/FrmTableHTMLGenerator.php (2)

56-72: Update version placeholders before release.

The @since x.x tags should be replaced with the actual version number when this feature is released.

---

216-225: Make border removal resilient to whitespace variations.

The str_replace() approach is brittle and will fail if the border style has extra whitespace or a missing trailing semicolon (e.g., border-top: 1px solid #ddd or when it's the last property).

Apply this diff to use a regex pattern that tolerates formatting variations:

 public function remove_border( $html, $position = 'top' ) {
-  $search = sprintf(
-    'border-%1$s:%2$s solid %3$s;',
-    $position,
-    $this->style_settings['border_width'],
-    $this->style_settings['border_color']
-  );
-
-  return str_replace( $search, '', $html );
+  $pattern = sprintf(
+    '/border-%s:\s*%s\s+solid\s+%s;?/',
+    preg_quote( $position, '/' ),
+    preg_quote( $this->style_settings['border_width'], '/' ),
+    preg_quote( $this->style_settings['border_color'], '/' )
+  );
+  return preg_replace( $pattern, '', $html );
 }

Based on past review comments.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 49bb3f5 and f483047.

📒 Files selected for processing (1)

• classes/models/FrmTableHTMLGenerator.php (6 hunks)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)

• GitHub Check: PHP 7.4 tests in WP trunk
• GitHub Check: PHP 8 tests in WP trunk
• GitHub Check: Cypress

🔇 Additional comments (4)

classes/models/FrmTableHTMLGenerator.php (4)

176-176: LGTM with proper escaping applied.

The table style now includes border-spacing and properly escapes attributes with esc_attr(). The width attribute is also correctly escaped. Ensure the width validation from the previous comment is addressed.

Also applies to: 185-187

---

200-200: LGTM - attribute escaping properly applied.

The $td_style now correctly uses esc_attr() to escape the entire style attribute, addressing the security concern from previous reviews. The dynamic cell_padding is integrated correctly, pending validation in the constructor.

Based on past review comments.

Also applies to: 203-203

---

274-289: LGTM - visibility change is appropriate.

Changing tr_style() from protected to public allows external components to generate style-consistent table rows. The method logic remains unchanged and correct.

---

291-311: LGTM - getter methods follow standard accessor pattern.

The new public getters get_table_style() and get_td_style() provide clean external access to the computed styles. Implementation is straightforward and correct.

[Crabcyborg]

Thanks @truongwp!

I think we can go ahead and merge the email styler update.

🚀

[truongwp]

@Crabcyborg What do you think about the current email style previewing data? Is it ok?

[Crabcyborg]

@truongwp I think the preview content looks good enough for now.

[coderabbitai]

Actionable comments posted: 1

♻️ Duplicate comments (2)

classes/views/frm-settings/email/settings.php (2)

24-28: Add keyboard and screen reader accessibility to the upgrade overlay.

The upgrade overlay is not accessible to keyboard or screen reader users. Add role, tabindex, and aria-label attributes as previously suggested.

 $upgrade_attrs = array(
 	'data-upgrade' => __( 'Email Styles Settings', 'formidable' ),
 	'class'        => 'frm_show_upgrade',
 	'style'        => 'position:absolute;top:0;right:0;bottom:0;left:0;',
+	'role'         => 'button',
+	'tabindex'     => '0',
+	'aria-label'   => __( 'Upgrade to PRO to edit email styles', 'formidable' ),
 );

---

121-127: Add a label for the Typography select.

The disabled Typography select has no associated label, which is an accessibility issue. Add a visible label as previously suggested.

 <td class="frm-pt-xl">
 	<div class="frm_grid_container" style="position:relative;">
 		<div class="frm6">
+			<label for="frm-email-typography-font" style="margin-bottom:4px;"><?php esc_html_e( 'Font', 'formidable' ); ?></label>
-			<select disabled>
+			<select id="frm-email-typography-font" disabled>
 				<option><?php esc_html_e( 'Sans serif', 'formidable' ); ?></option>
 			</select>
 		</div>

🧹 Nitpick comments (1)

classes/views/frm-settings/email/settings.php (1)

87-111: Consider adding explicit IDs to color picker buttons for label association.

While the labels are adjacent to the color pickers (acceptable for accessibility), adding explicit for attributes on labels and corresponding id attributes on the color picker buttons would improve programmatic association for assistive technologies.

Example for the first picker:

-<label style="margin-bottom:4px;"><?php esc_html_e( 'Email Background', 'formidable' ); ?></label>
+<label for="frm-email-bg-color" style="margin-bottom:4px;"><?php esc_html_e( 'Email Background', 'formidable' ); ?></label>

Then update the helper to accept an optional ID parameter, or wrap each picker in a fieldset with legend.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a76bb31 and f60c848.

📒 Files selected for processing (1)

• classes/views/frm-settings/email/settings.php (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

classes/views/frm-settings/email/settings.php (3)

classes/helpers/FrmTipsHelper.php (2)

• FrmTipsHelper (6-456)
• show_tip (43-77)

classes/helpers/FrmAppHelper.php (2)

• FrmAppHelper (6-4623)
• array_to_html_params (1367-1376)

classes/controllers/FrmSettingsController.php (2)

• FrmSettingsController (6-498)
• fake_color_picker (489-497)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)

• GitHub Check: PHP 8 tests in WP trunk
• GitHub Check: PHP 7.4 tests in WP trunk
• GitHub Check: Cypress

🔇 Additional comments (1)

classes/views/frm-settings/email/settings.php (1)

31-79: LGTM! Header Image section is well-structured.

The section has proper label-control associations, appropriate disabled states, and correct upgrade overlays. The typo fix for "Outside" has been applied.