Closed Bug 625465 Opened 9 years ago Closed 20 hours ago

Items in list view should launch description view with a single click on an add-on entry

Categories

(Toolkit :: Add-ons Manager, defect, P3)

defect

Tracking

()

RESOLVED INVALID
Tracking Status
blocking2.0 --- .x+

People

(Reporter: jboriss, Unassigned)

References

Details

(Keywords: polish)

Attachments

(8 files, 12 obsolete files)

109.87 KB, image/png
Details
1.18 MB, image/png
Details
147.91 KB, image/png
Details
1.38 KB, image/png
Details
1.38 KB, image/png
Details
1.43 KB, image/png
Details
242.42 KB, image/png
Details
45.20 KB, patch
mossop
: review-
Details | Diff | Splinter Review
tl;dr: A patch for this bug should: (1) make descriptive view one click away in list view, (2) add a mouseover state for list view entries, and (3) add an arrow graphic at the end of each list view description.

Currently, there are two ways for a user to go from list view in the add-ons manager to description view.  

1. Double clicking an entry in list view.  Double clicking is largely a desktop interaction that is rarely used on the web and should be avoided where possible.  The only current uses of double clicking within Firefox are actions meant to simulate desktop interactions (eg double clicking a bookmark in the Library to open it, as though it were a file or application).  The other problem with double clicking is that it's utterly undiscoverable.

2. Clicking the "more" link after an add-on's description.  This has been in the design since the original mockups, and is certainly more discoverable than double clicking.  However, feedback from the nightlies has consistently shown that users don't click on "more" and aren't aware that this opens description view.  I've sat down with a few users and told them to open list view and then get more information about a particular add-on.  Most say they don't know how to access additional information.  I then suggest they click on "more," and the response is usually: "oh, *that's* what that does."  Some users clicked once on an add-on entry.

Because description view is currently not easily discoverable, the design is being changed so a single click on an item in list view launches description view.

This is a move that has some benefits and drawbacks, but whose benefits I feel outweigh the drawbacks.

Benefits:

- Simplified UI.  "More" button is no longer needed.

- Easier to discover.  Clicking once on an entry was the action most users took to get more information about an add-on when asked.  This is probably because the items in list view already have some affordances of buttons in their styling - they're square, like targets, and have shading and gradients similar to buttons.  On desktop computers, a subtle mouseover coloring will reinforce further that each item is a button.  On touch devices, a single click on a large target delve in is arleady  with most UI.  

- Easier target to get to descriptive mode.  With a button the size of list view, it should be quicker and easier for users to view more of an add-on's details.

Drawbacks:

- Makes multi-select harder for future releases.  The reason that individual list view items were not specificed to be buttons in the original design is because multi-select was in the plan for Firefox 4.0.  Multi-selection would allow users to quickly take bulk actions on add-ons without finding a preference or making many individual actions.  However, multi-select is no longer planned for the add-ons manager in Firefox 4.0, and to lose out on the benefits because this may be a part of a future design seems be a net loss.  The add-ons manager is functional without multi-select, which would always be an expert and hidden feature, even if implemented.  Ultimately, we should be designing each feature optimally for how the add-ons manager is and not how it may be.

- The fact that two buttons (remove and disable) will still be present in list view makes the clickable list view item somewhat inconsistent with other Firefox widgets.  The two buttons on each entry essentially are buttons-within-buttons, which is an unusual move that is taken nowhere else in Firefox.  An added disadvantage is that missing the target for remove and disable could mean accidentally entering descriptive view.  However, I feel this drawback is worth the advantages.  The two buttons that remain are important to leave in the forefront of list view because they are the most common interactions users would take with add-ons, and most users should never need descriptive view.  To users that do use descriptive view, it's an easy single click to enter.  If descriptive view is accidentally access, a single click on the back buttons restores the previous screen.  I don't think users will find this somewhat unusual widget hard to understand and use.

