Closed Bug 128647 Opened 18 years ago Closed 11 years ago

[RFE] Handler for WM_COPY/WM_CUT/WM_PASTE/WM_CLEAR

Categories

(Core :: DOM: UI Events & Focus Handling, defect, P4, major)

x86
Windows XP
defect

Tracking

()

RESOLVED FIXED
mozilla1.9.2b1

People

(Reporter: kazhik, Assigned: masayuki)

References

Details

(Keywords: intl)

Attachments

(2 files, 6 obsolete files)

Mozilla doesn't have handler function for WM_COPY/WM_CUT
/WM_PASTE/WM_CLEAR. But standard windows application should
have them.
QA Contact: madhur → rakeshmishra
QA Contact: rakeshmishra → trix
We can do this real easy. Just handle the right messages and send the
appropriate NS_KEYPRESS. I got this working on OS/2.
.
Assignee: joki → saari
QA Contact: trix → ian
ATOK(Japanese IME) users want to fix this bug.

Because WM_COPY is used for adding new word to dictionary.
This bug is inconvenience for Japanese ATOK users.
VJE-Delta ver 4.0 that is IME for JP. It is using WM_COPY for its reconverting.
So, we need to implement this. Therefore I change Severity "enhancement" to
"minor". 
# The reason of "minor" is that VJE-Delta is minor IME in Japan.
Assignee: saari → masayuki
Severity: enhancement → minor
Priority: -- → P4
Target Milestone: --- → Future
Target Milestone: Future → mozilla1.9beta
bumping to major to match bug 301205
Severity: minor → major
Keywords: intl
Target Milestone: mozilla1.9alpha8 → ---
QA Contact: ian → events
Attached patch Patch v1.0 (obsolete) — Splinter Review
this patch works fine for me, but needs more some works.
Status: NEW → ASSIGNED
Attached patch Patch v2.0 (obsolete) — Splinter Review
Attachment #391091 - Attachment is obsolete: true
Attached patch Patch v3.0 (obsolete) — Splinter Review
This adds to support the clipboard/editing related messages such as WM_CUT, WM_COPY, WM_PASTE, WM_CLEAR, EM_UNDO, EM_REDO, EM_CANPASTE, EM_CANUNDO and EM_CANREDO.

I added nsContentCommandEvent for them and we will be able to use it in other bugs (I need to request the cursor move without key events in other bugs). So, I think this name is good for the previous name (nsEditEvent).

The events are handled in nsEventStateManager. The ESM gets the command controller for the requested action and checks whether the command is enabled or not. And also it executes the command if it's requested.

This patch also moves the event type checking in nsViewManager and nsPresShell to the inline functions in the nsGUIEvent.h. This helps to add some new events. And this guarantees the consistence of the conditions between them. Especially, they are using different logic on current trunk for checking whether the event is dispatched at the coordinates.
Attachment #391516 - Attachment is obsolete: true
Attachment #391535 - Flags: superreview?(roc)
Attachment #391535 - Flags: review?(emaijala)
Flags: wanted1.9.2?
Comment on attachment 391535 [details] [diff] [review]
Patch v3.0

