Bug 45071 - Only intercept ESC key press when autocorrection UI is visible.
Summary: Only intercept ESC key press when autocorrection UI is visible.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.6
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on: 44958
Blocks:
  Show dependency treegraph
 
Reported: 2010-09-01 16:58 PDT by Jia Pu
Modified: 2010-09-15 01:35 PDT (History)
7 users (show)

See Also:


Attachments
Proposed patch (v1) (10.66 KB, patch)
2010-09-03 15:07 PDT, Jia Pu
no flags Details | Formatted Diff | Diff
proposed patch (v2) (10.77 KB, patch)
2010-09-13 11:32 PDT, Jia Pu
mitz: review-
Details | Formatted Diff | Diff
Proposed patch (v3) (10.74 KB, patch)
2010-09-13 11:51 PDT, Jia Pu
mitz: review+
Details | Formatted Diff | Diff
Proposed patch (v4) (11.68 KB, patch)
2010-09-14 14:39 PDT, Jia Pu
mitz: review-
Details | Formatted Diff | Diff
Proposed patch (v5) (11.72 KB, patch)
2010-09-14 17:39 PDT, Jia Pu
mitz: review+
mitz: commit-queue+
Details | Formatted Diff | Diff
Proposed patch (v6) (11.80 KB, patch)
2010-09-14 19:59 PDT, Jia Pu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jia Pu 2010-09-01 16:58:35 PDT
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.
Comment 1 Jia Pu 2010-09-03 15:07:59 PDT
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.
Comment 2 Jia Pu 2010-09-03 15:47:24 PDT
<rdar://problem/8378285>
Comment 3 mitz 2010-09-09 10:09:09 PDT
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.
Comment 4 Jia Pu 2010-09-09 19:19:04 PDT
(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.
Comment 5 Jia Pu 2010-09-13 11:32:38 PDT
Created attachment 67441 [details]
proposed patch (v2)

updated according to comment #3.
Comment 6 mitz 2010-09-13 11:38:05 PDT
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.
Comment 7 Jia Pu 2010-09-13 11:51:51 PDT
Created attachment 67445 [details]
Proposed patch (v3)

updated according to comment #6.
Comment 8 mitz 2010-09-14 00:46:16 PDT
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.
Comment 9 mitz 2010-09-14 00:56:02 PDT
Committed revision 67451.
Comment 10 WebKit Review Bot 2010-09-14 01:01:58 PDT
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
Comment 11 mitz 2010-09-14 01:03:36 PDT
Build fix in r67452.
Comment 12 Eric Seidel (no email) 2010-09-14 04:02:06 PDT
This appears to ahve broken leopard tests :(
Comment 13 Eric Seidel (no email) 2010-09-14 04:07:42 PDT
Committed r67458: <http://trac.webkit.org/changeset/67458>
Comment 14 Eric Seidel (no email) 2010-09-14 04:08:21 PDT
Rolled out r67451 and r67451 as this change broke a whole bunch of builders and is blocking the commit-queue.
Comment 15 Eric Seidel (no email) 2010-09-14 04:16:05 PDT
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.
Comment 16 Eric Seidel (no email) 2010-09-14 04:16:16 PDT
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'
Comment 17 Csaba Osztrogonác 2010-09-14 05:09:57 PDT
(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
Comment 18 WebKit Review Bot 2010-09-14 06:35:19 PDT
http://trac.webkit.org/changeset/67458 might have broken GTK Linux 32-bit Debug
Comment 19 Jia Pu 2010-09-14 14:39:14 PDT
Created attachment 67607 [details]
Proposed patch (v4)

Fixed build warning/error on non-mac platforms.
Comment 20 Eric Seidel (no email) 2010-09-14 14:41:16 PDT
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 21 mitz 2010-09-14 17:13:14 PDT
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!
Comment 22 Jia Pu 2010-09-14 17:39:18 PDT
Created attachment 67623 [details]
Proposed patch (v5)

Added back "Reviewed by NOBODY (OOPS!)." line, so that build doesn't fail.
Comment 23 Jia Pu 2010-09-14 19:39:14 PDT
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.
Comment 24 Jia Pu 2010-09-14 19:59:15 PDT
Created attachment 67637 [details]
Proposed patch (v6)

Updated all three ChangeLog files with "Reviewed by NOBODY (OOPS!)." line.
Comment 25 WebKit Commit Bot 2010-09-14 21:39:22 PDT
Comment on attachment 67637 [details]
Proposed patch (v6)

Clearing flags on attachment: 67637

Committed r67534: <http://trac.webkit.org/changeset/67534>
Comment 26 WebKit Commit Bot 2010-09-14 21:39:28 PDT
All reviewed patches have been landed.  Closing bug.
Comment 27 WebKit Review Bot 2010-09-15 01:35:00 PDT
http://trac.webkit.org/changeset/67539 might have broken GTK Linux 32-bit Debug