Bug 56665 – REGRESSION (r79953): Can't type in MS Outlook 2011

• Component: HTML Editing
• Status: RESOLVED FIXED

[Alexey Proskuryakov]

Steps to reproduce:
1. Open a new mail message window.
2. Try to input text in the message body.

Results: you can't focus the message body.

Regression range is 79951-79959, this change is the only suspect.

<rdar://problem/9140548>

[Chang Shu]

http://trac.webkit.org/changeset/79953

[Ryosuke Niwa]

It seems like the only way this could happen was if Outlook was overriding EditorClient or WebView.  Or it calls setEditable before the main frame is created.

[Alexey Proskuryakov]

Outlook does subclass WebView, but I don't see any indication of it touching sensitive parts.

When setEditable is called, a WebCore document already exists, and gets its designMode property set successfully.

[Chang Shu]

Per discussion on IRC with ap and rniwa, the root cause of the problem seems to be that r79953 removed the WebView level editablity which is persistent no matter whether underlying document itself is changed and editability gets lost. The resolution is to set this WebView editable value to WebCore. This avoids the callback from WebCore to WebKit which was the main goal in r79953 to improve performance.
I will work on a patch soon.
Thanks very much, guys and sorry for the regression.

[Chang Shu]

Created attachment 86283 [details]
incomplete patch: need to fix WebCore.exp.in and test failed case

[Alexey Proskuryakov]

It might be best to land a fix first, and work on a regression test separately.

[Ryosuke Niwa]

Comment on attachment 86283 [details]
incomplete patch: need to fix WebCore.exp.in and test failed case

View in context: https://bugs.webkit.org/attachment.cgi?id=86283&action=review

> Source/WebCore/editing/SelectionController.cpp:1827
>      bool caretBrowsing = m_frame->settings() && m_frame->settings()->caretBrowsingEnabled();
> -    if (!isNone() || !(document->inDesignMode() || caretBrowsing))
> +    if (!isNone() || !(m_frame->isContentEditable() || caretBrowsing))

We can just call Document::isContentEditable here.

> Source/WebCore/page/Frame.cpp:558
> +bool Frame::isContentEditable() const
> +{
> +    return page()->isPageEditable() || m_doc->inDesignMode();
> +}
> +

I don't think there's a need to add isContentEditable to Frame.  We should just talk to the page inside Node::isContentEditable.

[Chang Shu]

> I don't think there's a need to add isContentEditable to Frame.  We should just talk to the page inside Node::isContentEditable.

Good point. Thanks, will do.

[Chang Shu]

