597723 - Mouse scrolling should not close doorhanger notifications

Status: VERIFIED FIXED

(Toolkit :: General, defect)

Description

[Mehdi Mulani [:mmm] (I don't check this)]

Attachment: Page to test out.

On Mac OS X the OS simulates "momentum scrolling" by sending more scroll events after the user has stopped scrolling.

If the user is in the middle of a scroll or finished but momentum scrolling has taken over, a doorhanger notification could be shown and then instantly removed.

Wasn't able to produce a test case but instead a page to test the geolocation doorhanger not showing from a scroll.

Comment 1

[Alex Limi (:limi) — Firefox UX Team]

My recommendation would be to not dismiss the doorhanger notification on scroll events. It seems weird, since the notification is attached to the chrome, and a scroll operation is easy to trigger accidentally. Clicking in the content area focuses the content, and it makes sense to dismiss the dialog then, but scrolling doesn't change focus (at least on the Mac, you can scroll a background window without changing focus, and I believe the same to be the case on Windows, at least).

Comment 2

[:Margaret Leibovic]

Should all panels remain on scroll events, like the site identity panel and the bookmark star panel? I feel like these panels should behave consistently, and right now those panels also disappear on scroll events.

Comment 3

[Alex Limi (:limi) — Firefox UX Team]

Yes. Scrolling is not an event that should cancel the notification panels, clicking outside them is.

Comment 4

[:Margaret Leibovic]

This sounds like a widget level bug, so I'm cc'ing Enn to see what his thoughts are.

Comment 5

[Neil Deakin]

<panel> elements close when the mousewheel is used; this is so that autocomplete widgets rollup. However, this probably shouldn't be the case for all panels.

The relevant check is at http://mxr.mozilla.org/mozilla-central/source/layout/xul/base/src/nsXULPopupManager.cpp#216

Comment 6

[:Felipe Gomes (needinfo for replies!)]

Attachment: Patch

Per comment 3, this makes the mouse wheel to not dismiss arrow panels

Comment 7

[Neil Deakin]

I'd rather you did the reverse and checked specifically for autocomplete popups.

Comment 8

[:Felipe Gomes (needinfo for replies!)]

The awesomebar autocomplete panel is of type="autocomplete-richlistbox". What should I do for that?
 - match both autocomplete and autocomplete-richlistbox?
 - pr is it possible (and desirable) to match autocomplete-* ?
 - or add a new attribute for mousewheel rollup and check for that

Comment 9

[Neil Deakin]

Check that <textbox type="autocomplete" as is done in nsMenuPopupFrame::ConsumeOutsideClicks()

Comment 10

[:Felipe Gomes (needinfo for replies!)]

I can't seem to find an association between the panel and the textbox.

nsMenuChainItem->GetParent and nsMenuPopupFrame->mAnchorContent are both null, and the parent of the panel is the popupset, not the textbox.

Comment 11

[Johnathan Nightingale [:johnath]]

Aside: Felipe, you asked me in IRC if this was intended to be a hard or soft blocker since the whiteboard didn't match the etherpad. We originally considered this a hard blocker, but the consequences of losing the doorhanger are usually nuisance (failing to remember a password, failing to install an addon), not catastrophic, so we are treating it as a soft blocker (that, like many soft blockers, we would REALLY like to fix.)

Comment 12

[:Gavin Sharp [email: [e-mail]]]

The nsMenuPopupFrame::ConsumeOutsideClicks code just looks wrong. It would only ever be useful for autocomplete textboxes that don't specify external popups (i.e. whose popups are dynamically created and placed inside the textbox's anonymous content, which isn't the case for the URL bar), but even then GetParent() on the menupopup wouldn't find the actual textbox, as felipe points out.

Comment 13

[Neil Deakin]

OK, so let's just do something simple then. Either just check for a type attribute with any value, the type attribute set to 'autocomplete*' or just use a new attribute 'closeonscroll'. We can find a better solution later.

Comment 14

[:Felipe Gomes (needinfo for replies!)]

Attachment: Patch v2

As the rollup was originally meant only for autocomplete popups, I chose to check for "autocomplete*"

Comment 15

[Neil Deakin]

Comment on attachment 504876 [details] [diff] [review]
Patch v2

> PRBool nsMenuPopupFrame::ConsumeOutsideClicks()
> {
>+  //felipe

Don't think you meant to do this.


>+    *aShouldRollup = (value.Find(NS_LITERAL_STRING("autocomplete")) != -1);

Instead I'd use:

  *aShouldRollup = StringBeginsWith(value, NS_LITERAL_STRING("autocomplete"));

Comment 16

[:Felipe Gomes (needinfo for replies!)]

http://hg.mozilla.org/mozilla-central/rev/9c96acb338ca

Comment 17

[Cameron Dawson [:camd]]

Verified:  Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b11) Gecko/20100101 Firefox/4.0b11