Revert bug 346079 or make it less annoying (scrolling the mouse wheel in the search bar switches the engine)

VERIFIED FIXED in Firefox 3 beta3

Status

()

Firefox
Search
VERIFIED FIXED
11 years ago
7 years ago

People

(Reporter: reed, Assigned: Ehsan)

Tracking

({regression, ue})

Trunk
Firefox 3 beta3
regression, ue
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3 +
in-litmus +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment)

(Reporter)

Description

11 years ago
So, the new "feature" that was added in bug 346079 is just an annoyance to me to others. I accidentally change the search engine with my mouse wheel all the time causing my searches to go off to weird search engines in my list instead of Google. What mconnor thought might happen in bug 346079, comment #9 is happening... This is a poor user experience, especially when I don't want to change my search engine.

Bug 346079, comment #0 has a better idea, I think:
> Perhaps this would apply only to the engine icon itself.

I think that would be better, imho. It still allows for easy changing without driving people insane.

If that's not acceptable or doable, I think the entire patch from bug 346079 should be reverted.
Flags: blocking-firefox3?

Comment 1

11 years ago
How about making it (Ctrl/Cmd)+scroll to switch engines, to be consistent with (Ctrl/Cmd)+arrow?
(Reporter)

Comment 2

11 years ago
Oh, and I forgot to mention the thing that caused me to file this bug in the first place:

http://forums.mozillazine.org/viewtopic.php?t=617386

Apologies to those who were CC'd initially but don't care about this bug. I used "Clone This Bug" on the original bug, so it brought over the entire CC list.
(Assignee)

Comment 3

11 years ago
Created attachment 295210 [details] [diff] [review]
Patch to change the engine on accel+scroll

A simple fix for this problem, as mentioned in comment 1 is to change the engine only when the accel key is pressed.  This patch implements this fix.
Assignee: nobody → ehsan.akhgari
Status: NEW → ASSIGNED
Attachment #295210 - Flags: ui-review?(beltzner)
Attachment #295210 - Flags: review?(gavin.sharp)
(Assignee)

Updated

11 years ago
Attachment #295210 - Attachment description: Patch to change the engine on ctrl+scroll → Patch to change the engine on accel+scroll
Comment on attachment 295210 [details] [diff] [review]
Patch to change the engine on accel+scroll

Agree with the premise of the bug, and think that this is the right solution.
Attachment #295210 - Flags: ui-review?(beltzner) → ui-review+

Comment 5

11 years ago
I'd just like to provide further "anecdotal reports of [mconnor's] concern"; I noticed the change to searchbar functionality and it's been bugging the hell out of me as I change my search engine every 2-3 minutes, and I would absolutely love it if it just went away.

That said -- Meta+scroll is hardly discoverable.  I've noticed the new glow on the searchbar dropdown arrow in recent builds; perhaps something similar to that which only appears when you're holding down Meta, just to draw attention to the possibilities of interaction?
Attachment #295210 - Flags: review?(gavin.sharp) → review+
(Reporter)

Comment 6

11 years ago
Comment on attachment 295210 [details] [diff] [review]
Patch to change the engine on accel+scroll

Fix a problem with the scrollwheel causing an unexpected change in search engine.
Attachment #295210 - Flags: approval1.9?

Updated

11 years ago
Attachment #295210 - Flags: approval1.9? → approval1.9+
(Assignee)

Comment 7

11 years ago
Marking for check-in.  Now that the patch has approval, the blocking flag is no longer needed.
Flags: blocking-firefox3?
Keywords: checkin-needed
(Reporter)

Comment 8

