Bug 45071 – Only intercept ESC key press when autocorrection UI is visible.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jia Pu]

This is related to autocorrection UI implemented in bug 44958, and <rdar://problem/8378285>.

Currently we dismiss autocorrection panel and return true whenever executeCancelOperation() is called. However, the regular functionality of ESC key is to stop loading. We should return false in executeCancelOperation() when correction panel isn't visible. By doing so, we allow the ESC key event to be handled by the upstream of responder chain.

[Jia Pu]

Created attachment 66555 [details]
Proposed patch (v1)

Provided supportedDismissCorrectionUI() as the isSupported() function for the executeCancelOperation() command. This function returns true only when the autocorrection UI is shown.

[Jia Pu]

<rdar://problem/8378285>

Comment on attachment 66555 [details]
Proposed patch (v1)

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

> WebCore/editing/Editor.cpp:2822
> +bool Editor::isCorrectionUIShown()
Is there a reason for calling it “correction UI” here when everywhere else it’s “correction panel”? I would name this (and the client method) isShowingCorrectionPanel(), so that editor->isShowingCorrectionPanel() reads like a sentence with editor being the subject.

> WebCore/editing/EditorCommand.cpp:1125
> +static bool supportedDismissCorrectionUI(Frame* frame, EditorCommandSource)
> +{
> +    return frame->editor()->isCorrectionUIShown();
> +}
Perhaps in addition to checking this we should still limit to menu and key binding? I don’t think we want to expose this through the execCommand DOM interface, for example.

[Jia Pu]