(In reply to comment #6)
> It might be best to land a fix first, and work on a regression test separately.

I will provide a formal patch on Monday once I have access to my Mac. Thanks.

[Chang Shu]

Created attachment 86318 [details]
fix patch

Alexey, can you kindly check if this patch fixes the reported problem on MS Outlook 2011? Thanks!

[Alexey Proskuryakov]

Comment on attachment 86318 [details]
fix patch

View in context: https://bugs.webkit.org/attachment.cgi?id=86318&action=review

Thank you for a quick fix! I verified that it addresses the problem for me.

The fix looks good to me. I had a few comments.

> Source/WebCore/page/DragController.cpp:356
> +    if (doc && (m_didInitiateDrag || doc->isPluginDocument() || (doc->isContentEditable())))

Unneeded braces added here.

> Source/WebCore/page/Page.h:296
> +        void setPageEditable(bool isPageEditable) { m_isPageEditable = isPageEditable; }
> +        bool isPageEditable() { return m_isPageEditable; }

"Page" sounds like a tautology to me in names of methods defined on Page. Do you think that it helps avoid confusion?

> Source/WebCore/page/Page.h:403
> +        bool m_isPageEditable;

Even more tautological as a member variable name.

> Source/WebKit/mac/WebView/WebView.mm:5010
> +        _private->page->setPageEditable(flag);
>          if (!_private->tabKeyCyclesThroughElementsChanged && _private->page)

There is a check for null page below the line you added. This can't be right - either the check is unnecessary (and then the check in isEditable is also unnecessary), or this can crash.

> LayoutTests/ChangeLog:10
> +        Restore the expected result before r79953.
> +
> +        * platform/mac/editing/selection/designmode-no-caret-expected.txt:

I don't understand why this result changes back and forth. How can this patch affect anything in layout tests, given that we don't call -[WebView setEditable:]?

This needs to be explained in ChangeLog.

[Chang Shu]

> "Page" sounds like a tautology to me in names of methods defined on Page. Do you think that it helps avoid confusion?

Yeah, the reason I added "page" here is just for avoiding confusion. But I am not sure if it's necessary either. Any suggestions, like isEditable, isContentEditable?

> > Source/WebKit/mac/WebView/WebView.mm:5010
> > +        _private->page->setPageEditable(flag);
> >          if (!_private->tabKeyCyclesThroughElementsChanged && _private->page)
> 
> There is a check for null page below the line you added. This can't be right - either the check is unnecessary (and then the check in isEditable is also unnecessary), or this can crash.

Will fix.

> I don't understand why this result changes back and forth. How can this patch affect anything in layout tests, given that we don't call -[WebView setEditable:]?
> 
> This needs to be explained in ChangeLog.

My previous patch indeed changed behavior slightly. Here were my findings in bug 54292.
"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."

We are restoring the old behavior here.

[Ryosuke Niwa]

(In reply to comment #12)
> > "Page" sounds like a tautology to me in names of methods defined on Page. Do you think that it helps avoid confusion?
> 
> Yeah, the reason I added "page" here is just for avoiding confusion. But I am not sure if it's necessary either. Any suggestions, like isEditable, isContentEditable?

isEditable.

[Chang Shu]

Created attachment 86343 [details]
fix patch 2: addressed the reviews

[Alexey Proskuryakov]

Comment on attachment 86343 [details]
fix patch 2: addressed the reviews

View in context: https://bugs.webkit.org/attachment.cgi?id=86343&action=review

> Source/WebKit/mac/WebView/WebView.mm:5008
> +    if ([self isEditable] != flag && _private->page) {

This looks rather suspicious.  -[WebView setEditable:] should always work, so failing when there is no page would be a bug. Is that even a possible situation?

> LayoutTests/ChangeLog:11
> +        Restore the expected result before r79953. r79953 changed behavior slightly in function
> +        finishedLoadingWithDataSource in file mac/WebView/WebHTMLRepresentation.mm, where
> +        core(webFrame)->editor()->applyEditingStyleToBodyElement() was called but not before r79953
> +        or after this patch.

I'm wondering if that code is correct. Not something to fix in this bug, obviously.

[Chang Shu]

> > Source/WebKit/mac/WebView/WebView.mm:5008
> > +    if ([self isEditable] != flag && _private->page) {
> 
> This looks rather suspicious.  -[WebView setEditable:] should always work, so failing when there is no page would be a bug. Is that even a possible situation?

I am not sure if there is such a case. My code change is just a rewrite of the original code.

> 
> > LayoutTests/ChangeLog:11
> > +        Restore the expected result before r79953. r79953 changed behavior slightly in function
> > +        finishedLoadingWithDataSource in file mac/WebView/WebHTMLRepresentation.mm, where
> > +        core(webFrame)->editor()->applyEditingStyleToBodyElement() was called but not before r79953
> > +        or after this patch.
> 
> I'm wondering if that code is correct. Not something to fix in this bug, obviously.

Yes, it's likely that r79953 was actually the right behavior. If we see some more significant impact than this test case, we should open a bug.

[WebKit Commit Bot]

Comment on attachment 86343 [details]
fix patch 2: addressed the reviews

Clearing flags on attachment: 86343

Committed r81600: <http://trac.webkit.org/changeset/81600>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

> I am not sure if there is such a case. My code change is just a rewrite of the original code.

What I mean is that with original code, setEditable: was always successful, because it didn't need to check if page was null. It's not the case now.

Thanks again for fixing this so quickly!

[Chang Shu]

> What I mean is that with original code, setEditable: was always successful, because it didn't need to check if page was null. It's not the case now.

Actually, the original code checked page against null, too, implicitly. First in the first if statement, explicitly, then in [self _mainCoreFrame], if page is null, mainFrame will be null and nothing will execute.

[WebKit Review Bot]

http://trac.webkit.org/changeset/81600 might have broken GTK Linux 64-bit Debug
The following tests are not passing:
http/tests/inspector/network/network-size.html