Bug 54292 - remove the support of Frame::isContentEditable and its dependencies
: remove the support of Frame::isContentEditable and its dependencies
Status: RESOLVED FIXED
: WebKit
HTML Editing
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
:
: 54290
  Show dependency treegraph
 
Reported: 2011-02-11 10:15 PST by
Modified: 2011-03-01 00:10 PST (History)


Attachments
fix patch (4.26 KB, patch)
2011-02-11 10:29 PST, Chang Shu
no flags Review Patch | Details | Formatted Diff | Diff
fix patch 2 (6.14 KB, patch)
2011-02-13 08:16 PST, Chang Shu
no flags Review Patch | Details | Formatted Diff | Diff
fix patch 3 (23.36 KB, patch)
2011-02-14 10:44 PST, Chang Shu
rniwa: review-
Review Patch | Details | Formatted Diff | Diff
fix patch 4 (33.83 KB, patch)
2011-02-21 14:31 PST, Chang Shu
no flags Review Patch | Details | Formatted Diff | Diff
fix patch 5 (33.87 KB, patch)
2011-02-21 18:22 PST, Chang Shu
no flags Review Patch | Details | Formatted Diff | Diff
fix patch 6 (35.04 KB, patch)
2011-02-22 14:44 PST, Chang Shu
rniwa: review+
commit-queue: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
fix patch 7 (35.04 KB, patch)
2011-02-28 07:03 PST, Chang Shu
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 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 From 2011-02-11 10:29:00 PST -------
Created an attachment (id=82142) [details]
fix patch
------- Comment #2 From 2011-02-11 14:18:49 PST -------
(From update of attachment 82142 [details])
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 From 2011-02-11 15:18:41 PST -------
Sure, will try.
------- Comment #4 From 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 From 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 From 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 From 2011-02-13 08:16:08 PST -------
Created an attachment (id=82262) [details]
fix patch 2
------- Comment #8 From 2011-02-13 14:47:22 PST -------
(In reply to comment #7)
> Created an attachment (id=82262) [details] [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 From 2011-02-14 10:44:58 PST -------
Created an attachment (id=82337) [details]
fix patch 3
------- Comment #10 From 2011-02-14 16:22:38 PST -------
(From update of attachment 82337 [details])
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 From 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 From 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 From 2011-02-16 23:59:32 PST -------
Kevin, any update on Wx side?
------- Comment #14 From 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 From 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 From 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 From 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 From 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 From 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 From 2011-02-21 14:31:47 PST -------
Created an attachment (id=83215) [details]
fix patch 4
------- Comment #21 From 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 From 2011-02-21 18:22:45 PST -------
Created an attachment (id=83252) [details]
fix patch 5

fix gtk build
------- Comment #23 From 2011-02-21 18:25:57 PST -------
(From update of attachment 83252 [details])
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 From 2011-02-21 18:41:46 PST -------
(From update of attachment 83252 [details])
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 From 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 From 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 From 2011-02-21 22:22:55 PST -------
I do not, sorry.
------- Comment #28 From 2011-02-22 06:28:26 PST -------
the above regression shows up after the latest change on mac/WebView/WebView.mm.
------- Comment #29 From 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 From 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 From 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 From 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 From 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 From 2011-02-22 14:44:42 PST -------
Created an attachment (id=83394) [details]
fix patch 6
------- Comment #35 From 2011-02-27 15:13:50 PST -------
(From update of attachment 83394 [details])
(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 From 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 From 2011-02-27 16:40:00 PST -------
(From update of attachment 83394 [details])
thanks for the review!
------- Comment #38 From 2011-02-27 18:07:03 PST -------
(From update of attachment 83394 [details])
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 From 2011-02-28 07:03:09 PST -------
Created an attachment (id=84053) [details]
fix patch 7

cleaned up some junks in the ChangeLog
------- Comment #40 From 2011-02-28 17:47:23 PST -------
(From update of attachment 84053 [details])
Clearing flags on attachment: 84053

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