134503 - Double clicking on the scroll bar arrow launches Properties dialog

Status: VERIFIED FIXED

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

Description

[Jaime Rodriguez, Jr.]

Build ID: 2002032803
Reproducible: Always

1. Launch N6
2. Select Edit Page from the File menu to edit a document
3. Edit your document
4. Scroll down the page you created, by clicking quickly on the down arrow

Results: The Table Properties dialogue is launched

Expected Behavior: Should be allowed to scroll down the page without the
dialogue poping up and slowing my work down.

Comment 1

[Jaime Rodriguez, Jr.]

Nominating for nsbeta1, and marking as ADT3. There is an existing workaround
(i.e. Stop clicking on the down arrow in the scroll bar, and use the slider),
but it defintely frustrates the user. This is not that severe because a savy
user could work around the issue, but it will affect heavy publishing users who
scroll using down arrow.

Comment 2

[Kathleen :Brade]

The suggested workaround is insufficient.  On Mac, that could cause the context
menu to appear (click-hold activiates).

Simon fixed some or all of this issue recently.  Simon--any pointers/suggestions?

nominate for editorbase since 99% of composer users double-click and 99% of
composer users use scrollbars so we must have lots of users who "double-click"
in scrollbars...

Comment 3

[Charles Manske]

Please supply a sample page. I can't reproduce this.

Comment 4

[michael wendell]

I am able to reproduce this problem on the 04-01 trunk build. 

But it does not appear to bring up table properties everytime.  What I am seeing
is that Composer brings up the properties of whatever object the caret is inside.  

For example, if the caret is inside a link when you double click the scrollbar,
link properties will pop up.  If the caret is inside a table, the table
properties will come up. etc, etc, etc

Comment 5

[Charles Manske]

This is lame! The dblclick event should NOT get into the content window.
Simon: This is because the scrollbar is inside the content window, right?
Double-clicking ends up being processed by EditorDblClick(). Luckily, I can
identify it by the nodeName. Hacky fix comming.

Comment 6

[Charles Manske]

Attachment: Patch v1

Comment 7

[Akkana Peck]

I know some ways of getting a node's name are slow -- is element.nodeName one of
them?  If so, we might want to cache rather than getting it twice for every
double-click.

The name of the scrollbar arrow being "html" seems lame, as does the fact that
it claims to be part of the content area.  That creates another bug, which is
that the cursor remains an I-beam when mousing over the scrollbar.  We should
eventually fix that as well (should I file a separate bug?  Is there one already?)

But I can't think of anything else that should have a node name of "html" that
we *would* want to double-click on, so I'm willing to r=akkana this as a
temporary fix, pending an answer about the performance question.  (Kin or Simon
should know.)

Comment 8

[Charles Manske]

Darn. The I-beam cursor over the scrollbars is the side effect of fixing 
bug 128534: the I-beam is the default cursor for the entire content area.
Not having arrow over scrollbars sucks! Please file a new bug. I think we should 
backout fix to 128534 if there's not another way to fix that.
Simon: is there some css attribute or class that we can use to identify the 
scrollbar parts?
I can't see how getting the nodeName when you double click would be a 
performance issue! If you want to scroll faster, you click and hold on arrow,
not repeatedtly click, right?

Comment 9

[Jaime Rodriguez, Jr.]

adt1.0.0+ (on behalf of ADT) for checkin to the 1.0 trunk.

Comment 10

[Scott MacGregor]

Charlie asked me to comment in this bug about how mailnews handles this same
situation. The code we have in mailnews to handle this problem can be found at
the top of threadPane.js. We use element.localName to check for the elements we
are interested in so we we don't process double clicks from scrollbars.

Comment 11

[Simon Fraser [no longer active]]

Comment on attachment 77105 [details] [diff] [review]
Patch v1

sr=sfraser

Comment 12

[Simon Fraser [no longer active]]

Comment on attachment 77105 [details] [diff] [review]
Patch v1

Withdrawing sr.
Talked to kin about this, and he nailed the real problem. The deal here is that
the editor's double-click handler is put on the <editor> element, whereas it
used to live on the <body> of the document being edited. Now that it's too
high, it gets double-clicks from the scrollbar (since these bubble up).

So the real solution is to move the double-click handler off of the <editor>
tag (which I know was a bad idea), and dynamically set it on the body of the
editor's document in the 'document done' callback, like we used to in C++.

Comment 13

[Syd Logan]

removing nomination

Comment 14

[Syd Logan]

Should only have adt1.0.0 nomination after review and super review are in hand

Comment 15

[Charles Manske]

Attachment: Patch v2

1. "onclick" and "ondblclick" attribute removed from XUL for both Composer and 

