Closed Bug 597723 Opened 9 years ago Closed 9 years ago

Mouse scrolling should not close doorhanger notifications

Categories

(Toolkit :: General, defect)

x86
macOS
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla2.0b10
Tracking Status
blocking2.0 --- final+

People

(Reporter: mmm, Assigned: Felipe)

References

(Depends on 1 open bug)

Details

(Whiteboard: [softblocker][doorhanger])

Attachments

(2 files, 1 obsolete file)

Attached file 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.
blocking2.0: --- → final+
Keywords: uiwanted
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).
Keywords: uiwanted
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.
Yes. Scrolling is not an event that should cancel the notification panels, clicking outside them is.
This sounds like a widget level bug, so I'm cc'ing Enn to see what his thoughts are.
<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
Product: Firefox → Toolkit
QA Contact: general → general
Assignee: nobody → felipc
Attached patch Patch (obsolete) — Splinter Review
Per comment 3, this makes the mouse wheel to not dismiss arrow panels
Attachment #502584 - Flags: review?(enndeakin)
Whiteboard: [soft blocker]
I'd rather you did the reverse and checked specifically for autocomplete popups.
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
Check that <textbox type="autocomplete" as is done in nsMenuPopupFrame::ConsumeOutsideClicks()
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.
Whiteboard: [soft blocker] → [softblocker]
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.)
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.
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.
Attached patch Patch v2Splinter Review
As the rollup was originally meant only for autocomplete popups, I chose to check for "autocomplete*"
Attachment #502584 - Attachment is obsolete: true
Attachment #504876 - Flags: review?(enndeakin)
Attachment #502584 - Flags: review?(enndeakin)
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"));
Attachment #504876 - Flags: review?(enndeakin) → review+
Summary: Momentum scrolling can cause doorhanger notifications to not be seen → Mouse scrolling should not close doorhanger notifications
http://hg.mozilla.org/mozilla-central/rev/9c96acb338ca
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b10
Depends on: 631473
Verified:  Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b11) Gecko/20100101 Firefox/4.0b11
Status: RESOLVED → VERIFIED
Whiteboard: [softblocker] → [softblocker][doorhanger]
Depends on: 643770
Depends on: 643758
You need to log in before you can comment on or make changes to this bug.