(In reply to comment #3)
> (From update of attachment 66555 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=66555&action=prettypatch
> 
> > WebCore/editing/Editor.cpp:2822
> > +bool Editor::isCorrectionUIShown()
> Is there a reason for calling it “correction UI” here when everywhere else it’s “correction panel”? I would name this (and the client method) isShowingCorrectionPanel(), so that editor->isShowingCorrectionPanel() reads like a sentence with editor being the subject.
> 

Actually I wonder if we should change all occurrence of "correction panel" to "correction UI", since the term "panel" really originated from AppKit. It may not be considered appropriate on other platforms.

> > WebCore/editing/EditorCommand.cpp:1125
> > +static bool supportedDismissCorrectionUI(Frame* frame, EditorCommandSource)
> > +{
> > +    return frame->editor()->isCorrectionUIShown();
> > +}
> Perhaps in addition to checking this we should still limit to menu and key binding? I don’t think we want to expose this through the execCommand DOM interface, for example.

Ok, I will change this.

[Jia Pu]

Created attachment 67441 [details]
proposed patch (v2)

updated according to comment #3.

Comment on attachment 67441 [details]
proposed patch (v2)

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

> WebCore/editing/EditorCommand.cpp:1130
> +static bool supportedDismissCorrectionPanel(Frame* frame, EditorCommandSource source)
> +{
> +    if (supportedFromMenuOrKeyBinding(frame, source))
> +        return frame->editor()->isShowingCorrectionPanel();
> +    return false;
> +}
Personally, I would write this as
    return supportedFromMenuOrKeyBinding(frame, source) && frame->editor()->isShowingCorrectionPanel();

> WebKit/mac/WebCoreSupport/WebEditorClient.mm:842
> +bool WebEditorClient::isShowingCorrectionPanel() {
> +    return m_correctionPanelTag != InvalidCorrectionPanelTag;
> +}
The opening brace should go on its own line.

r- so you can fix that style issue and consider the other comment.

[Jia Pu]

Created attachment 67445 [details]
Proposed patch (v3)

updated according to comment #6.

Comment on attachment 67445 [details]
Proposed patch (v3)

I didn’t notice the missing reviewer line. This means that the commit-queue script can’t process this patch.

Committed revision 67451.

[WebKit Review Bot]

http://trac.webkit.org/changeset/67451 might have broken Qt Linux Release minimal
The following changes are on the blame list:
http://trac.webkit.org/changeset/67449
http://trac.webkit.org/changeset/67450
http://trac.webkit.org/changeset/67451

Build fix in r67452.

[Eric Seidel (no email)]

This appears to ahve broken leopard tests :(

[Eric Seidel (no email)]

Committed r67458: <http://trac.webkit.org/changeset/67458>

[Eric Seidel (no email)]

Rolled out r67451 and r67451 as this change broke a whole bunch of builders and is blocking the commit-queue.

[Eric Seidel (no email)]

As far as I can tell from the buildbots this was the right patch, but it's not immediately obvious from this change or the test why it was failing.  The failure was clearly consistant and across multiple platforms.  Hopefully it will be easy to spot for you two.

My sincerest apologies if my rollout was in error and I'm happy to re-land.  But the buildbots seem pretty certain that this change was at fault.

[Eric Seidel (no email)]

The diff:

--- /Volumes/Data/WebKit-BuildSlave/snowleopard-intel-release-tests/build/layout-test-results/fast/events/event-input-contentEditable-expected.txt	2010-09-14 03:07:36.000000000 -0700
+++ /Volumes/Data/WebKit-BuildSlave/snowleopard-intel-release-tests/build/layout-test-results/fast/events/event-input-contentEditable-actual.txt	2010-09-14 03:07:36.000000000 -0700
@@ -12,7 +12,7 @@
 PASS event.target.id is 'target4'
 PASS event.target.innerHTML is '<a href="http://www.example.com/">This text should be a link.</a>'
 PASS event.target.id is 'target6parent'
-PASS event.target.innerHTML is '<span id="target6start"><a href="http://www.example.com/">Start,</a></span><span id="target6middle"><a href="http://www.example.com/">Middle,</a></span><span id="target6end"><a href="http://www.example.com/">End.</a></span>'
+FAIL event.target.innerHTML should be <span id="target6start"><a href="http://www.example.com/">Start,</a></span><span id="target6middle"><a href="http://www.example.com/">Middle,</a></span><span id="target6end"><a href="http://www.example.com/">End.</a></span>. Was <span id="target6start"><a href="http://www.example.com/">Start,</a></span><a href="http://www.example.com/"><span id="target6middle">Middle,</span><span id="target6end">End.</span></a>.
 PASS event.target.id is 'target7'
 PASS event.target.innerHTML is 'X'
 PASS event.target.id is 'target8'

[Csaba Osztrogonác]

(In reply to comment #14)
> Rolled out r67451 and r67451 as this change broke a whole bunch of builders and is blocking the commit-queue.

s/r67451 and r67451/r67451 and r67452 ;)

Thx for rolling out, these changesets broke fast/events/event-input-contentEditable.html on Qt and on GTK:

http://build.webkit.org/results/Qt%20Linux%20Release/r67453%20%2819915%29/fast/events/event-input-contentEditable-pretty-diff.html

http://build.webkit.org/results/GTK%20Linux%2032-bit%20Release/r67453

[WebKit Review Bot]

http://trac.webkit.org/changeset/67458 might have broken GTK Linux 32-bit Debug

[Jia Pu]

Created attachment 67607 [details]
Proposed patch (v4)

Fixed build warning/error on non-mac platforms.

[Eric Seidel (no email)]

bug 45616 is also claiming responsibility for the test break.  It's possible I simply read the buildbot wrong last night at 4 am...

Comment on attachment 67607 [details]
Proposed patch (v4)

r- because this will fail the commit-queue again. Please leave in (or in this case, add back) the line that says Reviewed by NOBODY (OOPS!). Thanks!

[Jia Pu]

Created attachment 67623 [details]
Proposed patch (v5)

Added back "Reviewed by NOBODY (OOPS!)." line, so that build doesn't fail.

[Jia Pu]

Comment on attachment 67623 [details]
Proposed patch (v5)

I manually added "Reviewed by NOBODY (OOPS)." line without changing the line range. So the patch is malformed. I will submit another patch.

[Jia Pu]

Created attachment 67637 [details]
Proposed patch (v6)

Updated all three ChangeLog files with "Reviewed by NOBODY (OOPS!)." line.

[WebKit Commit Bot]

Comment on attachment 67637 [details]
Proposed patch (v6)

Clearing flags on attachment: 67637

Committed r67534: <http://trac.webkit.org/changeset/67534>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/67539 might have broken GTK Linux 32-bit Debug