Message Composer.
2. Click event handlers were added on the <body> element instead. Thus the 
handler doesnt' get the dblclick message from the scrollbar.
3. A new MessageComposeDocumentStateListener was added so the double-click
monitoring works in Message Composer as well. (The existing
DocumentStateListener
for Message Composer is implemented in C++, so we couldn't use that one.)

Comment 16

[Charles Manske]

Testing proceedures:
In both web and message Composers: double click on HR, Images, Links, Lists, and
inside tables (past end of text) and you see the approriate property dialog.
Double-clicking on text (except in a link) should select a word.
In Composer-only, a single click on an element icon in "Show All Tags" mode
should select the element.
Right click should bring up context menu and should not be affected by this fix.

Comment 17

[Kathleen :Brade]

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

Comment 18

[Kathleen :Brade]

-->brade since cmanske is on vacation

Comment 19

[Kathleen :Brade]

Attachment: same as Charley's patch except editor.js is factored

I took Charley's patch and factored the event listener code in editor.js.  This
made the entire patch file smaller and should make the code more readable.

Comment 20

[Akkana Peck]

Comment on attachment 78349 [details] [diff] [review]
same as Charley's patch except editor.js is factored

r=akkana. Nice and clean, and works fine in composer, html and plaintext mail
compose.

I do have one request: I was confused by why we had to register a mail-specific
document state listener in the mail case in EditorSharedStartup but we aren't
registering anything for other cases there.  I came to the conclusion that this
was because the composer window registers its listener in EditorStartup, but
mail compose doesn't call that routine and only calls EditorSharedStartup.  Is
that true?  (In that case, should EditorStartup live in ComposerCommands.js
instead of here?)  Could you add a comment in EditorSharedStartup explaining
why mail is treated differently here?

I wonder, actually: do we need the editor click handler in the plaintext
editor?  Maybe we should eventually put all the click handler registration in
EditorSharedStartup and not call it in the plaintext case?

Comment 21

[Akkana Peck]

Comment on attachment 78349 [details] [diff] [review]
same as Charley's patch except editor.js is factored

Whoops, found a problem.  We do need to handle plaintext composer separately --
if I run the editor on a plaintext file (so it goes into plaintext mode) and
double click, the word does get selected, but I also see two JS errors:

************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "Component returned failure code: 0x80004001
(NS_ERROR_NOT_IMPLEMENTED) [nsIEditorShell.GetSelectedElement]"  nsresult:
"0x80004001 (NS_ERROR_NOT_IMPLEMENTED)"  location: "JS frame ::
chrome://editor/content/editor.js :: EditorDblClick :: line 1298"  data: no]
************************************************************
************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "Component returned failure code: 0x80004001
(NS_ERROR_NOT_IMPLEMENTED) [nsIEditorShell.GetSelectedElement]"  nsresult:
"0x80004001 (NS_ERROR_NOT_IMPLEMENTED)"  location: "JS frame ::
chrome://editor/content/editor.js :: EditorDblClick :: line 1298"  data: no]
************************************************************

Plaintext mail is fine, it's only plaintext compose that's affected.

(There's also a problem involving single vs. double click, but Kathy already
fixed that.)

Comment 22

[Kathleen :Brade]

Attachment: improved patch

This patch fixes the click/dbl-click problem in previous patches as well as
addresses some plaintext editing problems.

Comment 23

[Akkana Peck]

Comment on attachment 78390 [details] [diff] [review]
improved patch

r=akkana.  This one works everywhere I can think of to test it: composer, html
mail, plaintext mail, composer on *.txt, plaintext editor, and textareas, doing
the right thing for single and double clicks with no error messages.

Comment 24

[Kathleen :Brade]

Attachment: patch (don't call removal so no JS warnings)

Comment 25

[Akkana Peck]

Comment on attachment 78932 [details] [diff] [review]
patch (don't call removal so no JS warnings)

r=akkana
This one works on linux in all the cases I tested, no JS warnings even at exit
time.  Kathy and I and Simon are under the understanding that the removal will
happen automatically at the right time when the document is torn down, without
having to be specified explicitly.

Comment 26

[kinmoz]

Comment on attachment 78932 [details] [diff] [review]
patch (don't call removal so no JS warnings)

sr=[e-mail]

Comment 27

[Jaime Rodriguez, Jr.]

Me thinks this should stay as an [ADT3], as the reporter, and knowing the ADT
criteria (i.e. This is not very severe, nor do all people click on the arrow to
scroll down the page *really quickly*. Some use keyboard nav, some mouse roller,
and others grip the slider).

Comment 28

[Kathleen :Brade]

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

Comment 29

[Jaime Rodriguez, Jr.]

Adding adt1.0.0-.  We can ship Beta without this.  Let's look at this again for RTM.

Comment 30

[Charles Manske]

I'm back

Comment 31

[Kathleen :Brade]

taking bug back; this landed on the trunk

Comment 32

[Kathleen :Brade]

fix landed on trunk only

Comment 33

[sujay]

verified on 4/18 trunk.

Comment 34

[Jaime Rodriguez, Jr.]

We should take this one for RTM, and should consider for beta as this is a
regression.

Comment 35

[jag (Peter Annema)]

Erh ... so ... isn't |removeEditorClickEventListener| a dead function then?
If so, please remove it (and maybe add a comment in
|NotifyDocumentWillBeDestroyed| that we don't need to explicitely remove the
listener).

Comment 36

[Kathleen :Brade]

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

Comment 37

[scottputterman]

I didn't realize how bad this was until it started happening to me.  Changing
back to ADT2 and renominating.  It looks like this has been verified on the trunk.

Comment 38

[scottputterman]

adding adt1.0.0+.  Please check into the branch as soon as possible and add the
fixed1.0.0 keyword.

Comment 39

[Scott Collins]

Comment on attachment 78932 [details] [diff] [review]
patch (don't call removal so no JS warnings)

a=scc for checkin to the mozilla 1.0 branch

Comment 40

[Kathleen :Brade]

checked into branch

Comment 41

[sujay]

verified on 4/25 branch build.

Comment 42

[Kathleen :Brade]

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