Polish HTML structure of the response in the res.redirect() function#5167
Polish HTML structure of the response in the res.redirect() function#5167bjohansebas merged 6 commits intoexpressjs:masterfrom
Conversation
|
Your changes look good to me, you just missed the tests for the redirect body in |
Hi, I have updated the test file, could you review it again? Thank you! |
|
Looks good now, but I'm not the maintainer. |
|
Hi @Bernice55231, would you like to resolve the conflicts in this PR? |
|
Keep in mind GHSA-qw6h-vgh9-j6wx when handling this conflict to prevent regressions :) |
There was a problem hiding this comment.
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
lib/response.js:840
- The string concatenation split into multiple lines may risk issues with automatic semicolon insertion in JavaScript; consider moving the '+' operator to the end of the preceding line to ensure the concatenation works as intended.
+ '<body><p>' + statuses.message[status] + '. Redirecting to ' + u + '</p></body>'
…rowser compatibility
|
thanks @Bernice55231! |
jonchurch
left a comment
There was a problem hiding this comment.
The change itself is fine, but wanted to leave context here for the future.
The justification for this PR is not accurate per the description nor the linked issue.
To be clear, no browser will fail to render the previous html.
The linked issue specifically wanted to allow the title to be seen in case the user's browser was blocking redirects, or if it launched an app and left the redirect page visible.
The title will now be "Found" for the default (302) which isnt much of an improvement over having the URL. But thats related to the issue not so much this PR.
However, since the PR leaves out the <html> tag, this still technically isnt valid HTML5.
But that does not matter, as browsers will render that for you.
|
Good catch, I hadn’t realized that the HTML tag was still missing. |
|
Thank you for everyone's cooperation!
However, from an accessibility perspective, it is better to at least include the start tag and set the |
Fixed #5058
To prevent the issue of not showing the DOM body in the old-versioned browser or HTTPServer in redirecting method, it is better to have the
<!DOCTYPE html>' and<title>` elements in the response body. If we use an old-versioned browser, it may not automatically fulfill the correct HTML file and thus show only plain text on that page if it does not succeed in redirecting.