Bug 35028

Summary: [Qt] Add selectedHtml function to QWebView
Product: WebKit Reporter: Andreas Kling <kling>
Component: WebKit QtAssignee: Andreas Kling <kling>
Status: RESOLVED FIXED    
Severity: Enhancement CC: adawit, eric, faure, kenneth, laszlo.gombos, luiz, tonikitoo, vestbo, wstokes, yael
Priority: P2 Keywords: Qt
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Bug Depends on:    
Bug Blocks: 31552    
Attachments:
Description Flags
Proposed patch abarth: review+, kling: commit-queue-

Description Andreas Kling 2010-02-17 03:57:32 PST
Original bug report: http://bugreports.qt.nokia.com/browse/QTBUG-5525

toUpper and toLower

This is a feature request, not a bug report. I've been building what I
call a "RichTextEdit", aka a simple text editor built on top of an
editable QWebView. Things like bold, underline, italic all work pretty
well. I need one additional piece of functionality: toUpper and
toLower. There is no built in support for such functionality in
QWebKit, nor do I suggest there necessarily should be. But it would be
nice if there was a way for me to implement it myself. This is what I
do currently:

void RichTextEdit::toUpperCase() { execCommand("insertHTML", selectedText().toUpper()); }
void RichTextEdit::toLowerCase() { execCommand("insertHTML", selectedText().toLower()); }

void RichTextEdit::execCommand(const QString &cmd, const QString &arg)
{ QWebFrame *frame = page()->mainFrame(); QString js = QString("document.execCommand(\"%1\", false, \"%2\")").arg(cmd).arg(arg); std::string str = js.toStdString(); frame->evaluateJavaScript(js); }

Aka, I use the insertHTML function and simply replace the selected
text. The problem with this is:

1) I'd like to preserve the selection and
2) I lose complex internal formatting within the selection. For
example, if the text was

<b>One</b> Two <b>Three</b> Four <b>Five</b>

Add I select the entire thing and do a toUpper then the result is the
entire text becomes bolded. If I could get at the HTML, do my
modifications, and push that HTML back over the selected html, then
I'd be a very happy camper. I suspect that would work if I simply had
a function like

QString QWebView::sectedHTML()

and if the insertHTML javascript function provided a means to maintain
the selection (aka select the insertion if we're doing a replace).

If there is a way to improve my existing code without modifications to
Qt I'd love to hear about it. :)
Comment 1 Kenneth Rohde Christiansen 2010-09-24 09:22:40 PDT
why QWebView? What about QGraphicsWebView, or QWebPage?
Comment 2 Will Stokes 2010-09-24 09:44:52 PDT
You're right, this should probably lie farther down the food chain in something like QWebPage. What is fundamental is also being able to set the selected html as well, thus I'm looking for something like

QString QWebPage::selectedHTML() const;

void QWebPage::setSelectedHTML(QString val);

where the latter results in a web page with updated content and the updated portion still selected
Comment 3 David Faure 2010-09-24 13:26:16 PDT
Well, this is very similar to selectedText(), so I would say it should be both in QWebPage and QWebView, just like selectedText() currently. But yes, at least in QWebPage ;-)

IMHO let's keep "setSelectedHTML" out of this wish (i.e. make a separate one for it), it sounds quite more involved, than a simple "get me the selection as HTML" when there's already a "get me the selection as plain text".
Comment 4 Will Stokes 2010-09-24 13:49:56 PDT
David, you make a good point. FYI, this is how I currently convert a selection portion to upper case. I replace the selected portion. Aka I have to refer to javascript's insertHTML command...

QString text = m_webView->selectedText();
text = input.toUpper();

QWebFrame *frame = m_webView->page()->mainFrame();
QString js = QString("document.execCommand(\"%1\", false, \"%2\")").arg("insertHTML").arg(text);
frame->evaluateJavaScript(js);
Comment 5 Will Stokes 2010-09-24 13:50:49 PDT
Sorry, that should be:

QString text = m_webView->selectedText();
text = text.toUpper();

QString js = QString("document.execCommand(\"%1\", false, \"%2\")").arg("insertHTML").arg(text);

QWebFrame *frame = m_webView->page()->mainFrame();
frame->evaluateJavaScript(js);
Comment 6 Andreas Kling 2010-09-24 14:46:33 PDT
Created attachment 68755 [details]
Proposed patch

Added QWebView::selectedHtml() and QWebPage::selectedHtml()
Returned markup originates from the current selection range.
Comment 7 Adam Barth 2010-09-26 21:35:52 PDT
Comment on attachment 68755 [details]
Proposed patch

LGTM
Comment 8 Andreas Kling 2010-09-28 07:36:25 PDT
Comment on attachment 68755 [details]
Proposed patch

This needs API review before landing.
Comment 9 Dawit A. 2010-09-30 16:41:13 PDT
Just want to mention this here for the record. Already discussed it with Andreas on IRC, but I am not sure what the proper resolution should/would be:

The call to Range::toHTML() used to obtain the selected text will result in the internal style information, which is not part of the actual page, being included in the html fragment that is returned. This might be appropriate if your intention is to paste the selection into a Rich text editing environment, but that cannot be assumed for this API. At least I do not think. There are use cases such as page validation or text to speech where is additionally and rather ugly inclusion is not a necessary welcome.

Infact doing this issue was enough of a concern that someone put the following comments atop WebCore::createMarkup (WebCore/editing/markup.cpp), the function the Range::toHTML() function relies on to create the HTML representation:

// FIXME: Shouldn't we omit style info when annotate == DoNotAnnotateForInterchange? 
// FIXME: At least, annotation and style info should probably not be included in range.markupString()
Comment 10 Eric Seidel (no email) 2010-10-13 12:26:29 PDT
Attachment 68755 [details] was posted by a committer and has review+, assigning to Andreas Kling for commit.
Comment 11 Eric Seidel (no email) 2010-12-14 01:35:47 PST
What's left to be done here?
Comment 12 Andreas Kling 2011-01-07 05:59:53 PST
Committed r75242: <http://trac.webkit.org/changeset/75242>