HTML API: Respect newlines setting modifiable HTML#25
HTML API: Respect newlines setting modifiable HTML#25
Conversation
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * If the intention is to start with a leading newline, ensure that it is preserved | ||
| * by adding an additional leading newline. | ||
| */ | ||
| if ( 'TEXTAREA' === $this->get_tag() && str_starts_with( $plaintext_content, "\n" ) ) { |
There was a problem hiding this comment.
The check only handles leading \n but should also handle leading \r and \r\n to be consistent with the text node handling on lines 3779-3784. When users provide content starting with \r or \r\n, these will be normalized to \n during HTML parsing, and the leading newline will be incorrectly stripped. To fix this, consider using the same approach as line 3781: check if the first character is a newline variant using 1 === strspn( $plaintext_content, "\n\r", 0, 1 ) or check for both "\n" and "\r" explicitly.
| if ( 'TEXTAREA' === $this->get_tag() && str_starts_with( $plaintext_content, "\n" ) ) { | |
| if ( 'TEXTAREA' === $this->get_tag() && 1 === strspn( $plaintext_content, "\n\r", 0, 1 ) ) { |
| public function test_modifiable_text_special_textarea() { | ||
| $processor = new WP_HTML_Tag_Processor( '<textarea></textarea>' ); | ||
| $processor->next_token(); | ||
| $processor->set_modifiable_text( "\nAFTER NEWLINE" ); | ||
| $this->assertSame( | ||
| "\nAFTER NEWLINE", | ||
| $processor->get_modifiable_text(), | ||
| 'Should have preserved the leading newline in the content.' | ||
| ); | ||
| } |
There was a problem hiding this comment.
The test coverage for TEXTAREA with leading carriage return (\r) and carriage return + newline (\r\n) is missing. Similar to the WP_HTML_Processor tests, this test should also cover these newline variants to ensure consistent behavior across different newline representations.
@todo split into data provider
sirreal
force-pushed
the
html-api/set-mod-html-leading-newlines
branch
from
749ad1b to
b7ddbb4
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
| /** | ||
| * TEXTAREA elements ignore the first newline in their content. | ||
| * Setting the modifiable text with a leading newline should ensure that the leading newline | ||
| * is present in the resulting TEXTAREA. | ||
| * | ||
| * @ticket 64609 | ||
| */ | ||
| public function test_modifiable_text_special_textarea() { | ||
| $processor = WP_HTML_Processor::create_fragment( '<textarea></textarea>' ); | ||
| $processor->next_token(); | ||
| $processor->set_modifiable_text( "\nAFTER NEWLINE" ); | ||
| $this->assertSame( | ||
| "\nAFTER NEWLINE", | ||
| $processor->get_modifiable_text(), | ||
| 'Should have preserved the leading newline in the TEXTAREA content.' | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * TEXTAREA elements ignore the first newline in their content. | ||
| * Setting the modifiable text with a leading carriage return should be normalized | ||
| * and ensure the leading newline is present in the resulting TEXTAREA. | ||
| * | ||
| * @ticket 64609 | ||
| */ | ||
| public function test_modifiable_text_special_textarea_carriage_return() { | ||
| $processor = WP_HTML_Processor::create_fragment( '<textarea></textarea>' ); | ||
| $processor->next_token(); | ||
| $processor->set_modifiable_text( "\rCR" ); | ||
| // Newline normalization transforms \r into \n, and special handling should preserve it. | ||
| $this->assertSame( | ||
| "\nCR", | ||
| $processor->get_modifiable_text(), | ||
| 'Should have normalized carriage return and preserved the leading newline in the TEXTAREA content.' | ||
| ); | ||
| $this->assertEqualHTML( | ||
| "<textarea>\n\nCR</textarea>", | ||
| $processor->get_updated_html(), | ||
| '<body>', | ||
| 'Should have doubled the newline in the output HTML to preserve the leading newline.' | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * TEXTAREA elements ignore the first newline in their content. | ||
| * Setting the modifiable text with a leading carriage return + newline should be normalized | ||
| * and ensure the leading newline is present in the resulting TEXTAREA. | ||
| * | ||
| * @ticket 64609 | ||
| */ | ||
| public function test_modifiable_text_special_textarea_carriage_return_newline() { | ||
| $processor = WP_HTML_Processor::create_fragment( '<textarea></textarea>' ); | ||
| $processor->next_token(); | ||
| $processor->set_modifiable_text( "\r\nCR-N" ); | ||
| // Newline normalization transforms \r\n into \n, and special handling should preserve it. | ||
| $this->assertSame( | ||
| "\nCR-N", | ||
| $processor->get_modifiable_text(), | ||
| 'Should have normalized carriage return + newline and preserved the leading newline in the TEXTAREA content.' | ||
| ); | ||
| $this->assertEqualHTML( | ||
| "<textarea>\n\nCR-N</textarea>", | ||
| $processor->get_updated_html(), | ||
| '<body>', | ||
| 'Should have doubled the newline in the output HTML to preserve the leading newline.' | ||
| ); | ||
| } |
There was a problem hiding this comment.
@copilot Combine these three nearly-identical TEXTAREA tests into a single test using a data provider.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $processor = new WP_HTML_Tag_Processor( '<textarea></textarea>' ); | ||
| $processor->next_token(); | ||
| $processor->set_modifiable_text( "\nAFTER NEWLINE" ); | ||
| $this->assertSame( | ||
| "\nAFTER NEWLINE", |
There was a problem hiding this comment.
This test only asserts get_modifiable_text() after setting textarea content, but doesn’t assert the serialized output (get_updated_html()). Since the newline-stripping behavior is an HTML parsing/serialization concern (TEXTAREA drops the first LF), please also assert the updated HTML contains the extra leading newline needed to preserve the intended leading newline (and consider asserting next_token() / set_modifiable_text() return true to validate test setup).
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.