501154 - Improve clipboard event code

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Neil Deakin]

Attachment: work in progress

This is some preliminary work to help with implementing bug 407983.

- consolidates the two places where cut/copy/paste events are fired from
- adds back most of the tests from bug 280959
- removes some needless indirection to the document viewer and presshell.

Comment 1

[Neil Deakin]

Attachment: version 2

Bit more cleanup and fixes a memory access error that caused random crashes. Also now passes tests on the tryserver.

Comment 2

[Neil Deakin]

Attachment: consolidate code where clipboard events are fired

Comment 3

[Neil Deakin]

Comment on attachment 428276 [details] [diff] [review]
consolidate code where clipboard events are fired

This isn't the right patch

Comment 4

[Neil Deakin]

Attachment: better patch

Comment 5

[[e-mail]]

Comment on attachment 429157 [details] [diff] [review]
better patch

I'm not keen on the name DoClipboardAction that only ever does the Copy part of the action, if applicable.

How are the cut and paste global window commands supposed to work?

Why is FirePasteEvent subtly different to DoCutOrCopy(NS_CUT)?

Comment 6

[Neil Deakin]

(In reply to comment #5)
> (From update of attachment 429157 [details] [diff] [review])
> I'm not keen on the name DoClipboardAction that only ever does the Copy part of
> the action, if applicable.
> 
> How are the cut and paste global window commands supposed to work?

They currently always return false for IsCommandEnabled, so effectively do nothing. Even if DoCommand is called directly (as key handlers do), it only fires the event, and has no other behaviour, which I think is a bug. Cut and paste only do anything in editors.

Comment 7

[[e-mail]]

(In reply to comment #6)
> (In reply to comment #5)
> > How are the cut and paste global window commands supposed to work?
> They currently always return false for IsCommandEnabled
That's not true, they're used for designMode/contentEditable.

Comment 8

[Neil Deakin]

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > How are the cut and paste global window commands supposed to work?
> > They currently always return false for IsCommandEnabled
> That's not true, they're used for designMode/contentEditable.

Editable areas use the editor controller (nsEditorCommands.cpp) which does respond with true as necessary.

The controller commands in GlobalWindowCommands.cpp only respond if an editor hasn't already done so, in which case, it is non-editable, and thus cut and paste aren't available.

The implementation of the latter is at http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDocumentViewer.cpp#2604 which just unconditionally returns false.

Note that I've tested that clipboard operations on designMode and contentEditable areas still work with this patch.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

I'm not quite sure now whether I should review this, since the patch got
sr-. Neil (either one), should the patch be reviewed?

Comment 10

[[e-mail]]

(In reply to comment #9)
> I'm not quite sure now whether I should review this, since the patch got
> sr-. Neil (either one), should the patch be reviewed?
Yes, although I'm still waiting for the answer to my last question.

Comment 11

[Neil Deakin]

I've combined FirePasteEvent and DoCutOrCopy into one method.

Comment 12

[Neil Deakin]

Attachment: version 5

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 431927 [details] [diff] [review]
version 5

>+
>+    /**
>+     * Retrieve the selection for the given presShell and document. If the
>+     * current focus within the document has its own selection, aSelection
>+     * will be set to it and this focused content node returned. Otherwise,
>+     * aSelection will be set to the document's selection and null will be
>+     * returned.
>+     */
>+    static nsIContent* GetSelectionForCopy(nsIPresShell* aPresShell,
>+                                           nsIDocument* aDocument,
>+                                           nsISelection** aSelection);
Since a document can have only one presshell, perhaps there isn't need
for the aPresShell parameter.

>+
>+  // It seems to be unsafe to fire an event handler during reflow (bug 393696)
>+  PRBool isReflowing = PR_TRUE;
>+  nsresult rv = presShell->IsReflowLocked(&isReflowing);
>+  if (NS_FAILED(rv) || isReflowing)
>+    return PR_FALSE;

Don't check IsReflowLocked, but nsContentUtils::IsSafeToRunScript()
And could you perhaps add warning if it isn't safe to run script.

Comment 14

[Neil Deakin]

http://hg.mozilla.org/mozilla-central/rev/27259a0fcbe6

Comment 15

[chris hofmann]

I just ran across a crash that looks near where this code change happend.  regression?

http://crash-stats.mozilla.com/report/index/bp-dbda259d-64e6-4861-9e91-87d8c2100322

There was a lot going on when I hit the crash so I'm not sure how to reproduce.  I could have been loading this page http://www.networkworld.com/news/2010/031910-mozilla-confirms-critical-firefox.html

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

There is indeed a null check missing .