Closed Bug 591753 Opened 14 years ago Closed 9 years ago

Category buttons should highlight on hover even when selected

Categories

(Toolkit :: Add-ons Manager, defect)

defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed
blocking2.0 --- -

People

(Reporter: martijn.martijn, Assigned: 15electronicmotor)

Details

(Whiteboard: [good first bug])

Attachments

(2 files, 3 obsolete files)

The mouse cursor of the left column entries (Get Add-ons, Extension, Themes, Plugins) should be a pointer, so I know that when clicking on them, something happens.

When I'm under "More" under extensions, for example, I'm not really sure I could click on "Extensions" again, to get back again to the beginning of "Extensions".
And when you're at the beginning of "Extensions", the cursor of the "Extensions" entry should be normal, then.
Makes sense. Should be an easy fix.
Severity: normal → minor
blocking2.0: --- → ?
OS: Windows 7 → All
Hardware: x86 → All
Whiteboard: [good first bug]
Version: unspecified → Trunk
I'm not sure we should do this but if UX agree then I'd take a patch. Doesn't block though.
blocking2.0: ? → -
Not sure we'd want this, tbh. The cursor isn't a "hand" on normal browser tabs, so I don't see why it would be for these items (which are essentially tabs rotated 90 deg).

However, I think it might make sense to give them hover effects, depending on the platform. Still uiwanted for that, though. :)
With tabs you at least have a hover effect, the left column entries in the addons manager only have a useless tooltip if you stay too long.
Assignee: nobody → jboriss
Keywords: uiwanted
Jennifer, are you still working on this? I'm going through older "good first bugs" that have been inactive for a while. 
Is this still valid?
Flags: needinfo?(jboriss)
I would like to work on this bug.Please assign it to me if nobody working on it.
Attached patch bug_591753_1.diff (obsolete) — Splinter Review
Attachment #8630393 - Flags: review?(robert.strong.bugs)
Attachment #8630393 - Flags: feedback?
Comment on attachment 8630393 [details] [diff] [review]
bug_591753_1.diff

This really should be reviewed by Dave Townsend or Blair McBride or be directed to someone that has time and the knowledge to review this code even though they are both marked as not accepting review requests
Attachment #8630393 - Flags: review?(robert.strong.bugs)
Attachment #8630393 - Flags: feedback?
Dave, can either you or Blair either review this or direct this towards someone for review
Flags: needinfo?(dtownsend)
Comment on attachment 8630393 [details] [diff] [review]
bug_591753_1.diff

Review of attachment 8630393 [details] [diff] [review]:
-----------------------------------------------------------------

I can review it, but I still want UX to weigh in on whether this is the right thing. Markus?

