EditorLinkDialogtarget attribute on <a> is not whitelisted by default (see #2549077: Allow the "Limit allowed HTML tags" filter to also restrict HTML attributes, and only allow a small whitelist of attributes by default), using that checkbox has no effect: after filtering, the target attribute is removedtarget doesn't actually make sense in the vast majority of use cases. So why even have it? (See #3 for more nuance.)(Opened on behalf of @DuaelFr — see #2585173-17: [regression] "Allowed HTML tags" setting corrupted upon accessing Text Format configuration UI.): Should we allow
<a target> in the Standard install profile?
Remove the "Open in new window" checkbox from EditorLinkDialog.
TBD
The "Open in new window" checkbox no longer shows up in EditorLinkDialog. But it doesn't work anyway.
None.
None.
EditorLinkDialogtarget attribute on <a> is not whitelisted by default (see #2549077: Allow the "Limit allowed HTML tags" filter to also restrict HTML attributes, and only allow a small whitelist of attributes by default), using that checkbox has no effect: after filtering, the target attribute is removedtarget doesn't actually make sense in the vast majority of use cases. So why even have it? (See #3 for more nuance.)We don't want to ship with broken functionality; and there is also no point in shipping with functionality 99% doesn't need.
| Comment | File | Size | Author |
|---|---|---|---|
| #13 | remove_target_drupallink-2590403-13.patch | 1.2 KB | duaelfr |
| #6 | 2590403-before.png | 7.03 KB | duaelfr |
| #6 | 2590403-after.png | 5.75 KB | duaelfr |
| #5 | remove_open_in_new_window_checkbox-2590403-5.patch | 784 bytes | wim leers |
Comments
Comment #2
duaelfrAs standard profile is about demonstrating what's available in Core, I think we should.
If we don't, we should remove the "Open in a new windows" checkbox from the EditorLinkDialog and let contrib deal with it.
Comment #3
wim leersI think we should not allow the
targetattribute by default.Rationale: core is about providing sane defaults, not showing everything that is possible. Very few sites actually need to use the
targetattribute. Basic HTML is about providing the minimal WYSIWYG editor that still covers the >95% case.Longer rationale: offering this option for every link makes very little sense. You're forcing authors to think about this for every link they create. That's bad, and unnecessary for >90% of users. If sites want to force all links in a formatted text field to have a certain target, they should instead just add a filter that adds the
targetattribute.Conclusion: Therefore, I'm +1 for #2's proposal to remove the checkbox from
EditorLinkDialogand let contrib deal with it.Comment #4
rootwork@Wim Leers I agree. Can't really think of anything to add :)
Comment #5
wim leersAlright. Let's do this then.
So, rationale:
EditorLinkDialogtargetattribute on<a>is not whitelisted by default, using that checkbox has no effect: after filtering, thetargetattribute is removedtargetdoesn't actually make sense in the vast majority of use cases. So why even have it? (See #3 for more nuance.)Comment #6
duaelfrI had to apply #2598070-21: [regression] CKEditor Link button does not show if HTML filtering is enabled to make that test as the link button does not appear on the editor right now.
Everything is good for me. Let's RTBC and let the testbot quietly end its tests.
We also need a "rc target evaluation" but I don't know how to argue about removing that thing that used to work in beta15.
Before:
After:
Comment #7
wim leers#2549077: Allow the "Limit allowed HTML tags" filter to also restrict HTML attributes, and only allow a small whitelist of attributes by default caused this. Which was a super important security improvement.
The reason it worked before that patch/issue: because we weren't filtering attributes yet.
Comment #8
duaelfrWell done!
Testbot is happy, let's hope that's going to become a propre rc target soon :)
Thank you for your attention @Wim Leers
Comment #9
catchDiscussed with @alexpott @xjm and @effulgentsia
We agree with removing the checkbox since _target isn't a feature core should support (contrib would be welcome to of course).
One question that came up is whether existing content with the _target attribute set could be affected by the change, we think not because the form is merged with any attributes already on the link.
Tagging rc target.
Comment #10
catchCommitted/pushed to 8.0.x, thanks!
Comment #12
wim leersThanks! This also allowed me to close #2578957: (regression) "Open in new window" checkbox does not work anymore, and will other CKEditor issues move forward!
Comment #13
duaelfrWe forgot to remove all references to the target attribute in core/modules/ckeditor/js/plugins/drupallink/plugin.js
Comment #14
wim leersCan you please open a follow-up issue for that? We avoid reopening issues like this. It can just be plus a link to #13. Thanks.
Comment #15
duaelfrFollow-up opened #2608722: Follow-up for #2590403: Remove forgotten references to the target attribute in the drupallink CKEditor plugin
Comment #16
wim leersThanks, replied!