Closed Bug 134503 Opened 23 years ago Closed 23 years ago

Double clicking on the scroll bar arrow launches Properties dialog

Categories

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

Product:
Core
Component:
DOM: UI Events & Focus Handling
Type:
defect
Priority:
Not set
Severity:
major

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla1.0

People

(Reporter: jaimejr, Assigned: Brade)

References

Details

(Keywords: regression, Whiteboard: [ADT2] editorbase, [FIX ON TRUNK])

Attachments

(1 file, 4 obsolete files)

patch (don't call removal so no JS warnings)
(deleted), patch
akkzilla
: review+
kinmoz
: superreview+
scc
: approval+
Details | Diff | Splinter Review
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.
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.
Keywords: nsbeta1
Whiteboard: [ADT3]
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...
Whiteboard: [ADT3] → [ADT3] editorbase
Target Milestone: --- → mozilla1.0
Please supply a sample page. I can't reproduce this.
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
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.
Assignee: syd → cmanske
Component: Editor: Composer → Event Handling
Keywords: patch, review
Whiteboard: [ADT3] editorbase → [ADT3] editorbase, FIX IN HAND, need r=,sr=
Attached patch Patch v1 (obsolete) (deleted) — — Splinter Review
Status: NEW → ASSIGNED
Summary: Double clicking on the scroll bar arrow launches Table Properties → Double clicking on the scroll bar arrow launches Properties dialog
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.)
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?
Keywords: adt1.0.0
adt1.0.0+ (on behalf of ADT) for checkin to the 1.0 trunk.
Keywords: adt1.0.0 → adt1.0.0+
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 on attachment 77105 [details] [diff] [review] Patch v1 sr=sfraser
Attachment #77105 - Flags: superreview+
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++.
Attachment #77105 - Flags: superreview+ → needs-work+
removing nomination
Keywords: adt1.0.0+, nsbeta1 → nsbeta1+
Should only have adt1.0.0 nomination after review and super review are in hand
Attached patch Patch v2 (obsolete) (deleted) — — Splinter Review
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.)
Attachment #77105 - Attachment is obsolete: true
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.
*** Bug 136290 has been marked as a duplicate of this bug. ***
-->brade since cmanske is on vacation
Assignee: cmanske → brade
Status: ASSIGNED → NEW
Keywords: mozilla1.0, regression
OS: Windows 98 → All
Hardware: PC → All
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.
Attachment #78036 - Attachment is obsolete: true
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?
Attachment #78349 - Flags: review+
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.)
Attachment #78349 - Flags: review+ → needs-work+
Attached patch improved patch (obsolete) (deleted) — — Splinter Review
This patch fixes the click/dbl-click problem in previous patches as well as addresses some plaintext editing problems.
Attachment #78349 - Attachment is obsolete: true
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.
Attachment #78390 - Flags: review+
Attachment #78390 - Flags: needs-work+
Attachment #78390 - Attachment is obsolete: true
Status: NEW → ASSIGNED
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.
Attachment #78932 - Flags: review+
Comment on attachment 78932 [details] [diff] [review] patch (don't call removal so no JS warnings) sr=kin@netscape.com
Attachment #78932 - Flags: superreview+
Keywords: adt1.0.0
Whiteboard: [ADT3] editorbase, FIX IN HAND, need r=,sr= → [ADT2] editorbase, FIX IN HAND
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).
Whiteboard: [ADT2] editorbase, FIX IN HAND → [ADT3] editorbase, FIX IN HAND
*** Bug 137619 has been marked as a duplicate of this bug. ***
Adding adt1.0.0-. We can ship Beta without this. Let's look at this again for RTM.
Keywords: adt1.0.0 → adt1.0.0-
Whiteboard: [ADT3] editorbase, FIX IN HAND → [ADT3 RTM] editorbase, FIX IN HAND
I'm back
Assignee: brade → cmanske
Status: ASSIGNED → NEW
taking bug back; this landed on the trunk
Assignee: cmanske → brade
fix landed on trunk only
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
verified on 4/18 trunk.
Status: RESOLVED → VERIFIED
We should take this one for RTM, and should consider for beta as this is a regression.
Whiteboard: [ADT3 RTM] editorbase, FIX IN HAND → [ADT3 RTM] editorbase, [FIX ON TRUNK]
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).
*** Bug 139220 has been marked as a duplicate of this bug. ***
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.
Keywords: adt1.0.0- → adt1.0.0
Whiteboard: [ADT3 RTM] editorbase, [FIX ON TRUNK] → [ADT2] editorbase, [FIX ON TRUNK]
adding adt1.0.0+. Please check into the branch as soon as possible and add the fixed1.0.0 keyword.
Keywords: adt1.0.0 → adt1.0.0+
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
Attachment #78932 - Flags: approval+
checked into branch
Keywords: fixed1.0.0
verified on 4/25 branch build.
Keywords: verified1.0.0
*** Bug 142800 has been marked as a duplicate of this bug. ***
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: