Summary: | remove the support of Frame::isContentEditable and its dependencies | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chang Shu <cshu> | ||||||||||||||||
Component: | HTML Editing | Assignee: | Chang Shu <cshu> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | abarth, commit-queue, darin, eric, gustavo.noronha, gustavo, kevino, mrobinson, rniwa, simon.fraser, tonikitoo, webkit.review.bot, xan.lopez | ||||||||||||||||
Priority: | P2 | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | All | ||||||||||||||||||
OS: | All | ||||||||||||||||||
Bug Depends on: | |||||||||||||||||||
Bug Blocks: | 54290 | ||||||||||||||||||
Attachments: |
|
Description
Chang Shu
2011-02-11 10:15:41 PST
Created attachment 82142 [details]
fix patch
Comment on attachment 82142 [details]
fix patch
Is it possible to simultaneously remove EditorClient::clientIsEditable? It'll be nice if we did that in one patch to ensure no port is doing anything unusual in EditorClient::clientIsEditable.
Sure, will try. > Is it possible to simultaneously remove EditorClient::clientIsEditable? It'll be nice if we did that in one patch to ensure no port is doing anything unusual in EditorClient::clientIsEditable.
The only other place that uses this function is in WebCore/page/DragController.cpp, DragController::operationForLoad. Any idea to fix this?
(In reply to comment #4) > > Is it possible to simultaneously remove EditorClient::clientIsEditable? It'll be nice if we did that in one patch to ensure no port is doing anything unusual in EditorClient::clientIsEditable. > > The only other place that uses this function is in WebCore/page/DragController.cpp, DragController::operationForLoad. Any idea to fix this? That code is probably written by someone who doesn't understand editing. It's probably safe to replace it by document->inDesignMode() > > The only other place that uses this function is in WebCore/page/DragController.cpp, DragController::operationForLoad. Any idea to fix this?
>
> That code is probably written by someone who doesn't understand editing. It's probably safe to replace it by document->inDesignMode()
Excellent!
Created attachment 82262 [details]
fix patch 2
(In reply to comment #7) > Created an attachment (id=82262) [details] > fix patch 2 You should also remove EditorClient::clientIsEditable and remove its implementation from each port to verify that not calling EditorClient::clientIsEditable is self-evidently correct. Created attachment 82337 [details]
fix patch 3
Comment on attachment 82337 [details] fix patch 3 View in context: https://bugs.webkit.org/attachment.cgi?id=82337&action=review r- because this patch will break WebKit API in many ports. > Source/WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp:-486 > - return webkit_web_view_get_editable(m_webView); It seems like we're regressing GTK port this way. We need to modify webkit_web_view_set_editable/webkit_web_view_get_editable so that it properly turns on/off design-mode. > Source/WebKit/mac/WebCoreSupport/WebEditorClient.mm:-251 > - return [m_webView isEditable]; Ditto. We need to update setEditable in WebKit/mac/WebView/WebView.mm to turn on/off design mode for mac port as well. > Source/WebKit/qt/WebCoreSupport/EditorClientQt.cpp:-243 > - return m_page->isContentEditable(); We need to update QWebPage::setContentEditable as well. > Source/WebKit/wx/WebKitSupport/EditorClientWx.cpp:-207 > - if (webKitWin) > - return webKitWin->IsEditable(); We also need to modify Wx port as well but they're doing something funky like executing any editing command only if the frame is editable. We need someone who knows Wx port here. (In reply to comment #10) > > Source/WebKit/wx/WebKitSupport/EditorClientWx.cpp:-207 > > - if (webKitWin) > > - return webKitWin->IsEditable(); > > We also need to modify Wx port as well but they're doing something funky like executing any editing command only if the frame is editable. We need someone who knows Wx port here. This support was added a very long time ago, and looking over it, we can probably just remove the checks for IsEditable() in the wx commands that call Editor APIs. So IIUC, on the wx side we will need to remove the IsEditable() checks in wxWebKit APIs, then re-implement MakeEditable to call setDesignMode, and IsEditable to return whether document()->designMode() is set, is this right? Thanks! (In reply to comment #11) > This support was added a very long time ago, and looking over it, we can probably just remove the checks for IsEditable() in the wx commands that call Editor APIs. So IIUC, on the wx side we will need to remove the IsEditable() checks in wxWebKit APIs, then re-implement MakeEditable to call setDesignMode, and IsEditable to return whether document()->designMode() is set, is this right? Yeah, that makes sense. That'll align the implementation of Wx port with the rest. Kevin, any update on Wx side? I am also tied up on something else and will come back to this bug later. --Chang (In reply to comment #13) > Kevin, any update on Wx side? Sorry, I don't understand... Until this patch is landed, I can't make a patch for the wx port without breaking the build, can I? (In reply to comment #15) > (In reply to comment #13) > > Kevin, any update on Wx side? > > Sorry, I don't understand... Until this patch is landed, I can't make a patch for the wx port without breaking the build, can I? You can. Take a look at Qt, Mac, & Windows ports. They set design mode when editability is changed. (In reply to comment #16) > (In reply to comment #15) > > (In reply to comment #13) > > > Kevin, any update on Wx side? > > > > Sorry, I don't understand... Until this patch is landed, I can't make a patch for the wx port without breaking the build, can I? > > You can. Take a look at Qt, Mac, & Windows ports. They set design mode when editability is changed. I see them calling editor()->applyEditingStyleToBodyElement(), is this what you mean by setting design mode? And actually, after the EditorClient::clientIsEditable check is removed by this patch, I don't see how you can turn off editing on the page once you've set it, at least without resetting and reloading the page? > > You can. Take a look at Qt, Mac, & Windows ports. They set design mode when editability is changed.
>
> I see them calling editor()->applyEditingStyleToBodyElement(), is this what you mean by setting design mode? And actually, after the EditorClient::clientIsEditable check is removed by this patch, I don't see how you can turn off editing on the page once you've set it, at least without resetting and reloading the page?
I think calling editor()->applyEditingStyleToBodyElement() is something extra that QWebPage::setContentEditable does. QWebPage::setContentEditable also sets the editable flag and this flag is retrieved by EditorClient::clientIsEditable later on. So I think we should replace
d->editable = editable;
with
document->setDesignMode(editable);//something like that
Thus, we can remove the editable flag along with EditorClient::clientIsEditable.
I will work on this sometime next week.
(In reply to comment #18) > > > You can. Take a look at Qt, Mac, & Windows ports. They set design mode when editability is changed. > > > > I see them calling editor()->applyEditingStyleToBodyElement(), is this what you mean by setting design mode? And actually, after the EditorClient::clientIsEditable check is removed by this patch, I don't see how you can turn off editing on the page once you've set it, at least without resetting and reloading the page? > > I think calling editor()->applyEditingStyleToBodyElement() is something extra that QWebPage::setContentEditable does. QWebPage::setContentEditable also sets the editable flag and this flag is retrieved by EditorClient::clientIsEditable later on. So I think we should replace > d->editable = editable; > with > document->setDesignMode(editable);//something like that > Thus, we can remove the editable flag along with EditorClient::clientIsEditable. > > I will work on this sometime next week. Okay, thanks, I think I've got it now. :) Created attachment 83215 [details]
fix patch 4
Attachment 83215 [details] did not build on gtk: Build output: http://queues.webkit.org/results/7943108 Created attachment 83252 [details]
fix patch 5
fix gtk build
Comment on attachment 83252 [details] fix patch 5 View in context: https://bugs.webkit.org/attachment.cgi?id=83252&action=review > Source/WebKit/qt/Api/qwebpage.cpp:3167 > - if (d->editable != editable) { > - d->editable = editable; > + if (isContentEditable() != editable) { > d->page->setTabKeyCyclesThroughElements(!editable); > if (d->mainFrame) { > WebCore::Frame* frame = d->mainFrame->d->frame; > + frame->document()->setDesignMode(editable ? WebCore::Document::on : WebCore::Document::off); Chang, did you verify that this change doesn't break qt port? i.e. ran all layout tests successfully? > Source/WebKit/wx/WebFrame.cpp:414 > - m_isEditable = enable; > + if (enable != isEditable() && m_impl->frame && m_impl->frame->document()) > + m_impl->frame->document()->setDesignMode(enable ? WebCore::Document::on : WebCore::Document::off); > } > > - > +bool wxWebFrame::isEditable() > +{ > + if (m_impl->frame && m_impl->frame->document()) > + return m_impl->frame->document()->inDesignMode(); > + return false; > +} Kevin, could you verify that this change indeed doesn't break wx? Comment on attachment 83252 [details] fix patch 5 View in context: https://bugs.webkit.org/attachment.cgi?id=83252&action=review >> Source/WebKit/wx/WebFrame.cpp:414 >> +} > > Kevin, could you verify that this change indeed doesn't break wx? The fix itself is the right one, but there's a typo. wx coding style says to capitalize the first letter, so the method name should be IsEditable, not isEditable. Thanks for the comments. I'm trying to see if it builds on all platforms (don't r- yet :). There's a typo already and I'll also run the the tests before the next patch. I ran layout tests on Mac port and got one failure: --- /tmp/layout-test-results/editing/selection/designmode-no-caret-expected.txt 2011-02-21 19:22:09.000000000 -0800 +++ /tmp/layout-test-results/editing/selection/designmode-no-caret-actual.txt 2011-02-21 19:22:09.000000000 -0800 @@ -9,9 +9,11 @@ RenderBlock (anonymous) at (0,0) size 784x54 RenderText {#text} at (0,0) size 784x54 text run at (0,0) width 759: "This tests to see that a caret is placed inside an editable document that is entirely editable even when no caret is requested" + text run at (759,0) width 4: " " text run at (0,18) width 118: "programmatically. " text run at (118,18) width 187: "We do this as a convenience. " text run at (305,18) width 479: "Right now, we only do this convenience when a document's frame becomes" + text run at (784,18) width 0: " " text run at (0,36) width 378: "first responder or when a document's window becomes key." RenderBlock {PRE} at (0,67) size 784x15 RenderText {#text} at (0,0) size 88x15 Simon, do you know what is going wrong here? I do not, sorry. the above regression shows up after the latest change on mac/WebView/WebView.mm. (In reply to comment #28) > the above regression shows up after the latest change on mac/WebView/WebView.mm. What do you mean by that? (In reply to comment #29) > (In reply to comment #28) > > the above regression shows up after the latest change on mac/WebView/WebView.mm. > > What do you mean by that? I mean the regression was caused by the code change in the WebView.mm, which was done in patch#5. There was no regression up until patch#4. The difference in #5 is that the webview::iseditable always sync with designmode after the change but it's not the case before. I am looking into further details. Here are some findings for the regression: In function finishedLoadingWithDataSource in file mac/WebView/WebHTMLRepresentation.mm, if ([webView isEditable]) core(webFrame)->editor()->applyEditingStyleToBodyElement(); The if condition always returns false before my change because the local flag is false. In my patch, the local flag is removed and isEditable checks the designmode, which is true. It seems to me the new behavior is the right one and we should update the expected results. Agree? Disagree? (In reply to comment #31) > Here are some findings for the regression: > In function finishedLoadingWithDataSource in file mac/WebView/WebHTMLRepresentation.mm, > if ([webView isEditable]) > core(webFrame)->editor()->applyEditingStyleToBodyElement(); > > The if condition always returns false before my change because the local flag is false. In my patch, the local flag is removed and isEditable checks the designmode, which is true. > > It seems to me the new behavior is the right one and we should update the expected results. Agree? Disagree? But why does updating layout causes new text nodes to appear? These new spaces do exist in the code but I'm not sure if we should be emitting them in the render tree when they're at end of lines. Could someone familiar with the rendering engine comment on this point? > > It seems to me the new behavior is the right one and we should update the expected results. Agree? Disagree?
>
> But why does updating layout causes new text nodes to appear? These new spaces do exist in the code but I'm not sure if we should be emitting them in the render tree when they're at end of lines. Could someone familiar with the rendering engine comment on this point?
Function Editor::applyEditingStyleToElement sets some properties:
style->setProperty(CSSPropertyWordWrap, "break-word", false, ec);
style->setProperty(CSSPropertyWebkitNbspMode, "space", false, ec);
style->setProperty(CSSPropertyWebkitLineBreak, "after-white-space", false, ec);
Created attachment 83394 [details]
fix patch 6
Comment on attachment 83394 [details] fix patch 6 (In reply to comment #33) > > > It seems to me the new behavior is the right one and we should update the expected results. Agree? Disagree? > > > > But why does updating layout causes new text nodes to appear? These new spaces do exist in the code but I'm not sure if we should be emitting them in the render tree when they're at end of lines. Could someone familiar with the rendering engine comment on this point? > > Function Editor::applyEditingStyleToElement sets some properties: > style->setProperty(CSSPropertyWordWrap, "break-word", false, ec); > style->setProperty(CSSPropertyWebkitNbspMode, "space", false, ec); > style->setProperty(CSSPropertyWebkitLineBreak, "after-white-space", false, ec); Ok. The change looks same to me so let's hope that this won't cause any regressions. Thanks a lot for making this change! I believe this and your follow-up patch will greatly improve the performance. > Ok. The change looks same to me so let's hope that this won't cause any regressions. Thanks a lot for making this change! I believe this and your follow-up patch will greatly improve the performance.
I meant to say the change looks "sane".
Comment on attachment 83394 [details]
fix patch 6
thanks for the review!
Comment on attachment 83394 [details] fix patch 6 Rejecting attachment 83394 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sf', 'la..." exit_code: 2 Last 500 characters of output: ://git.webkit.org/WebKit d5109a9..a3991dd master -> origin/master Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ... Currently at 79833 = d5109a91a14034dcc4f948631a41b2efed4487e5 r79834 = a3991dd0f5c78b808f362dec70ef788cc6b7e5bb Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc First, rewinding head to replay your work on top of it... Fast-forwarded master to refs/remotes/origin/master. Full output: http://queues.webkit.org/results/8070229 Created attachment 84053 [details]
fix patch 7
cleaned up some junks in the ChangeLog
Comment on attachment 84053 [details] fix patch 7 Clearing flags on attachment: 84053 Committed r79953: <http://trac.webkit.org/changeset/79953> All reviewed patches have been landed. Closing bug. http://trac.webkit.org/changeset/79953 might have broken SnowLeopard Intel Release (Build) http://trac.webkit.org/changeset/79954 might have broken SnowLeopard Intel Release (Build) *** Bug 54050 has been marked as a duplicate of this bug. *** |