Bug 54292 – remove the support of Frame::isContentEditable and its dependencies

• Component: HTML Editing
• Status: RESOLVED FIXED

[Chang Shu]

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.

[Chang Shu]

Created attachment 82142 [details]
fix patch

[Ryosuke Niwa]

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.

[Chang Shu]

Sure, will try.

[Chang Shu]

> 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?

[Ryosuke Niwa]

(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()

[Chang Shu]

> > 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!

[Chang Shu]

Created attachment 82262 [details]
fix patch 2

[Ryosuke Niwa]

(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.

[Chang Shu]

Created attachment 82337 [details]
fix patch 3

[Ryosuke Niwa]

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.

[Kevin Ollivier]

(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!

[Ryosuke Niwa]

(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.

[Ryosuke Niwa]

Kevin, any update on Wx side?

[Chang Shu]

I am also tied up on something else and will come back to this bug later.
--Chang

[Kevin Ollivier]

(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?

[Ryosuke Niwa]

(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.

[Kevin Ollivier]

(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?

[Chang Shu]

> > 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.

[Kevin Ollivier]

(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. :)

[Chang Shu]

Created attachment 83215 [details]
fix patch 4

[Collabora GTK+ EWS bot]

Attachment 83215 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7943108

[Chang Shu]

Created attachment 83252 [details]
fix patch 5

fix gtk build

[Ryosuke Niwa]

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?

[Kevin Ollivier]

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.

[Chang Shu]

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.

[Ryosuke Niwa]

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?

[Simon Fraser (smfr)]

I do not, sorry.

[Chang Shu]

the above regression shows up after the latest change on mac/WebView/WebView.mm.

[Ryosuke Niwa]

(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?

[Chang Shu]

(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.

[Chang Shu]

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?

[Ryosuke Niwa]

(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?

[Chang Shu]

> > 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);

[Chang Shu]

Created attachment 83394 [details]
fix patch 6

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

> 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".

[Chang Shu]

Comment on attachment 83394 [details]
fix patch 6

thanks for the review!

[WebKit Commit Bot]

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

[Chang Shu]

Created attachment 84053 [details]
fix patch 7

cleaned up some junks in the ChangeLog

[WebKit Commit Bot]

Comment on attachment 84053 [details]
fix patch 7

Clearing flags on attachment: 84053

Committed r79953: <http://trac.webkit.org/changeset/79953>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/79953 might have broken SnowLeopard Intel Release (Build)

[WebKit Review Bot]

http://trac.webkit.org/changeset/79954 might have broken SnowLeopard Intel Release (Build)

[Ryosuke Niwa]

*** Bug 54050 has been marked as a duplicate of this bug. ***