Override target attributes in anchors in templates. #1587
Conversation
| } else if (!target) { | ||
| // TODO(dvoytenko, #1572): Change this once sanitization of targets is | ||
| // addressed. The most correct value is probably _top. | ||
| updatedTarget = '_blank'; |
There was a problem hiding this comment.
I think you have these conditionals confused in the comment: anything without a target should behave as target="_top" and anything with should temporarily be assigned to target="_blank".
PS. can you swap the conditionals to avoid the !target-else double negation?
if (target == '_top' || target == '_blank') {
//...
} else if (target) {
// ...
} else {
// ...
}There was a problem hiding this comment.
Fixed double negation.
However, the conditions themselves are correct - the default temporarily is "_blank". We will have to switch it later (or completely remove this code) once the sanitization issue is addressed. It's documented in the comments.
dvoytenko
force-pushed
the
templ-target
branch
from
ea3f945 to
81142f5
Compare
|
|
||
| // TODO(dvoytenko, #1572): This code should be unnecessary after | ||
| // sanitization issue has been addressed. | ||
| anchor.setAttribute('target', '_blank'); |
There was a problem hiding this comment.
I think this'll also open new windows for fragment urls like #top and #section-1, too.
There was a problem hiding this comment.
This is ok since we process hash navigation separately in document-click.js. My hope, also, is to remove the whole thing once we resolve sanitizer problems.
|
One last comment, then LTGM. |
Override target attributes in anchors in templates.
Related to #1572.