Bug 67093 – [Qt] Default window.alert shows HTML entities in certain cases

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Farsmo]

window.alert('&') shows &
window.alert('"') shows "

However when the string contains '<', HTML entities become correctly interpreted again:

window.alert('<good> &" ...') shows <good> &" ...

If you want an example look at the QString slot test in https://bugs.webkit.org/show_bug.cgi?id=67092 .

[Steffen Imhof]

Created attachment 143039 [details]
Patch to fix the display of alert(), confirm() and prompt()

The problem is caused by the HTML escaping introduced in https://bugs.webkit.org/show_bug.cgi?id=34429, which in turn was added to make sure HTML tags are shown in the alert boxes. The root cause for me seem to be Qt's text detection automatisms. So the text is sometimes interpreted as plain text and sometimes as rich text (with a few possible HTML tags).
In my opinion the correct solution here is to force plain text. (As far as I can tell, inside the message boxes this should not pose any security risk?!)
The static convenience methods cannot be used anymore in this case, so the code grew a bit.
QInputDialog does not provide an API to access the contained label, so I had to resort to some "less-than-nice" meta object inspection for "prompt()". A possible alternative would be to write our own dialog for this case, but I think this would be overkill.
A somewhat related problem is fixed with this patch, too: QLabel interprets '&' characters as the start of a mnemonic identifier, dropping the '&' and underlining the next character. This is worked around by doubling the '&'s beforehand.
I haven't seen yet, how to test this automatically, but I don't think a test should be necessary for this fix. (If I am wrong here, I'd be glad for some pointers how one would write such a test case.)

[Daniel Bates]

Comment on attachment 143039 [details]
Patch to fix the display of alert(), confirm() and prompt()

View in context: https://bugs.webkit.org/attachment.cgi?id=143039&action=review

> Source/WebKit/qt/Api/qwebpage.cpp:2109
> +    box.setWindowTitle(tr("JavaScript Alert - %1").arg(mainFrame()->url().host()));

The window title is disingenuous. This is a JavaScript confirm dialog. Previously this was reflected in the window title.

[Steffen Imhof]

Created attachment 143208 [details]
Patch with fix for comment #2

(In reply to comment #2)
> The window title is disingenuous. This is a JavaScript confirm dialog. Previously this was reflected in the window title.

Oops, good catch. I fixed this in the updated patch.

[Simon Hausmann]

Comment on attachment 143208 [details]
Patch with fix for comment #2

I think the patch itself is correct, but there's one little glitch that I think should be fixed before landing. The current order of things is

    setText(msg)
    setTextFormat(Qt::PlainText);

In theory the order of setting properties shouldn't matter, and in fact in terms of correctness it won't make a difference here. But in terms of performance a setText(msg) call with text that contains tags will with QLabel under the hood result in the allocation of a relatively heavy QTextControl instance. If you call setTextFormat(Qt::PlainText) _before_ calling setText, then we can avoid the creation and sub-sequent deletion of the QTextControl inside.

[Steffen Imhof]

Created attachment 143756 [details]
Updated patch, setting plain text mode first in all 3 cases

Updated the patch to address Simon's concerns.

[Simon Hausmann]

Comment on attachment 143756 [details]
Updated patch, setting plain text mode first in all 3 cases

r=me

[WebKit Review Bot]

Comment on attachment 143756 [details]
Updated patch, setting plain text mode first in all 3 cases

Clearing flags on attachment: 143756

Committed r118354: <http://trac.webkit.org/changeset/118354>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.