402992 - Do not switch to a hand cursor on favicon hover

Status: VERIFIED FIXED

(Firefox :: Shell Integration, defect)

Description

[Alex Faaborg [:faaborg] (Firefox UX)]

Switching to the hand cursor on favicon hover (on both the location bar favicon and tab favicon) has the advantage of making the drag operation more discoverable.  However, this is non standard behavior both in terms of fitting in with other applications on the OS, and in terms of normal use of the hand cursor.  Additionally, changing the mouse cursor when the user happens to mouse over a favicon on their way to another target is slightly jarring.  Favicon drags are not incredibly common, so the hand cursor is most likely to appear in situations where the user has no intention of interacting with the favicon.

While dragging favicons to the bookmarks toolbar is useful, it does not require increased discoverability at the cost of violating user interface guidelines and a minor increase of UI complexity.

Comment 1

[Justin Dolske [:Dolske]]

Bug 372773 recently added this to the tab favicon, the url bar favicon has had this since at least mid 2004.

This is easy enough to undo (look for CSS "cursor: -moz-grab"), and Alex's argument is compelling, but since Beltzner ui-r+ the recent addition of this I'll wait for him to waffle. :)

Comment 2

[Alex Faaborg [:faaborg] (Firefox UX)]

beltzner: can I get ui approval on this?

Comment 3

[Mike Beltzner [:beltzner, not reading bugmail]]

We added it to the tabstrip to differentiate between a "this will drag the tab" action and a "this will drag the URI" action, but really, I think that's something which is better made evident by changing the cursor depending on the hover target of the drag, and even more really, I don't think I care to spend the effort to do that (though I'd take that patch!)

Anyway, ui-r. :)

Comment 4

[Justin Dolske [:Dolske]]

Attachment: Patch for review, v.1

I removed all the CSS rules for #wrapper-urlbar-container, because it seems that element ID doesn't exist any more (ie, the rules were already deadwood).

The only other usage of -moz-grab in the browser is in the window for customizing a toolbar, I didn't bother changing that since it seemed more sensible, and no one uses it anyway. :)

Comment 5

[Asaf Romano (gone)]

Comment on attachment 289890 [details] [diff] [review]
Patch for review, v.1

r=mano code-removal-wise.

Comment 6

[Damon Sicore (:damons)]

So, does beltzner's "ui-r" in Comment #3 above mean a minus?  Sorry if that's normal usage, but it seems ambiguous.  Once someone confirms that all reviews have been done, we'll approve.

Comment 7

[Mike Connor [:mconnor]]

Comment on attachment 289890 [details] [diff] [review]
Patch for review, v.1

I think this is good to go, beltzner's comment was a +, not a -, afaict.

Comment 8

[Justin Dolske [:Dolske]]

Checking in browser/themes/gnomestripe/browser/browser.css;
/cvsroot/mozilla/browser/themes/gnomestripe/browser/browser.css,v  <--  browser.css
new revision: 1.141; previous revision: 1.140
done
Checking in browser/themes/pinstripe/browser/browser.css;
/cvsroot/mozilla/browser/themes/pinstripe/browser/browser.css,v  <--  browser.css
new revision: 1.98; previous revision: 1.97
done
Checking in browser/themes/winstripe/browser/browser.css;
/cvsroot/mozilla/browser/themes/winstripe/browser/browser.css,v  <--  browser.css
new revision: 1.138; previous revision: 1.137
done

Comment 9

[Dão Gottwald [:dao]]

(In reply to comment #4)
> Created an attachment (id=289890) [details]
> Patch for review, v.1
> 
> I removed all the CSS rules for #wrapper-urlbar-container, because it seems
> that element ID doesn't exist any more (ie, the rules were already deadwood).

#wrapper-urlbar-container exists when you customize the toolbar. The patch should be backed out.

Comment 10

[Justin Dolske [:Dolske]]

Attachment: Patch, as checked in.

Comment 11

[Justin Dolske [:Dolske]]

Attachment: Regression patch, v.1

This fixes the regression Dao noted, by restoring the deleted rules. [There's a -moz-grab cursor in them, which is why I noticed them, but the usage there should be fine. I had deleted the rules because I thought they were just entirely unused.]

Comment 12

[Mike Connor [:mconnor]]

Comment on attachment 291564 [details] [diff] [review]
Regression patch, v.1

r=mconnor

Comment 13

[Justin Dolske [:Dolske]]

Checked in. Thanks for catching this so quick, Dao.

Checking in browser/themes/gnomestripe/browser/browser.css;
/cvsroot/mozilla/browser/themes/gnomestripe/browser/browser.css,v  <--  browser.css
new revision: 1.142; previous revision: 1.141
done
Checking in browser/themes/pinstripe/browser/browser.css;
/cvsroot/mozilla/browser/themes/pinstripe/browser/browser.css,v  <--  browser.css
new revision: 1.99; previous revision: 1.98
done
Checking in browser/themes/winstripe/browser/browser.css;
/cvsroot/mozilla/browser/themes/winstripe/browser/browser.css,v  <--  browser.css
new revision: 1.139; previous revision: 1.138
done

Comment 14

[Marcia Knous [:marcia]]

verified fixed using  Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b2pre) Gecko/2007120505 Minefield/3.0b2pre. I verified using the Mac nightly as well.

Comment 15

[Alex Faaborg [:faaborg] (Firefox UX)]

I'm still seeing this with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b3pre) Gecko/2008010604 Minefield/3.0b3pre

Comment 16

[Justin Dolske [:Dolske]]

Are you using Proto? It still sets the cursor to the hand.

Comment 17

[Alex Faaborg [:faaborg] (Firefox UX)]

Yeah, I momentarily forgot that we haven't landed proto yet.  Sorry about that.

Comment 18

[Justin Dolske [:Dolske]]

Proto just landed, and regressed this.

Comment 19

[Justin Dolske [:Dolske]]

Attachment: Patch to fix this again, v.1

Comment 20

[Justin Dolske [:Dolske]]

Since this was the same change as was previously reviewed/approved, I went ahead and landed it.

Checking in browser/themes/pinstripe/browser/browser.css;
/cvsroot/mozilla/browser/themes/pinstripe/browser/browser.css,v  <--  browser.css
new revision: 1.110; previous revision: 1.109
done

Comment 21

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED using:

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b4pre) Gecko/2008020423 Minefield/3.0b4pre

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b3pre) Gecko/2008020419 Minefield/3.0b3pre

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4pre) Gecko/2008020423 Minefield/3.0b4pre

-and-

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

Comment 22

[Stephen Donner [:stephend] Not actively reading bugmail]

Setting in-litmus? so that I remember to remove the hand-cursor verbiage from the testcase(s).