If one click on a list view item were the only way to enter descriptive view, it's possible it would still not be discoverable enough.  Even though the list view items have affordances of clickable buttons, the fact that there are buttons-within-buttons may make users hesitate to click on the list view entry itself.  To this end, placing an arrow graphic at the end of the description of an add-on would show that there is more information a click away.  If users only use this arrow "button" to enter descriptive view, all they are missing is an easier target.  If users know that one click enters descriptive view, a single arrow graphic should not clutter the list view UI too much.
Blocks: 623250
tracking-fennec: --- → ?
Keywords: polish, uiwanted
Whiteboard: [softblocker]
That's kinda hard switch compared what we have right now. The entries in the different categories still feel like a listbox items. I believe that at least at the beginning users will be irritated with this behavior. Especially that a complete new page appears immediately. Something like sliding the content of the listbox out of the visible area and slide in the detail page would IMO help users a lot here to understand what's happening.

Otherwise I agree with the broken discovery of the details view, and that we should improve it.
I assume you also meant blocking2.0 and not Fennec.
blocking2.0: --- → ?
tracking-fennec: ? → ---
blocking2.0: ? → final+
(In reply to comment #2)
> I assume you also meant blocking2.0 and not Fennec.

Yep.

(In reply to comment #1)
> That's kinda hard switch compared what we have right now. The entries in the
> different categories still feel like a listbox items. I believe that at least
> at the beginning users will be irritated with this behavior. Especially that a
> complete new page appears immediately. Something like sliding the content of
> the listbox out of the visible area and slide in the detail page would IMO help
> users a lot here to understand what's happening.
> 
> Otherwise I agree with the broken discovery of the details view, and that we
> should improve it.
Some sliding animation may be possible in the future.  And I agree that this behavior may surprise people at first.  However, I think it's far worse that right now, no one knows how to get more information about an add-on.
Assignee: nobody → bmcbride
Status: NEW → ASSIGNED
Attached patch WiP v1 (obsolete) — Splinter Review
Makes item select-on-hover, and enter detail view with a single click. Needed a bunch of test fixes. 

It *doesn't* currently test that hover selects items - sending mousemove events via EventUtils.synthesizeMouse() isn't working for me, and I have no idea why. Any hints would be greatly appreciated!

I'm blocked on images from Boriss. Specifically, an image for the button that's to replace the "More" link - for Windows and OSX (Linux should have a stock icon), for normal, hover, and maybe active states. Also mockups (or at least a description) showing what a hovered item looks like (since there was talk of it being slightly different). Note: if this takes more than a day or two more, I suggest this gets landed without it and have that fixed in a followup bug. This sort of change is something that needs beta coverage, IMO.
Attachment #506634 - Flags: feedback?(dtownsend)
(In reply to comment #4)
> It *doesn't* currently test that hover selects items - sending mousemove events
> via EventUtils.synthesizeMouse() isn't working for me, and I have no idea why.
> Any hints would be greatly appreciated!

Do you have a snippet so we can get more insight in which specific code you have used for the test?
Attached file WiP test (not working) (obsolete) —
Here's the unfinished test I was trying to get working (I meant to attach it earlier). The hover-on-select code is basically the same as what the autocomplete binding uses (with some cleaning up and handling scrolling)... but I couldn't find any test for that.
Comment on attachment 506712 [details]
WiP test (not working)

>function get_item_center(aItem) {
>  var list = gManagerWindow.document.getElementById("addon-list");
>  return {
>    x: (aItem.boxObject.x - list.boxObject.x) + (aItem.boxObject.width / 2),
>    y: (aItem.boxObject.y - list.boxObject.y) + (aItem.boxObject.height / 2)
>  };

That's not needed when you use synthesizeMouseAtCenter:
http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/EventUtils.js#233

But shouldn't we better use the top left corner, i.e. (10, 10)? Not sure if for smaller windows the buttons are already present in the middle of the list entry.

>  EventUtils.synthesizeMouse(list, itemCenter.x, itemCenter.y, {type: "mousemove"}, gManagerWindow);

Can you check if the list entry receives the correct event? synthesizeMouseExpectEvent would be a good helper to test that.
(In reply to comment #4)
> Created attachment 506634 [details] [diff] [review]
> WiP v1
> 
> Makes item select-on-hover, and enter detail view with a single click. Needed a
> bunch of test fixes. 

Do we actually need select-on-hover? All the bug requests is a mouseover state
(In reply to comment #8)
> Do we actually need select-on-hover? All the bug requests is a mouseover state

Yes - to keep keyboard navigation in sync and working sanely. It feels very wrong (and confusing) without that.
Comment on attachment 506634 [details] [diff] [review]
WiP v1

In general looks ok but I'm suspicious of the selection logic. Some comments explaining what it is intending to do would help. As far as I can tell as soon as the mouse enters any richlistitem it will get selected (because a mouseout will have fired making mLastMoveTime null so Date.now() is large enough to trigger the change). So I'm not sure what the rest of the timing stuff in the mousemove handler is meant to be doing.
Attachment #506634 - Flags: feedback?(dtownsend) → feedback-
Keywords: uiwanted
The attached is a guide for the colors and textures of list view for Windows 7 and OSX.

When the user mouses over an item in list view, a color change should suggest that the add-ons are clickable targets. 
When the user clicks, the add-on’s entry should recess slightly, producing a shadow consistent with the texture of the manager.

Windows 7 Normal:

Enabled add-ons: background varies with shine and gradient
Disabled add-ons: #e1e5eb solid background
Blocked add-ons: #f1d5da solid background

Windows 7 Mouseover:

Enabled add-ons: #f3f7fb solid background
Disabled add-ons: #f3f7fb solid background, faded text and icon do not change
Blocked add-ons: #efe9ec solid background,  faded text and icon do not change

Windows 7 On Click:

Enabled add-ons: #edf3f8 solid background, black shadow, 15% opacity, 120° angle, 1px distance, 4px wide
Disabled add-ons: #edf3f8 solid background, black shadow, 15% opacity, 120° angle, 1px distance, 4px wide
Blocked add-ons: #f1e3e7 solid background, black shadow, 15% opacity, 120° angle, 1px distance, 4px wide

OSX Normal:

Enabled add-ons: about #c9cfd8, varies slightly due to gradient
Disabled add-ons: #c8cdd3 solid background
Blocked add-ons: #d9bcc2 solid background

OSX Mouseover:

Enabled add-ons: #e3e6eb solid background
Disabled add-ons: #e3e6eb solid background (faded text and icon never change)
Blocked add-ons: #ddcad0 solid background (faded text and icon never change)

OSX On Click: 

Enabled add-ons: #d6dbe2 solid background, black shadow, 15% opacity, 120° angle, 1px distance, 4px wide
Disabled add-ons: #d6dbe2 solid background, black shadow, 15% opacity, 120° angle, 1px distance, 4px wide
Blocked add-ons: #dac2cb solid background, black shadow, 15% opacity, 120° angle, 1px distance, 4px wide
Photoshop file for the attached guide can be found at https://people.mozilla.com/~jboriss/bugs/bug625465/
Blocks: 630682
Blocks: 630683
I'm attaching a guide to the arrow button shown in the mockups and the graphic files themselves.
Attached image Graphic file: Normal arrow icon (obsolete) —
Attached image Graphic file: Clicked arrow icon (obsolete) —
(In reply to comment #9)
> (In reply to comment #8)
> > Do we actually need select-on-hover? All the bug requests is a mouseover state
> 
> Yes - to keep keyboard navigation in sync and working sanely. It feels very
> wrong (and confusing) without that.

As long as I'm spamming everyone mercilessly - Blair, are there any other graphics or files you need here?
Attachment #509276 - Attachment is obsolete: true
Attached image Graphic file: Clicked arrow icon (obsolete) —
Attachment #509279 - Attachment is obsolete: true
Attachment #510204 - Attachment is obsolete: true
(In reply to comment #10)
> In general looks ok but I'm suspicious of the selection logic. Some comments
> explaining what it is intending to do would help. As far as I can tell as soon
> as the mouse enters any richlistitem it will get selected (because a mouseout
> will have fired making mLastMoveTime null so Date.now() is large enough to
> trigger the change). So I'm not sure what the rest of the timing stuff in the
> mousemove handler is meant to be doing.

I removed the mouseout handler - I can't remember why I added that, and it seemed questionable/wrong. But essentially, the timing stuff ensures we don't kill performance, by making sure at least 30ms has passed since the last update. That code is an adaption of what the autocomplete listbox binding does (with stuff added to make scrolling update the selected item too).
Attached patch Patch v1 (obsolete) — Splinter Review
Updated with comments, and the new styles for selected/pressed items, and the "more" button.

Does *not* have a test, yet.
Attachment #506634 - Attachment is obsolete: true
Attachment #511517 - Flags: review?(dtownsend)
(In reply to comment #22)
> Created attachment 511517 [details] [diff] [review]
> Patch v1
> 
> Updated with comments, and the new styles for selected/pressed items, and the
> "more" button.
> 
> Does *not* have a test, yet.

This is looking great!  One slight correction on OSX: the on-click shadow is just a bit too dark compared to the mockups (see attached) - could you decrease the opacity on that a bit?
(In reply to comment #23)
> This is looking great!  One slight correction on OSX: the on-click shadow is
> just a bit too dark compared to the mockups (see attached) - could you decrease
> the opacity on that a bit?

Done - matches much better now (hadn't noticed until that screenshot). Gonna try to finish the test off today (assuming nothing else gets in the way again).
Comment on attachment 511517 [details] [diff] [review]
Patch v1

Looks ok though Boriss commented on an issue that needs fixing. It looks like you are testing that clicking on the list does go to the detail view, maybe making the selection test test using the mouseover event is all that is needed otherwise here?
Attachment #511517 - Flags: review?(dtownsend) → review+
Attached patch Patch v2 (obsolete) — Splinter Review
A test! Also made a small change to the way the timing is handled, since testing uncovered that the first mousemove event was being ignored. And I've reduced the opacity of the inset shadow on OSX by 5% (sounds like a tiny amount, but 10% made it too light).
Attachment #511517 - Attachment is obsolete: true
Attachment #512698 - Flags: review?(dtownsend)
Should this get reviewed, it has implicit a=beltzner
Whiteboard: [softblocker] → [softblocker][pre-approved by beltzner]
Comment on attachment 512698 [details] [diff] [review]
Patch v2

Looks good however please don't include the string removal in the initial landing. Since this is a softblocker if we find some regressions from this then it'll have to bounce and I don't want that to turn into an l10n problem. We can do the string removal after a few days when it looks clear.
Attachment #512698 - Flags: review?(dtownsend) → review+
Attached patch Patch v2.01 (obsolete) — Splinter Review
Patch without the string removal, ready for landing.
Attachment #506712 - Attachment is obsolete: true
Attachment #512698 - Attachment is obsolete: true
Separate string removal patch.
Keywords: checkin-needed
Whiteboard: [softblocker][pre-approved by beltzner] → [softblocker][pre-approved by beltzner][ready to land]
http://hg.mozilla.org/mozilla-central/rev/6b3353ea6228

not closing bug since there is still the string removal patch... May I suggest to move it to a separate dedicated bug so this can be closed?
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [softblocker][pre-approved by beltzner][ready to land] → [softblocker][pre-approved by beltzner][landed][string removal has to land yet]
backed out due to permaorange
http://hg.mozilla.org/mozilla-central/rev/fbb6c03b8cbc
http://hg.mozilla.org/mozilla-central/rev/6e3e92057d43

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1297949342.1297950589.25787.gz

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser-window/browser_list.js | Focus should have moved to the more button - Got [object XULElement], expected [object XULElement]
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser-window/browser_list.js | Focus should have moved to the disable button - Got [object HTMLInputElement], expected [object XULElement]
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser-window/browser_list.js | Focus should have moved to the remove button - Got [object XULElement], expected [object XULElement]
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser-window/browser_list.js | Focus should have moved to the remove button - Got [object XULElement], expected [object XULElement]
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser-window/browser_list.js | Focus should have moved to the more button - Got [object XULElement], expected [object XULElement]
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser-window/browser_list.js | Focus should have moved to the list - Got [object HTMLInputElement], expected [object XULElement]
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/browser_list.js | Focus should have moved to the more button - Got [object XULElement], expected [object XULElement]
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/browser_list.js | Focus should have moved to the disable button - Got [object HTMLInputElement], expected [object XULElement]
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/browser_list.js | Focus should have moved to the remove button - Got [object XULElement], expected [object XULElement]
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/browser_list.js | Focus should have moved to the remove button - Got [object XULElement], expected [object XULElement]
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/browser_list.js | Focus should have moved to the more button - Got [object XULElement], expected [object XULElement]
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/browser_list.js | Focus should have moved to the list - Got [object XULElement], expected [object XULElement]
Whiteboard: [softblocker][pre-approved by beltzner][landed][string removal has to land yet] → [softblocker][pre-approved by beltzner][needs new patch (permaorange)]
Attached patch Patch v2.02 - ready for checkin (obsolete) — Splinter Review
*sigh* Test bitrot. The new focus additions to browser_list.js was clicking on an item, expected it to only gain focus.focus it.
Attachment #512970 - Attachment is obsolete: true
Attachment #512970 - Attachment description: Patch v2.01 - ready for checkin → Patch v2.01
Whiteboard: [softblocker][pre-approved by beltzner][needs new patch (permaorange)] → [softblocker][pre-approved by beltzner][needs landing]
(In case someone gets to it before I do)
Keywords: checkin-needed
what will happen when you back out of the description view, will you preserve the scroll position in the list?
(In reply to comment #0)
>  If descriptive view is accidentally access, a single
> click on the back buttons restores the previous screen.  

Will it really, or will it return us to the *top* of the extension list, making us scroll all the way down again?
(In reply to comment #35)
> what will happen when you back out of the description view, will you preserve
> the scroll position in the list?

That's bug 586956.
Landed <http://hg.mozilla.org/mozilla-central/rev/8c2aa133200a> and backed out <http://hg.mozilla.org/mozilla-central/rev/f523472a47e6> because of test failures:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1298062783.1298066033.23047.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1298062587.1298065895.22796.gz
etc.

Note that this took bug 631827 down with it.
Keywords: checkin-needed
Whiteboard: [softblocker][pre-approved by beltzner][needs landing] → [softblocker][pre-approved by beltzner]
This seems to fail mochitest-other on debug builds on all platforms and OPT builds on Windows.
Test failure is:

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/browser_bug625465.js | Correct item should be selected - Got 1, expected 3
Attached patch Patch v2.03 (obsolete) — Splinter Review
Fixed test - passes on slower debug builds now.

However, I'm gonna ask for re-approval since it missed the final beta (in comment 4 I said I wanted beta feedback). I think the code is relatively safe and is all tested, but the UI change is going to be a surprise.
Attachment #513286 - Attachment is obsolete: true
Attachment #514461 - Flags: approval2.0?
Comment on attachment 514461 [details] [diff] [review]
Patch v2.03

While I agree that this is a behavioural change, I think it's the one that will be expected and come across like a bugfix. Thanks for checking, though!

a=beltzner
Attachment #514461 - Flags: approval2.0? → approval2.0+
Keywords: checkin-needed
Whiteboard: [softblocker][pre-approved by beltzner] → [softblocker][has patch][needs landing]
This needs a new patch, as the current patch doesn't apply cleanly and the merging is not trivial.
Keywords: checkin-needed
Whiteboard: [softblocker][has patch][needs landing] → [softblocker][has patch]
Attached patch Patch v2.04 (obsolete) — Splinter Review
*sigh* Bitrot. Including with one of my own patches.
Attachment #514461 - Attachment is obsolete: true
Keywords: checkin-needed
Whiteboard: [softblocker][has patch] → [softblocker][has patch][needs landing]
Comment on attachment 515038 [details] [diff] [review]
Patch v2.04

>--- a/toolkit/themes/winstripe/mozapps/extensions/extensions.css
>+++ b/toolkit/themes/winstripe/mozapps/extensions/extensions.css

>+.details:-moz-focusring > .button-box {
>+  border-color: transparent;
> }

Other than breaking keyboard focus feedback, what's this supposed to do?
Keywords: checkin-needed
Whiteboard: [softblocker][has patch][needs landing] → [softblocker][has patch]
(In reply to comment #45)
> Comment on attachment 515038 [details] [diff] [review]
> Patch v2.04
> 
> >--- a/toolkit/themes/winstripe/mozapps/extensions/extensions.css
> >+++ b/toolkit/themes/winstripe/mozapps/extensions/extensions.css
> 
> >+.details:-moz-focusring > .button-box {
> >+  border-color: transparent;
> > }
> 
> Other than breaking keyboard focus feedback, what's this supposed to do?

Since the richlistitem is focusable and Enter activates it, I don't think you want this button to be focusable at all.

While playing with the patch, I also noticed that scrolling through the list with the arrow keys repeatedly focuses the item under the mouse cursor when it rests somewhere on the list. :\
(In reply to comment #0)
> Benefits:
> 
> - Simplified UI.  "More" button is no longer needed.

This doesn't work on Linux, since the new icon seems visually more complex than the "more" link. Can we entirely remove this thing and just use the hand pointer over the entire item (except for the inner buttons)?
(In reply to comment #47)
> (In reply to comment #0)
> > Benefits:
> > 
> > - Simplified UI.  "More" button is no longer needed.
> 
> This doesn't work on Linux, since the new icon seems visually more complex than
> the "more" link. Can we entirely remove this thing and just use the hand
> pointer over the entire item (except for the inner buttons)?

Is there a Linux icon equivalent we could substitute in?  Is the actual problem the button color?  Maybe I could add a more transparent version for Linux.
(In reply to comment #46)
> (In reply to comment #45)
> > Comment on attachment 515038 [details] [diff] [review]
> > Patch v2.04
> > 
> > >--- a/toolkit/themes/winstripe/mozapps/extensions/extensions.css
> > >+++ b/toolkit/themes/winstripe/mozapps/extensions/extensions.css
> > 
> > >+.details:-moz-focusring > .button-box {
> > >+  border-color: transparent;
> > > }
> > 
> > Other than breaking keyboard focus feedback, what's this supposed to do?
> 
> Since the richlistitem is focusable and Enter activates it, I don't think you
> want this button to be focusable at all.
> 
> While playing with the patch, I also noticed that scrolling through the list
> with the arrow keys repeatedly focuses the item under the mouse cursor when it
> rests somewhere on the list. :\

While it's not a big deal, I think it's be preferable to make the more icon focusable. The reason is that it's essentially there to give people a visual cue that there's more information, since not all users will know that the entire list view item is clickable. Not making the item focusable to such users will reintroduce the problem. To them it could seem that focus is simply broken, and thus there's no way to move to detailed view while just using the cursor.
(In reply to comment #48)
> > > Benefits:
> > > 
> > > - Simplified UI.  "More" button is no longer needed.
> > 
> > This doesn't work on Linux, since the new icon seems visually more complex than
> > the "more" link. Can we entirely remove this thing and just use the hand
> > pointer over the entire item (except for the inner buttons)?
> 
> Is there a Linux icon equivalent we could substitute in?

I don't think so...

> Is the actual problem the button color?

I got an orange icon, so yes, the color is part of the problem.

> Maybe I could add a more transparent version for Linux.

Why does the icon exist at all? Removing it should be a simplicity win across the board.
(In reply to comment #49)
> While it's not a big deal, I think it's be preferable to make the more icon
> focusable. The reason is that it's essentially there to give people a visual
> cue that there's more information, since not all users will know that the
> entire list view item is clickable.

Shouldn't the mouse cursor expose the clickability?

> Not making the item focusable to such users
> will reintroduce the problem. To them it could seem that focus is simply
> broken, and thus there's no way to move to detailed view while just using the
> cursor.

Are you referring to keyboard users now? For them I think a focus ring on the whole item is needed, as specified in the default richlistitem styling: http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/winstripe/global/richlistbox.css#62
(In reply to comment #50)
> (In reply to comment #48)
> Why does the icon exist at all? Removing it should be a simplicity win across
> the board.

1. It's a more minimal alternative to the more link we were using before
2. It's more consistent than the more link, as blue underlined links are being used for external URLs and not expanded information in the manager
3. It gives a visual cue to show the invisible information that clicking will provide more information

> Shouldn't the mouse cursor expose the clickability?

The changing style and cursor should suggest clickability, yes.  That's the  reason for showing a mouseover state.

> Are you referring to keyboard users now? For them I think a focus ring on the
> whole item is needed, as specified in the default richlistitem styling:
> http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/winstripe/global/richlistbox.css#62

There should definitely be a focus ring on the whole item. I was suggesting that we might want to have one on just the arrow button as well. But again, that's not a big deal.

Blair, would a semi-transparent version of the more button solve the problem on Linux that Dao is referring to?
(In reply to comment #52)
> > Shouldn't the mouse cursor expose the clickability?
> 
> The changing style and cursor should suggest clickability, yes.  That's the 
> reason for showing a mouseover state.

The patch adds the mouseover state only for the icon. I'm asking why it's not there for the whole clickable list item.
(In reply to comment #53)
> (In reply to comment #52)
> > > Shouldn't the mouse cursor expose the clickability?
> > 
> > The changing style and cursor should suggest clickability, yes.  That's the 
> > reason for showing a mouseover state.
> 
> The patch adds the mouseover state only for the icon. I'm asking why it's not
> there for the whole clickable list item.

Oh! I see what you're saying!  Sorry that I was unclear - the entire list view item *should* have a mouseover state cursor, because the whole item is a button.
Comment on attachment 514461 [details] [diff] [review]
Patch v2.03

a-; sorry, we're closing up for RC, this has languished as approved for too long, now too late.
Attachment #514461 - Flags: approval2.0+ → approval2.0-
Fair enough - sadly, this ended up having a bunch of different holdups (and playing with the latest changes, it still doesn't feel right).
** PRODUCT DRIVERS PLEASE NOTE **

This bug is one of 7 automatically changed from blocking2.0:final+ to blocking2.0:.x during the endgame of Firefox 4 for the following reasons:

 - it was marked as a soft blocking issue without a requirement for beta coverage
blocking2.0: final+ → .x+
(In reply to comment #56)
> Fair enough - sadly, this ended up having a bunch of different holdups (and
> playing with the latest changes, it still doesn't feel right).

Sadness. Blair, are you willing poke this again after 4?
(Bah, forgot to hit submit on Friday.)

Yes, want to get this finished still. 

Having the mouse cursor turn into a pointer when hovering over items, but not the buttons in those items, feels awkward. Will get a Try build for you to play with.
Flags: in-testsuite+
(In reply to comment #59)
> (Bah, forgot to hit submit on Friday.)
> 
> Yes, want to get this finished still. 
> 
> Having the mouse cursor turn into a pointer when hovering over items, but not
> the buttons in those items, feels awkward. Will get a Try build for you to play
> with.

The cursor shouldn't change when over a button - the user's only moving from a button to a more different button.  The button changing on hover should be enough to point out the distinction.
Blair, can you please provide a patch which can be landed on cedar (or alternatively, land it yourself over there?)

Thanks!
No longer blocks: 630682
Attachment #512972 - Attachment is obsolete: true
Attachment #515038 - Attachment is obsolete: true
Whiteboard: [softblocker][has patch]
Attached patch Patch v3Splinter Review
Woo! Blast from the past!

This is an unbitrotten version of the previous version of the patch, and addresses all remaining issues bought up in previous comments (such as comment 46 and comment 54).

Been tinkering with this about once every 6 months. The test has been the thorn in my side (specifically on Linux) - managed to finally get it working reliably (4 successful Try runs) this week, after converting over to the new DOM3 WheelEvent.

For review, changes that aren't just fixing bitrot should be limited to:
* event handling code in extensions.xml
* final change in content/extensions.css
* browser_list_mouse.js (formally browser_bug625465.js)
Attachment #663687 - Flags: review?(dtownsend+bugmail)
(In reply to Jennifer Morrow [:Boriss] (Firefox UX) from comment #60)
> (In reply to comment #59)
> > (Bah, forgot to hit submit on Friday.)
> > 
> > Yes, want to get this finished still. 
> > 
> > Having the mouse cursor turn into a pointer when hovering over items, but not
> > the buttons in those items, feels awkward. Will get a Try build for you to play
> > with.
> 
> The cursor shouldn't change when over a button - the user's only moving from
> a button to a more different button.  The button changing on hover should be
> enough to point out the distinction.

So we probably need to fix bug 646713 too then.
(In reply to Dave Townsend (:Mossop) from comment #63)
> So we probably need to fix bug 646713 too then.

Ugh, yea. Off to do more yak shaving, I guess....
Comment on attachment 663687 [details] [diff] [review]
Patch v3

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

Probably only simple stuff here but I'd really like for bug 646713 to be fixed before this lands.

::: toolkit/mozapps/extensions/content/extensions.css
@@ +21,5 @@
>    -moz-binding: url("chrome://mozapps/content/extensions/extensions.xml#sorters");
>  }
>  
> +.list {
> +  -moz-binding: url("chrome://mozapps/content/extensions/extensions.xml#addons-richlistbox");  

trailing whitespace

::: toolkit/mozapps/extensions/content/extensions.xml
@@ +508,5 @@
> +            item = item == this ? null : item.parentNode;
> +  
> +          if (!item)
> +            return;
> +  

trailing whitespace on a bunch of the above lines

@@ +526,5 @@
> +
> +        var now = Date.now();
> +
> +        if ((!this.mLastMoveTime) ||
> +            (now - this.mLastMoveTime > this.mMouseMoveThreshold)) {

So this means it's technically possible to end up with the mouse over a different element to that selected?

How about instead using DeferredTask to update the selection 30ms after the last mousemove/scroll?
Or would using mouseover/mouseout events be better?

@@ +537,5 @@
> +            return;
> +
> +          var index = this.getIndexOfItem(item);
> +          if (index != this.selectedIndex)
> +            this.selectedIndex = index;

A bunch of the above is repeated in _handleMouseScroll. It's not a lot but I think it still might be worth sharing the code to make sure they both get updated when necessary.

@@ +552,5 @@
> +        if (this.mMouseScrollEvents == 0)
> +          return;
> +        this.mMouseScrollEvents--;
> +        this._handleMouseScroll();
> +      ]]></handler>

I don't really get what's going on with these two handlers, can you add some comments?

@@ +1671,5 @@
>        <handler event="click" button="0"><![CDATA[
> +       if (event.originalTarget.localName != "button" &&
> +           event.originalTarget.localName != "checkbox" &&
> +           !event.originalTarget.classList.contains("text-link")) {
> +          this.showInDetailView();

I'm a little worried that this blocklist might break extension added content. Is there a short whitelist we can use instead, say label, description, image and richlistitem?
Attachment #663687 - Flags: review?(dtownsend+bugmail) → review-
Assignee: blair → nobody
Status: ASSIGNED → NEW
Priority: -- → P3

This no longer applies to the HTML version, it opens with a single click.

Status: NEW → RESOLVED
Closed: 20 hours ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.