::: toolkit/mozapps/extensions/content/extensions.css
@@ +24,5 @@
> +.category[selected="true"] > * {
> +  cursor: default;
> +}
> +
> +ort-controls {

This looks broken
Attachment #8630393 - Flags: feedback?(mjaritz)
Flags: needinfo?(jboriss)
Flags: needinfo?(dtownsend)
Dave, thanks for bringing me in.
We should keep aligned with about:preferences on this. As we treat inContent UI as all UI we should not use the hand-cursor. Having a hover state is enough indicator that this UI element is interactive.
(We only use the Hand-Cursor on text-links that do look like links. e.g. More Information; other UI elements work with a hover effect e.g. buttons, lists )
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → WONTFIX
Comment on attachment 8630393 [details] [diff] [review]
bug_591753_1.diff

Review of attachment 8630393 [details] [diff] [review]:
-----------------------------------------------------------------

please do not implement hand-cursor for left column entries.
Attachment #8630393 - Flags: feedback?(mjaritz) → feedback-
Sorry for late response and thanks for review.

If I don't implement hand-cursor then how users know their is something more. Maybe you can tell me what to use.
Flags: needinfo?(mjaritz)
These buttons currently have a hover-state - the background-color changes on hover. This is enough indication that there is something more. So there is nothing more to do on this bug. Thanks!
Flags: needinfo?(mjaritz)
Sorry shivang. In my reply, Imissed one aspect of this bug.
(In reply to Martijn Wargers [:mwargers] (QA) from comment #0)
> When I'm under "More" under extensions, for example, I'm not really sure I
> could click on "Extensions" again, to get back again to the beginning of
> "Extensions".

We should make sure to have a hover effect on the select element as well. Best only if the user is one level down (e.g. under "More" in Extensions) This hover should be similar to the hover of other buttons, but a bit darker. I would suggest using #4F5961 as a hover of the active element.
Thank you Shivang.
Assignee: jboriss → 15electronicmotor
Status: RESOLVED → REOPENED
Flags: needinfo?(15electronicmotor)
Resolution: WONTFIX → ---
I changed the respective css file according to you suggestion but I really think there is not need of more hovering effect on selected button.
Flags: needinfo?(15electronicmotor)
Attached patch Added more hovering effect (obsolete) — Splinter Review
Attachment #8642589 - Flags: review?(mjaritz)
Attachment #8642589 - Flags: feedback?(mjaritz)
Comment on attachment 8642589 [details] [diff] [review]
Added more hovering effect

Hello shivang,
this left column entries should not have a hand cursor, but only the hover effect they currently have.
I was suggesting adding a hover effect for the selected button for users to see that it is clickable, even if selected. (This is important for when "users" are on level down, e,g, under "more", as you already mentioned.)
Seeing the color I suggested implemented, I realize this is the wrong color. Having the dark selected state become darker (#1A2533) on hover keeps better aligned with the design, while still providing a hover.

We should add only the css for hover, and no hand-cursor:

.category[selected="true"]:hover {
  background-color:#1A2533;
}
Attachment #8642589 - Flags: review?(mjaritz)
Attachment #8642589 - Flags: review-
Attachment #8642589 - Flags: feedback?(mjaritz)
Attachment #8642589 - Flags: feedback+
Attached patch bug_591753_3.diff (obsolete) — Splinter Review
Attachment #8630393 - Attachment is obsolete: true
Attachment #8642589 - Attachment is obsolete: true
Attachment #8643008 - Flags: review?(mjaritz)
Attachment #8643008 - Flags: feedback?(mjaritz)
Comment on attachment 8643008 [details] [diff] [review]
bug_591753_3.diff

UI-wise it is ok. For review, best ask Dave [:mossop]
Attachment #8643008 - Flags: review?(mjaritz)
Attachment #8643008 - Flags: review?(dtownsend)
Attachment #8643008 - Flags: feedback?(mjaritz)
Attachment #8643008 - Flags: feedback+
Comment on attachment 8643008 [details] [diff] [review]
bug_591753_3.diff

Review of attachment 8643008 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/mozapps/extensions/content/extensions.css
@@ +20,5 @@
> +.category[selected="true"]:hover {
> +  background-color:#1A2533;
> +}
> +
> +ort-controls {

This looks broken, can you correct it and resubmit?
Attachment #8643008 - Flags: review?(dtownsend)
Sorry about last patch.
Attachment #8643008 - Attachment is obsolete: true
Attachment #8644398 - Flags: review?(dtownsend)
Attachment #8644398 - Flags: feedback?(dtownsend)
Summary: Mouse cursor of left column entries should be pointer → Category buttons should highlight on hover even when selected
Comment on attachment 8644398 [details] [diff] [review]
Removed buggy code.

Review of attachment 8644398 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry I should have spotted this error in the first place. The stylesheet you're amending here controls behavioural features of the UI. For styling you'll want to change this stylesheet: https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/shared/extensions/extensions.inc.css
Attachment #8644398 - Flags: review?(dtownsend)
Attachment #8644398 - Flags: review-
Attachment #8644398 - Flags: feedback?(dtownsend)
But https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/shared/extensions/extensions.inc.css is not the stylesheet for that Add-ons page, then why I have to change this stylesheet ?
Flags: needinfo?(dtownsend)
(In reply to shivang nagaria from comment #24)
> But
> https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/shared/
> extensions/extensions.inc.css is not the stylesheet for that Add-ons page,
> then why I have to change this stylesheet ?

It is the stylesheet for the add-ons page, included through the platform specific stylesheets like https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/osx/mozapps/extensions/extensions.css#5
Flags: needinfo?(dtownsend)
Attachment #8648469 - Flags: review?(dtownsend)
Attachment #8648469 - Flags: feedback?
Comment on attachment 8648469 [details] [diff] [review]
edited appropriate css file.

Review of attachment 8648469 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thank you for the patch.
Attachment #8648469 - Flags: review?(dtownsend)
Attachment #8648469 - Flags: review+
Attachment #8648469 - Flags: feedback?
https://hg.mozilla.org/mozilla-central/rev/cf6138f13ae9
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.