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

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect)

Description

[Joel Maher ( :jmaher ) (UTC -8)]

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

Comment 1

[cmtalbert]

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.

Comment 2

[Dave Townsend [:mossop]]

The two Neils are really the experts here, might try catching NeilAway on IRC later in the day.

Comment 3

[cmtalbert]

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

Comment 4

[Dave Townsend [:mossop]]

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.

Comment 5

[Neil Deakin]

Attachment: preventdefault on scroll events for datepicker and tree

Listboxes already call preventDefault so I didn't change them.

Comment 6

[[e-mail]]

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

Comment 7

[Joel Maher ( :jmaher ) (UTC -8)]

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.

Comment 9

[Joel Maher ( :jmaher ) (UTC -8)]

testing this patch with test_datepicker.xul, I find that we are still scrolling the main window down.

Comment 10

[Joel Maher ( :jmaher ) (UTC -8)]

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

Comment 11

[Neil Deakin]

It looks as if the page down and page up keys need to be cancelled in the datepicker as well.

Comment 12

[[e-mail]]

(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?

Comment 13

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: 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!

Comment 14

[Neil Deakin]

Listboxes already call preventDefault(). Trees don't, but the tests which perform page up/down don't seem to cause a problem.

Comment 15

[[e-mail]]

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

Comment 16

[Joel Maher ( :jmaher ) (UTC -8)]

what are the next steps that are needed to make this an official patch and get it landed?

Comment 17

[Neil Deakin]

Attachment: 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.

Comment 18

[[e-mail]]

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

Comment 19

[Neil Deakin]

Attachment: 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.

Comment 20

[[e-mail]]

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.

Comment 21

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://hg.mozilla.org/mozilla-central/rev/ae8e81569712