Updated: Comment #N
Problem/Motivation
Add nid field using views ui.
See double escaped dialog title.
This also affects standard core dialogs like e.g. the following issue that wants to use dialogs for delete confirmation forms: #2253257: Use a modal for entity delete operation links or any other dialog that e.g. contains em tags, characters that are encoded when displayed (like ') and so on.
Proposed resolution
The solution in the last patch #44 is to convert the HTML into simple plain text using PlainTextOutput::renderFromHtml()
for the title. It returns a string with HTML tags stripped and HTML entities decoded, which is what we need.
Remaining tasks
User interface changes
Dialog titles that contain HTML tags or characters that would be escaped like < or ' are displayed correctly.
API changes
None.
Comment | File | Size | Author |
---|---|---|---|
#64 | 2207247-64.patch | 6.94 KB | joelpittet |
#61 | 2207247-8.2-modal-dialog-dbl.61.patch | 9.94 KB | larowlan |
#61 | interdiff.txt | 2.63 KB | larowlan |
Comments
Comment #1
dawehnerThank you for posting. I will have a look at it.
Comment #2
dawehnerHa, I debugged that until it went to javascript.
Are we sure that the dialog API does not try to do shit here?
Comment #3
nod_If it is, it's jQuery UI Dialog, we're not doing anything special with the title on our end.
Comment #4
dawehnerWrong screenshot ...
Comment #5
nod_Checked, jQuery UI does that.
Comment #6
nod_Using this solves the problem.
Since we're already extending the dialog in #2113911: Modal style update it'd make sense to wrap that up in there.
Comment #7
dawehnerAs written in IRC I think it would be totally fine to remove the HTML for the views title, though I consider it as a bad to deal with sanitization in javascript instead of PHP.
Comment #8
olli CreditAttribution: olli commented#2126983-8: ConfigItemGroup form double escapes the form #title would remove the html in views, but something like #6 might be better. Is it safe to use options.title or should we add options.drupalTitle?
Comment #9
nod_I'd rather strip out the tags. Page titles can't have HTML in them.
Comment #10
dawehnerBut dialog titles can ... For such long titles as views UI has it is at least a little bit helpful.
Comment #11
larowlanAlso impacts #2254935: Use a modal for content entity form delete links confirmation forms
Comment #12
larowlanshould be red/green
Comment #14
dawehner@larowlan
I would pretty much prefer an actual fix on the JS side but yeah if this is not possibe, let's wait on the other issue and write a dedicated test?
Comment #15
olli CreditAttribution: olli commentedWhile waiting, added decodeEntities().
Comment #16
olli CreditAttribution: olli commentedAnd here is #6 with drupalTitle for views.
Comment #17
LewisNyman CreditAttribution: LewisNyman commentedComment #18
Jalandhar CreditAttribution: Jalandhar commentedPatch needs reroll.
Comment #19
rpayanmComment #20
jhedstromNeeds another reroll. Bumping to at least normal. Double-escaping on the UI might even qualify as major?
Comment #21
BerdirHad to create the patch from scratch, everything changed ;)
- Also added it to the modal renderer.
- Moved the strip_tags() call down to $title so that it runs for both #title and page title resolver calls.
I'm not exactly sure what else needs to be done here. The patch adds support for unescaped titles, but only views uses it, should we use that by default, combined with an Xss::filter() or so?
Comment #22
Wim LeersDefinitely needs docs to explain what it's doing and especially why.
Wouldn't something like
rawTitle
be more appropriate?We don't yet have test coverage for this?
Comment #25
mrjmd CreditAttribution: mrjmd commentedThis is a straight reroll of #21, so the feedback from #22 still need to be sorted.
Comment #26
s_leu CreditAttribution: s_leu at MD Systems GmbH commentedThe patch in #25 still applied but didn't work anymore. For some reason i found that the JS part isn't necessary anymore and the dialog titles are displayed properly without it as well, thus i removed it.
Didn't add tests for this yet as i wasn't really sure what needs to be tested now with the new patch. Could you give me a hint Wim?
Comment #27
sasanikolic CreditAttribution: sasanikolic at MD Systems GmbH commentedAdded the test for views. Still need to extend the \Drupal\system\Tests\Ajax\DialogTest.
Comment #28
sasanikolic CreditAttribution: sasanikolic at MD Systems GmbH commentedRemoved the html tags from the title in the routing file, since it doesn't matter. Instead, added the tags in the AjaxTestController, so we actually test them in the DialogTest.
Comment #29
sasanikolic CreditAttribution: sasanikolic at MD Systems GmbH commentedTests only patch.
Comment #31
jhedstromTest only patch was expected to fail.
Comment #32
joelpittetAttaching to a meta.
Comment #33
joelpittetThe meta has little to do with this @Berdir pointed out to me and I kinda knew when I added. Just want to give this some meta visibility, if there is another escaping meta floating around please swap it out.
Comment #34
joelpittetMay want to put in a
&
in this to show that we aren't getting&
as well. And add a couple tests to assert that there is no '&
'?We are asserting there is no-escaping, not double escaping because we are stripping out the tags and decoding all the entities that may have been escaped.
Comment #35
sasanikolic CreditAttribution: sasanikolic at MD Systems GmbH commentedAdded the test for & and fixed the comment.
Comment #36
maijs CreditAttribution: maijs at Wunder commentedAdding another two issues which address the same problem...
Comment #37
maijs CreditAttribution: maijs at Wunder commentedComment #38
dawehnerYeah let's certainly mark this issue as duplicate. Having more issues doesn't really help :)
Comment #39
BerdirWe might need some more discussion on which issue is a duplicate of what?
The other issues are not addressing the tags problem for example, they only do decodeEntities().
Should we maybe treat them as plain text and use PlainTextOutput?
Comment #40
dawehnerSee https://www.drupal.org/node/2207247#comment-8533791
Comment #41
dawehnerI guess we have to? Too bad that the nice functionality of using a limited list of HTML tags got lost.
Comment #42
Truptti CreditAttribution: Truptti at Axelerant commentedThe patch "dialog_titles_double-2207247-35.patch" in #35 has errors.Attaching snapshot for reference.
Updating the status to Needs Work.
Comment #43
Truptti CreditAttribution: Truptti at Axelerant commentedComment #44
sasanikolic CreditAttribution: sasanikolic at MD Systems GmbH commentedRerolled the patch and changed
decodeEntities()
torenderFromHtml()
.Comment #45
sasanikolic CreditAttribution: sasanikolic at MD Systems GmbH commentedProviding a screenshot to prove that the title is now displayed without any HTML markups and updating the issue summary.
Comment #46
BerdirUpdating the issue summary a bit more.
We have tests and we have been using the patches from this issue in our news portal distribution where we are displaying nodes in a dialog for more than a year now. Never had any problems with it.
This is tagged Javascript, but we're not actually touching any JS anymore, so I think I can RTBC this.
Comment #47
BerdirComment #48
alexpottSo it is jQuery that is causing the double escaping? I wonder why we're not putting this behaviour in the constructor of
OpenDialogCommand
? Also I think we should be detecting if$title
is an object that implementsMarkupInterface
and only doing thePlainTextOutput::renderFromHtml()
then since that is the only time when we know we are doing what we are supposed to do.If the above approach is wrong (and even it it is not) we need to document the expectation on both
OpenDialogCommand
andOpenModalDialogCommand
.Comment #49
BerdirYes, jQuery does the escaping AFAIK. So we need to send the unescaped HTML/plain text.
I also don't think that we should limit it to MarkupInterface. Any kind of input should be stripped of HTML tags and html encoded entities should be decoded, it's not relevant if it's safe or not.
Comment #52
larowlanReroll and fixes as per #48
Comment #53
larowlanComment #55
larowlanmeh
Comment #56
nod_Comment #57
dawehnerWe lack some documentation on those ajax commands what we are actually doing with the passed in title, as per #48
Comment #58
larowlanYep
Comment #59
joelpittetThanks for adding more details to the docs, sending back to RTBC.
Comment #60
dawehnerI don't want to be anal about it, but this describes what its doing vs. why. We should describe that the JS does its own escaping, so we prevent double escaping by doing that.
Comment #61
larowlanFor you, sure
Comment #62
dawehnerThank you @larowlan!
Comment #63
alexpottFrom @Berdir...
I think I was wrong in #48 and @berdir was right... sorry.
Comment #64
joelpittet@alexpott was this what you had in mind, I agree just strip tags on all things going into the modals and other js titles.
Comment #65
larowlanlets do it
Comment #66
alexpottCommitted and pushed 8f3e25c to 8.3.x and 3818756 to 8.2.x. Thanks!
Asserting not equal and equal on the same thing is a bit weird... I'm removing the assertNotEqual.