11 years ago
(In reply to comment #7)
> Now that the patch has approval, the blocking flag is no longer needed.

Please do not remove blocking requests that you did not request yourself. just because a patch has been approved doesn't mean that the bug isn't a blocker. If the bug gets reopened for some reason, drivers need to see the blocking request, and if you remove it, that never happens.
(Assignee)

Comment 9

11 years ago
(In reply to comment #8)
> (In reply to comment #7)
> > Now that the patch has approval, the blocking flag is no longer needed.
> 
> Please do not remove blocking requests that you did not request yourself. just
> because a patch has been approved doesn't mean that the bug isn't a blocker. If
> the bug gets reopened for some reason, drivers need to see the blocking
> request, and if you remove it, that never happens.

Sorry for the annoyance, my mistake.  I'm requesting the blocking flag again...
Flags: blocking-firefox3?
(Reporter)

Comment 10

11 years ago
Checking in browser/components/search/content/search.xml;
/cvsroot/mozilla/browser/components/search/content/search.xml,v  <--  search.xml
new revision: 1.115; previous revision: 1.114
done
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3 M11

Updated

10 years ago
Flags: blocking-firefox3? → blocking-firefox3+
I only made the modifier+scroll work on Mac, which makes sense, given the patch.

Verified FIXED using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b4pre) Gecko/2008020504 Minefield/3.0b4pre
Status: RESOLVED → VERIFIED
(In reply to comment #11)
> I only made the modifier+scroll work on Mac, which makes sense, given the
> patch.
> 
> Verified FIXED using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US;
> rv:1.9b4pre) Gecko/2008020504 Minefield/3.0b4pre

Whoops; re-marking as FIXED, so I can verify on Linux/Windows, because Fusion overrides CTRL as a zoom modifier when it's coupled with scroll.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → RESOLVED
Last Resolved: 11 years ago10 years ago
Resolution: --- → FIXED
Sorry for that false start; verified for real now with the rest of the builds:

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b4pre) Gecko/2008020604 Minefield/3.0b4pre

and Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4pre) Gecko/2008020604 Minefield/3.0b4pre
Status: RESOLVED → VERIFIED

Comment 14

10 years ago
ability to be able to enable/disable the mouse wheel search engine changing would be useful (from the optinos menu for example), since holding down ctrl is not as confortable, and many are alrealy very used to using the mouse wheel

Comment 15

10 years ago
I cant think of a good GUI place for this feature to be listed, perhaps leave it with ctrl modifier b default but have an about:config entry that allows any modifier key (including empty) to be used.
later versions could include a GUI option.
(Assignee)

Comment 16

10 years ago
(In reply to comment #15)
> I cant think of a good GUI place for this feature to be listed, perhaps leave
> it with ctrl modifier b default but have an about:config entry that allows any
> modifier key (including empty) to be used.
> later versions could include a GUI option.

If you file a followup bug for this and CC me, I'd be willing to provide a patch.  :-)
(Assignee)

Comment 17

10 years ago
(In reply to comment #16)
> (In reply to comment #15)
> > I cant think of a good GUI place for this feature to be listed, perhaps leave
> > it with ctrl modifier b default but have an about:config entry that allows any
> > modifier key (including empty) to be used.
> > later versions could include a GUI option.
> 
> If you file a followup bug for this and CC me, I'd be willing to provide a
> patch.  :-)

-> bug 417413
(Assignee)

Updated

10 years ago
Depends on: 417413
I've updated the litmus testcase to catch this behavior:
https://litmus.mozilla.org/show_test.cgi?id=6671
Flags: in-litmus+
Comment on attachment 295210 [details] [diff] [review]
Patch to change the engine on accel+scroll

>       <handler event="DOMMouseScroll"
>                phase="capturing"
>-               action="this.selectEngine(event, (event.detail > 0));"/>
>+#ifdef XP_MACOSX
>+               action="if (event.metaKey) this.selectEngine(event, (event.detail > 0));"/>
>+#else
>+               action="if (event.ctrlKey) this.selectEngine(event, (event.detail > 0));"/>
>+#endif
>     </handlers>
So, you find that modifiers="accel" doesn't work for a scroll event. (You did try, I hope. After all, modifiers="accel" works in <key>s and XBL key and mouse handlers.) So what do you do? Rather than filing a bug on XBL, you hack around it in UI code of course :-(
(Assignee)

Comment 20

7 years ago
(In reply to comment #19)
> Comment on attachment 295210 [details] [diff] [review]
> Patch to change the engine on accel+scroll
> 
> >       <handler event="DOMMouseScroll"
> >                phase="capturing"
> >-               action="this.selectEngine(event, (event.detail > 0));"/>
> >+#ifdef XP_MACOSX
> >+               action="if (event.metaKey) this.selectEngine(event, (event.detail > 0));"/>
> >+#else
> >+               action="if (event.ctrlKey) this.selectEngine(event, (event.detail > 0));"/>
> >+#endif
> >     </handlers>
> So, you find that modifiers="accel" doesn't work for a scroll event. (You did
> try, I hope. After all, modifiers="accel" works in <key>s and XBL key and mouse
> handlers.) So what do you do? Rather than filing a bug on XBL, you hack around
> it in UI code of course :-(

I don't remember what the results of my tests were, and what the current status is.  Have you verified that this problem (modifiers=accel not working for scroll events) still exists on trunk?

Updated

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