Bug 54292

Summary: remove the support of Frame::isContentEditable and its dependencies
Product: WebKit Reporter: Chang Shu <cshu>
Component: HTML EditingAssignee: 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 Flags
fix patch
none
fix patch 2
none
fix patch 3
rniwa: review-
fix patch 4
none
fix patch 5
none
fix patch 6
rniwa: review+, commit-queue: commit-queue-
fix patch 7 none

Description Chang Shu 2011-02-11 10:15:41 PST
Frame::isContentEditable is based on two things: Editor::clientIsEditable and Document::inDesignMode. In fact, it should only rely on Document::inDesignMode. We should remove this function and use Document::inDesignMode directly.
Comment 1 Chang Shu 2011-02-11 10:29:00 PST
Created attachment 82142 [details]
fix patch
Comment 2 Ryosuke Niwa 2011-02-11 14:18:49 PST
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.
Comment 3 Chang Shu 2011-02-11 15:18:41 PST
Sure, will try.
Comment 4 Chang Shu 2011-02-12 13:24:58 PST
> 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?
Comment 5 Ryosuke Niwa 2011-02-12 17:52:47 PST
(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()
Comment 6 Chang Shu 2011-02-12 18:51:59 PST
> > 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!
Comment 7 Chang Shu 2011-02-13 08:16:08 PST
Created attachment 82262 [details]
fix patch 2
Comment 8 Ryosuke Niwa 2011-02-13 14:47:22 PST
(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.
Comment 9 Chang Shu 2011-02-14 10:44:58 PST
Created attachment 82337 [details]
fix patch 3
Comment 10 Ryosuke Niwa 2011-02-14 16:22:38 PST
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.
Comment 11 Kevin Ollivier 2011-02-14 21:48:03 PST
(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!
Comment 12 Ryosuke Niwa 2011-02-14 21:55:57 PST
(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.
Comment 13 Ryosuke Niwa 2011-02-16 23:59:32 PST
Kevin, any update on Wx side?
Comment 14 Chang Shu 2011-02-17 06:32:29 PST
I am also tied up on something else and will come back to this bug later.
--Chang
Comment 15 Kevin Ollivier 2011-02-17 07:41:39 PST
(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?
Comment 16 Ryosuke Niwa 2011-02-17 07:54:50 PST
(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.
Comment 17 Kevin Ollivier 2011-02-17 08:33:02 PST
(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?
Comment 18 Chang Shu 2011-02-17 08:49:55 PST
> > 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.
Comment 19 Kevin Ollivier 2011-02-17 22:00:57 PST
(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. :)
Comment 20 Chang Shu 2011-02-21 14:31:47 PST
Created attachment 83215 [details]
fix patch 4
Comment 21 Collabora GTK+ EWS bot 2011-02-21 16:05:28 PST
Attachment 83215 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7943108
Comment 22 Chang Shu 2011-02-21 18:22:45 PST
Created attachment 83252 [details]
fix patch 5

fix gtk build
Comment 23 Ryosuke Niwa 2011-02-21 18:25:57 PST
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 24 Kevin Ollivier 2011-02-21 18:41:46 PST
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.
Comment 25 Chang Shu 2011-02-21 18:56:58 PST
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.
Comment 26 Ryosuke Niwa 2011-02-21 22:13:13 PST
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?
Comment 27 Simon Fraser (smfr) 2011-02-21 22:22:55 PST
I do not, sorry.
Comment 28 Chang Shu 2011-02-22 06:28:26 PST
the above regression shows up after the latest change on mac/WebView/WebView.mm.
Comment 29 Ryosuke Niwa 2011-02-22 06:43:50 PST
(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?
Comment 30 Chang Shu 2011-02-22 07:09:42 PST
(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.
Comment 31 Chang Shu 2011-02-22 08:02:22 PST
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?
Comment 32 Ryosuke Niwa 2011-02-22 08:31:34 PST
(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?
Comment 33 Chang Shu 2011-02-22 10:02:20 PST
> > 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);
Comment 34 Chang Shu 2011-02-22 14:44:42 PST
Created attachment 83394 [details]
fix patch 6
Comment 35 Ryosuke Niwa 2011-02-27 15:13:50 PST
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.
Comment 36 Ryosuke Niwa 2011-02-27 15:14:23 PST
> 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 37 Chang Shu 2011-02-27 16:40:00 PST
Comment on attachment 83394 [details]
fix patch 6

thanks for the review!
Comment 38 WebKit Commit Bot 2011-02-27 18:07:03 PST
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
Comment 39 Chang Shu 2011-02-28 07:03:09 PST
Created attachment 84053 [details]
fix patch 7

cleaned up some junks in the ChangeLog
Comment 40 WebKit Commit Bot 2011-02-28 17:47:23 PST
Comment on attachment 84053 [details]
fix patch 7

Clearing flags on attachment: 84053

Committed r79953: <http://trac.webkit.org/changeset/79953>
Comment 41 WebKit Commit Bot 2011-02-28 17:47:32 PST
All reviewed patches have been landed.  Closing bug.
Comment 42 WebKit Review Bot 2011-02-28 18:03:54 PST
http://trac.webkit.org/changeset/79953 might have broken SnowLeopard Intel Release (Build)
Comment 43 WebKit Review Bot 2011-02-28 18:04:01 PST
http://trac.webkit.org/changeset/79954 might have broken SnowLeopard Intel Release (Build)
Comment 44 Ryosuke Niwa 2011-03-01 00:10:00 PST
*** Bug 54050 has been marked as a duplicate of this bug. ***