193689 - double click in compose window is broken (doesn't select word)

Status: VERIFIED FIXED

(Core :: DOM: Events, defect)

Description

[Mitch]

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.

Comment 1

[esther]

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

Comment 2

[Kathleen :Brade]

-->selection

Comment 3

[Kathleen :Brade]

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

Comment 4

[John Keiser (jkeiser)]

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.

Comment 5

[R.K.Aa.]

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

Comment 6

[sairuh (rarely reading bugmail)]

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.

Comment 7

[chris hofmann]

what should we do for 1.3 on this one?

Comment 8

[Kathleen :Brade]

backout the checkin that caused this regression?

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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?

Comment 10

[John Keiser (jkeiser)]

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.

Comment 11

[John Keiser (jkeiser)]

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.

Comment 12

[John Keiser (jkeiser)]

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.

Comment 13

[John Keiser (jkeiser)]

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.

Comment 14

[saari (gone)]

EDITORBASE+

Comment 15

[John Keiser (jkeiser)]

Attachment: Patch

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 16

[John Keiser (jkeiser)]

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 17

[Johnny Stenback (:jst)]

Comment on attachment 115194 [details] [diff] [review]
Patch

sr=jst

Comment 18

[John Keiser (jkeiser)]

Attachment: Patch For Other Problems

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 19

[Johnny Stenback (:jst)]

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

sr=jst

Comment 20

[Brian Ryner (not reading)]

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

Are you sure that behavior for tables isn't intentional?

Comment 21

[John Keiser (jkeiser)]

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

Yes, I checked against an older build.

Comment 22

[Asa Dotzler [:asa]]

Comment on attachment 115194 [details] [diff] [review]
Patch

a=asa (on behalf of drivers) for checkin to 1.3

Comment 23

[John Keiser (jkeiser)]

Attachment: Patch For Other Problems, Really

Oops, I uploaded the wrong file before.

Comment 24

[Johnny Stenback (:jst)]

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

sr=jst

Comment 25

[John Keiser (jkeiser)]

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.

Comment 26

[Daniel Bratell]

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 27

[Kathleen :Brade]

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).

Comment 28

[John Keiser (jkeiser)]

Attachment: Patch For Other Problems v1.0.1

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 29

[Kathleen :Brade]

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 30

[John Keiser (jkeiser)]

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 :)

Comment 31

[John Keiser (jkeiser)]

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 32

[Asa Dotzler [:asa]]

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

Comment 33

[John Keiser (jkeiser)]

OK, both patches checked in to trunk and branch.

Comment 34

[sairuh (rarely reading bugmail)]

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 35

[John Keiser (jkeiser)]

Comment on attachment 115401 [details] [diff] [review]
Patch For Other Problems v1.0.1

I apparently didn't actually check this into trunk before.

Comment 36

[Asa Dotzler [:asa]]

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

Comment 37

[Asa Dotzler [:asa]]

Comment on attachment 115401 [details] [diff] [review]
Patch For Other Problems v1.0.1

a=asa for checkin to 1.4a

Comment 38

[John Keiser (jkeiser)]

Fix checked in to trunk, really ;)

Comment 39

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

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