oops, I didn't check the automated tests on tryserver. Pending the sr request but the Win32 part can be reviewed.
Attachment #391535 - Flags: superreview?(roc)
(In reply to comment #8)
> I added nsContentCommandEvent for them and we will be able to use it in other
> bugs (I need to request the cursor move without key events in other bugs). So,
> I think this name is good for the previous name (nsEditEvent).

I meant that the name is better than the previous name (nsEditEvent).
Comment on attachment 391535 [details] [diff] [review]
Patch v3.0

r+ for the Windows widget part.
Attachment #391535 - Flags: review?(emaijala) → review+
Attached patch Patch v3.0.1 (obsolete) — Splinter Review
Thank you, Ere.
Attachment #391535 - Attachment is obsolete: true
Attachment #392901 - Flags: superreview?(roc)
Attachment #392901 - Flags: review?(roc)
Is there any case outside IMEs where this is useful? I mean, should we implement this for OS/2, too?
(In reply to comment #13)
> Is there any case outside IMEs where this is useful? I mean, should we
> implement this for OS/2, too?

I'm not sure. But there are some possibilities: e.g., some utility software may access to the content of the focused application by these messages. So, this messages can improve the accessibility from other applications. The risk and the cost by this implementation are small, therefore, if you can implement this easily, I don't have any objections. But it shouldn't be major issue.
OK, I discovered that the relevant WM_ messages are only available in the Win32 compatibility layer of OS/2. I very much doubt that any applications exist (which are still useful today) that make use of them. So I won't do an OS/2 implementation after all.
+    aEvent->mIsEnabled = canDoIt ? 1 : PR_FALSE;

Why not just aEvent->mIsEnabled = canDoIt?

+  aEvent->mSucceeded = 1;

PR_TRUE?

+inline PRBool NS_IsUsingCoordinatesEvent(nsEvent* aEvent)

NS_IsEventUsingCoordinates would probably be better

+inline PRBool NS_IsFocusedShellTargettedEvent(nsEvent* aEvent)

NS_IsEventTargetedAtFocusedWindow would probably be better

I wonder if we should just use nsCommandEvent --- and extend it with the "enabled" flags --- instead of creating a new event type?
(In reply to comment #16)
> +    aEvent->mIsEnabled = canDoIt ? 1 : PR_FALSE;
> 
> Why not just aEvent->mIsEnabled = canDoIt?

Can I use PRBool value to PRPacked variable? Yes, I know we can use it actually. But I'm not sure whether we should do it or not so.

> I wonder if we should just use nsCommandEvent --- and extend it with the
> "enabled" flags --- instead of creating a new event type?

nsCommandEvent is handled in browser.js, but the new events cannot be handled by it. They should be handled by ESM or something. And they don't need to fire their DOM events. So, it seems that nsCommandEvent using creates a new different handling path of it. Don't other developers confuse by it?

I think the name of nsCommandEvent is wrong for its current meaning. It should be nsNavigationCommandEvent or something. And the new event should be nsCommandEvent. But we should not rename them....
(In reply to comment #17)
> (In reply to comment #16)
> > +    aEvent->mIsEnabled = canDoIt ? 1 : PR_FALSE;
> > 
> > Why not just aEvent->mIsEnabled = canDoIt?
> 
> Can I use PRBool value to PRPacked variable? Yes, I know we can use it
> actually. But I'm not sure whether we should do it or not so.

You should. The only legal values in PRBool are 1 or 0. So your code here doesn't make any difference.

> I think the name of nsCommandEvent is wrong for its current meaning. It should
> be nsNavigationCommandEvent or something. And the new event should be
> nsCommandEvent. But we should not rename them....

OK, I'm convinced :-)
Attached patch Patch v3.1 (obsolete) — Splinter Review
Attachment #392901 - Attachment is obsolete: true
Attachment #393130 - Flags: superreview?(roc)
Attachment #393130 - Flags: review?(roc)
Attachment #392901 - Flags: superreview?(roc)
Attachment #392901 - Flags: review?(roc)
Comment on attachment 393130 [details] [diff] [review]
Patch v3.1

+    aEvent->mIsEnabled = PRPackedBool(canDoIt);

Don't need the cast.
Attachment #393130 - Flags: superreview?(roc)
Attachment #393130 - Flags: superreview+
Attachment #393130 - Flags: review?(roc)
Attachment #393130 - Flags: review+
Attached patch Patch v3.2Splinter Review
Attachment #393130 - Attachment is obsolete: true
Attachment #393138 - Flags: superreview+
Attachment #393138 - Flags: review+
http://hg.mozilla.org/mozilla-central/rev/9c37b8a909a2
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2b1
This patch breaks cross compilation on Linux mingw by using mixed case in includes. The attached patch fixes it (it also fixes oleidl.h includes that I didn't notice earlier because it's ACCESSIBILITY ifdefed).
Attachment #393358 - Flags: review?(masayuki)
Comment on attachment 393358 [details] [diff] [review]
Use lower case in includes to fix compilation on mingw.

looks ok, but you need to change widget/tests/TestWinTSF.cpp too.
You're right, thanks. Attached patch includes test fix.
Attachment #393358 - Attachment is obsolete: true
Attachment #393362 - Flags: review?(masayuki)
Attachment #393358 - Flags: review?(masayuki)
Comment on attachment 393362 [details] [diff] [review]
 Use lower case in includes to fix compilation on mingw. 

Thank you for your work!
Attachment #393362 - Flags: review?(masayuki) → review+
Attachment #393362 - Flags: superreview?(roc)
http://www.mozilla.org/hacking/reviewers.html

Oh, we don't need sr+ for such simple fix now?
Comment on attachment 393362 [details] [diff] [review]
 Use lower case in includes to fix compilation on mingw. 

no, this doesn't need sr
Attachment #393362 - Flags: superreview?(roc) → superreview+
Keywords: checkin-needed
Whiteboard: "checkin-needed" is attachment 393362
http://hg.mozilla.org/mozilla-central/rev/bc6ffc794fc1
Keywords: checkin-needed
Whiteboard: "checkin-needed" is attachment 393362
Flags: wanted1.9.2?
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.