some keyboard events for datetimepicker, trees and listboxes can bubble up to the scroll the main content when they shouldn't

RESOLVED FIXED in mozilla7

Status

()

Toolkit
XUL Widgets
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jmaher, Assigned: Neil Deakin (mostly unavailable until September))

Tracking

(Depends on: 1 bug)

Trunk
mozilla7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [specialpowers][inbound])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

6 years ago
while working on moving mochitest-plain tests to mochitest-chrome in toolkit/widgets (bug 508710), I found a scenario where the browser would reload the url which would cause the tests to restart (sort of an infinite loop).

Here is some more information about it:
https://bugzilla.mozilla.org/show_bug.cgi?id=508710#c3
(Reporter)

Updated

6 years ago
Blocks: 508710
Whiteboard: [specialpowers]

Comment 1

6 years ago
Mossop, do you know of any other domain experts who can take a look at this while Neil is out?  This is blocking our on going work to remove enable privilege from mochitest.
The two Neils are really the experts here, might try catching NeilAway on IRC later in the day.

Comment 3

6 years ago
(In reply to comment #2)
> The two Neils are really the experts here, might try catching NeilAway on IRC
> later in the day.

Thanks Dave.
From the other bug, the problem is that certain events on these widgets aren't using preventDefault to stop scrolling of the main page from occurring.
Summary: datetimepicker, trees and listboxes can cause the browser to reload → some keyboard events for datetimepicker, trees and listboxes can bubble up to the scroll the main content when they shouldn't
Created attachment 536597 [details] [diff] [review]
preventdefault on scroll events for datepicker and tree

Listboxes already call preventDefault so I didn't change them.
Assignee: nobody → enndeakin
Status: NEW → ASSIGNED
Attachment #536597 - Flags: feedback?(jmaher)
Attachment #536597 - Flags: review?(neil)

Updated

6 years ago
Attachment #536597 - Flags: review?(neil) → review+

Comment 6

6 years ago
(In reply to comment #4)
> From the other bug, the problem is that certain events on these widgets
> aren't using preventDefault to stop scrolling of the main page from
> occurring.

The actual cause of bug 508710 turned out to be due to a stray page down event although that's orthogonal to the DOMMouseScroll event fix.
(Reporter)

Comment 7

6 years ago
Comment on attachment 536597 [details] [diff] [review]
preventdefault on scroll events for datepicker and tree

Review of attachment 536597 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/content/tests/widgets/tree_shared.js
@@ +1187,5 @@
> +  function mouseScrollListener(event) {
> +    defaultPrevented++;
> +  }
> +  window.addEventListener("DOMMouseScroll", mouseScrollListener, false);
> +

any chance this is going to affect other tests?  we have 6 test_tree_*.xul files that depend on tree_shared.js.
Attachment #536597 - Flags: feedback?(jmaher) → feedback+
(Reporter)

Updated

6 years ago
Duplicate of this bug: 667034
(Reporter)

Comment 9

6 years ago
testing this patch with test_datepicker.xul, I find that we are still scrolling the main window down.
(Reporter)

Comment 10

6 years ago
so far these are the tests which are causing the main window to scroll while running the test in an iframe:
 - test_datepicker.xul
 - test_mousecapture.xul (http://mxr.mozilla.org/mozilla-central/source/toolkit/content/tests/widgets/test_mousecapture.xul#163 and line 168 seem to be the problems)
 - test_tree_column_reorder.xul (only on OSX)

and these other tests fail to complete the test (I assume a lack of focus or something related to the focus and popup status):
 - test_menubar.xul
 - test_contextmenu_nested.xul
It looks as if the page down and page up keys need to be cancelled in the datepicker as well.
(In reply to comment #11)
> It looks as if the page down and page up keys need to be cancelled in the
> datepicker as well.
And possibly in trees and listboxes too, but we've presumably been lucky in that we've had no tests for them?
(Reporter)

Comment 13

6 years ago
Created attachment 542363 [details] [diff] [review]
add preventDefault to VK_[PG]DOWN

updated patch (with my zero knowledge of this area) to add a preventDefault to the down and pagedown key presses.  I assume this needs to be done in other areas as well or could have other side effects, so I am just uploading this patch since it does allow test_datetime.xul to pass!
Listboxes already call preventDefault(). Trees don't, but the tests which perform page up/down don't seem to cause a problem.
(In reply to comment #14)
> Listboxes already call preventDefault().
Ah yes, I now see there's a subtle difference between listboxes and scales as to where the prevent default happens.
(Reporter)

Comment 16

6 years ago
what are the next steps that are needed to make this an official patch and get it landed?
Created attachment 542528 [details] [diff] [review]
patch with tests

This patch adds preventDefault for scroll and page up/down events to datepicker with tests. I didn't add it to <tree> as that doesn't seem to be a problem.
Attachment #536597 - Attachment is obsolete: true
Attachment #542363 - Attachment is obsolete: true
Attachment #542528 - Flags: review?(neil)
(In reply to comment #17)
> I didn't add it to <tree> as that doesn't seem to be a problem.
Perhaps our tests just don't test this sort of thing well enough. After all, it was only luck that exposed the original bug in the first place.
Created attachment 543222 [details] [diff] [review]
updated patch

This patch adds event cancelling for key events in the tree. It also cancels the MozMousePixelScroll event as I found out was needed in bug 668502.
Attachment #542528 - Attachment is obsolete: true
Attachment #542528 - Flags: review?(neil)
Attachment #543222 - Flags: review?(neil)
Comment on attachment 543222 [details] [diff] [review]
updated patch

>+           if (this.changeOpenState(this.currentIndex, false)) {
>+              event.preventDefault();
>+              return;
>+            }
Indentation is wrong here. r=me with that fixed.
Attachment #543222 - Flags: review?(neil) → review+
(Reporter)

Updated

6 years ago
Whiteboard: [specialpowers] → [specialpowers][inbound]
http://hg.mozilla.org/mozilla-central/rev/ae8e81569712
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla7

Updated

6 years ago
Depends on: 734937
You need to log in before you can comment on or make changes to this bug.