User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.3b) Gecko/20030217 Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.3b) Gecko/20030217 Double clicking in the message compose window on text doesn't select the relevant word. *However* double clicking on a word in the message view window does correctly select words as expected. The last mozilla build (13th Feb) works fine, so something between there and today's build has broken word selection. Reproducible: Always Steps to Reproduce: 1. Compose an email 2. type text or copy/paste text into the body of the message 3. Try to select a word by double clicking in between a word Actual Results: Nothing. No word selected as expected behavior. Expected Results: Should select word as it does in the message view window and all other User Interfaces.

Confirmed regression, build 20030211 was OK, build 20030217 is not OK. This happens in composer, changing product and component

-->selection

cc those who reviewed/approved/wrote fix for bug 185889 / bug 103055 / bug 110072 which may have caused this regression

Definitely me, and definitely directly related to whether or not we target mouse events at text. If I back out the part of the patch that retargets the events (leaving everything else the same) this regression goes away.

*** Bug 193776 has been marked as a duplicate of this bug. ***

chris found something that i think is related on OS X: when you click-hold to bring up the context menu (mail compose, composer), the context menu appears at 0,0 (in the content area), rather than near the pointer. this regressed after 2/14. let us know if it's not related, though.

what should we do for 1.3 on this one?

backout the checkin that caused this regression?

My memory (which may be wrong) was that the checkin causing this was a fix for another regression. How do we get back to a known good state, and what would that make us lose?

I believe it will be safe to back out the retargeting part of the fix without regressing the other regressions--it would deliberately make bug 103055 and bug 185889 reopen, but that's better than having this. If I don't have good solutions for these regressions by tonight I will post a patch.

I have posted the backout patch I am using (it's not tested on every regression yet, I'm still investigating these bugs) in bug 185889. If drivers wants that patch today, I'll push it through and regress the aforementioned bugs.

All right, the problem is that composer wants to do things with elements on double click but not on text, so it calls preventDefault. So the fix is similar to bug 193568. I am currently trying to consolidate them and set them in originalTarget, but I have discovered a bug in nsXULElement::HandleDOMEvent() where it is setting originalTarget itself. See the difference between these lines: http://lxr.mozilla.org/seamonkey/source/content/base/src/nsGenericElement.cpp#1837 http://lxr.mozilla.org/seamonkey/source/content/xul/content/src/nsXULElement.cpp#3289 Joe Hewitt added this line in bug 104401 and forgot to remove it before checking in: http://bugzilla.mozilla.org/show_bug.cgi?id=104401#c10 . I will have to do so to fix this bug properly using originalTarget. At least it's clear cut.

originalTarget turns out to work, but it is a bad idea generally because places like this (nsHTMLEditorMouseListener) need to *not* get XBL anonymous content children, which they will with originalTarget. This is a general problem; we'll need a new field, probably in nsIDOMNSEvent.

EDITORBASE+

This patch fixes this problem and depends on the patch in bug 193568. There is another problem in that double click on text in a table still brings up the table properties dialog, but I'm not yet sure if it's directly related--it's a problem with the current code as well.

Comment on attachment 115194 [details] [diff] [review] Patch This improves the double click situation but doesn't solve the double click in table problem--still worth checking in soon.

Comment on attachment 115194 [details] [diff] [review] Patch sr=jst

Two other problems with editor we may as well resolve: - double clicking in a table brings up table properties dialog instead of selecting text - clicking on text when Show All Tags is operational selects all the text instead of setting the cursor

Comment on attachment 115211 [details] [diff] [review] Patch For Other Problems sr=jst

Comment on attachment 115211 [details] [diff] [review] Patch For Other Problems Are you sure that behavior for tables isn't intentional?

Comment on attachment 115211 [details] [diff] [review] Patch For Other Problems Yes, I checked against an older build.

Comment on attachment 115194 [details] [diff] [review] Patch a=asa (on behalf of drivers) for checkin to 1.3

Oops, I uploaded the wrong file before.

Comment on attachment 115226 [details] [diff] [review] Patch For Other Problems, Really sr=jst

The first patch was checked in last night and the original reason for this bug is gone. Leaving this bug open for the second patch.

The same goes for this as for bug 193568, the checkin was after the branch so it needs to be checked in on the branch too.

Comment on attachment 115226 [details] [diff] [review] Patch For Other Problems, Really Please request reviews from a module owner when changing editor files. Any of these people should be readily available: Kathy Brade, Simon Fraser, Kin Blas, Daniel Glazman, Joe Francis. Others may also be available (Akkana Peck, Charley Manske, Neil, etc). Please wrap this line so the length < 80 columns: + if (IsWebComposer() && event.explicitOriginalTarget && IsHTMLEditor() && gEditorDisplayMode == kDisplayModeAllTags) Fix the inconsistent whitespace (not introduced by you) on this line--remove space after '(': + var element = event.explicitOriginalTarget.QueryInterface( Components.interfaces.nsIDOMElement); Has this patch been tested for these behaviors: click to place caret double-click to select word double-click on link to open link properties double-click on an image to open image properties double-click hrule to open properties double-click image inside table to open image props double-click table to open table props double-click bullet/number of list to open props ... In particular I'm concerned about some of the combinations (lists inside tables). Also, I'm wondering if we have problems with contextual menus (they might also be based on event.target as opposed to event.explictOriginalTarget).

This patch fixes the whitespace nits. 1. click to place caret Works. 2. double-click to select word Works. 3. double-click on link to open link properties Works. Note that it also selects the word (and it did this before). Intentional? 4. double-click on an image to open image properties Works. Also selects image, see #3. 5. double-click hrule to open properties Works. 6. double-click image inside table to open image props Works. 7. double-click table to open table props Works. 8. double-click bullet/number of list to open props Works. Also works in table. This actually fixes it so that if you double click words in a list it does not bring up the properties dialog. This patch merely restores the original behavior it had before bug 185889 landed. Contextual menus don't seem to have a problem. Clicking on text inside a list inside a table brings up a menu with the list stuff on it *and* the table stuff.

Comment on attachment 115401 [details] [diff] [review] Patch For Other Problems v1.0.1 r=brade please add a comment in EditorDblClick on why we want to use event.explictOriginalTarget instead of event.target Thanks!

Comment on attachment 115401 [details] [diff] [review] Patch For Other Problems v1.0.1 re-marking sr=jst; the code hasn't changed since the sr :)

Checkin status: both patches are in trunk, attachment 115194 [details] [diff] [review] is on 1.3 branch. attachment 115401 [details] [diff] [review] awaits approval for 1.3 branch.

Comment on attachment 115401 [details] [diff] [review] Patch For Other Problems v1.0.1 a=asa (on behalf of drivers) for checkin to 1.3

OK, both patches checked in to trunk and branch.

thanks, john --vrfy'd fixed on all platforms (win2k, linux rh8.0, mac 10.2.4) with recent comm trunk builds (2/28, 3/3).

Comment on attachment 115401 [details] [diff] [review] Patch For Other Problems v1.0.1 I apparently didn't actually check this into trunk before.

If this isn't in the trunk can you get it there this evening?

Comment on attachment 115401 [details] [diff] [review] Patch For Other Problems v1.0.1 a=asa for checkin to 1.4a

Fix checked in to trunk, really ;)

*** Bug 197423 has been marked as a duplicate of this bug. ***