506733 - Hovering over result in autocomplete window should highlight it

Status: RESOLVED WONTFIX

(Camino Graveyard :: Location Bar & Autocomplete, enhancement)

Description

[Mehmet]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en; rv:1.9.0.13pre) Gecko/2009072700 Camino/2.1a1pre (like Firefox/3.0.13pre)
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en; rv:1.9.0.13pre) Gecko/2009072700 Camino/2.1a1pre (like Firefox/3.0.13pre)

Hello Camino-Team,

I have a little request for the new URL-Bar autocomplete feature in Camino 2.1:


***see steps to reproduce***

Thanks in advance !
Mehmet



Reproducible: Always

Steps to Reproduce:
1. type a letter into the url-bar
2. Pop-Up menu with the Bookmarks & History comes up
3. Hove with the mousecusor to a bookmark or a history item in the list
Actual Results:  
The blue highlighting don't move when you hove with mousecursor over the url-popup-list to a bookmark or history-item.

Expected Results:  
The blue highlighting should move with the mousecursor and the selected bookmark or history item should be shown in the url-bar. (Like in Safari 4.0.2 or in Firefox 3.x)

Comment 1

[Mehmet]

Attachment: Hardcopy for the request: blue highlightning should move with the mousecursor

Comment 2

[Chris Lawson (gone)]

That is, in fact, how menus generally work in Mac OS X, and even Firefox gets this right :-p

Comment 3

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Since the window looks like a menu, it seems like we should do this to make it not look bizarre.  

The one thing Chris and I don't like is Safari's changing the URL in the location bar on hover; that seems creepy and IE-like (and prone to accidental unwanted behavior).  As long as we only update the URL in the location bar on click, we're good.

Comment 4

[Mehmet]

Attachment: Suggestion

(In reply to comment #3)
> ...The one thing Chris and I don't like is Safari's changing the URL in the
> location bar on hover; that seems creepy and IE-like ....

Only a suggestion - if possible. There could be a second "brighter" highlight (like in Chrome) while mouseover the resultlist. (see screenshot) :P 

Regards
Mehmet

Comment 5

[Chris Peterson [:cpeterson]]

Attachment: autocomplete-v1.patch

Patch v1 implements autocomplete menu highlighting. The patch includes the basic functionality plus some minor UI tweaks.

AutoCompleteTextField.mm:
* Track mouseMoved events when TableView popup is open and select/highlight TableView row under mouse.
* Increase autocomplete menu's opacity from 0.95 to 0.98. I think the page titles are easier to when the web page underneath has lots of text.
* Reduce autocomplete menu's corner radius to 2. I think this looks more "anchored" to the location bar and visually matches the corner radius of Camino's tabs (and Chrome's autocomplete menu corners).
* No need to setBorderColor when setBorderWidth is 0.

AutoCompleteCell.m:
* Reduce header font size from 12 to 11 point (like Safari).
* Indent page title rows so header rows are easier to visually scan (like Safari).
* Draw header text with headerAttributes, not titleAttributes. This fixes some text layout problems from reducing header font size.

Comment 6

[Chris Peterson [:cpeterson]]

Attachment: autocomplete-v2.patch

Patch v2 extends patch v1 with some minor refactoring of the AutoComplete source files:

* Remove unused class AutoCompleteWindow.
* const-ify some static variables.
* AutoCompleteCell drawInteriorWithFrame: avoid allocating header or title attributes when not needed.
* Avoid some repeated method calls by caching results in local variables.

Comment 7

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #5)
> AutoCompleteCell.m:
> * Reduce header font size from 12 to 11 point (like Safari).
> * Indent page title rows so header rows are easier to visually scan (like
> Safari).

We specifically decided not to do these things when Dan did the UI rewrite.

Comment 8

[Chris Peterson [:cpeterson]]

Attachment: autocomplete-v1a.patch

autocomplete-v1a.patch: Incorporate Smokey's feedback (about not changing font size or row indentation).

Comment 9

[Chris Peterson [:cpeterson]]

Attachment: autocomplete-v2a.patch

autocomplete-v2a.patch: Incorporate Smokey's feedback (about not changing font size or row indentation).

Comment 10

[Ilya Sherman]

Comment on attachment 483891 [details] [diff] [review]
autocomplete-v1a.patch

Apologies for the way delayed review!  Please feel free to poke me aggressively if I'm being slow to respond in the future.


>+  // Track the mouse over the TableView, not the location bar or frame borders.
>+  NSRect tableViewTrackingRect;
>+  tableViewTrackingRect.size.width  = tableViewFrame.size.width;
>+  tableViewTrackingRect.size.height = tableViewFrame.size.height;

Based on the line below, it seems like (2 * kFrameMargin) should be subtracted from the width; and that the height needs a similar adjustment.  Does that result in too small a rect?

>+  tableViewTrackingRect.origin.x = locationFrame.origin.x + kFrameMargin;
>+  tableViewTrackingRect.origin.y = locationFrame.origin.y - locationFrame.size.height - kVerticalPadding - intercellSpacingHeight;


>+  // Mouse coordinates are relative to lower-left of the screen.
>+  NSPoint mouseLocation = [NSEvent mouseLocation];
>+
>+  // But TableView coordinates are relative to upper-left of the popup window.
>+  NSPoint tableViewLocation;
>+  tableViewLocation.x = mouseLocation.x - mTrackingRect.origin.x;
>+  tableViewLocation.y = mTrackingRect.origin.y - mouseLocation.y - 1;

I'm probably just being obtuse, but why "- 1" here?

>+  // Is the mouse over the TableView?
>+  if (tableViewLocation.x < 0 || tableViewLocation.x >= mTrackingRect.size.width ||
>+      tableViewLocation.y < 0 || tableViewLocation.y >= mTrackingRect.size.height)
>+  {

nit: This brace should be on the same line as the paren.

>+    [mTableView deselectAll:self];
>+    return;
>+  }
>+
>+  int mouseRow = [mTableView rowAtPoint:tableViewLocation];
>+  if (![self isHeaderRow:mouseRow]) {
>+    [mTableView selectRowIndexes:[NSIndexSet indexSetWithIndex:mouseRow]
>+            byExtendingSelection:NO];
>+  }

I see that this is not what Safari does, but I think we should -deselectAll: when hovering over a header row.  Thoughts from others?

Comment 11

[Ilya Sherman]

Comment on attachment 483892 [details] [diff] [review]
autocomplete-v2a.patch

>-static NSString* kCorePasteboardFlavorType_url = @"CorePasteboardFlavorType 0x75726C20";  // 'url '  url
>+static NSString *const kCorePasteboardFlavorType_url = @"CorePasteboardFlavorType 0x75726C20";  // 'url '  url

nit: "NSString *const" looks really weird to me.  It might be ok as is, but I would write it as "NSString* const" or as "NSString * const".  smorgan would probably have a more definitive suggestion here.


The rest of the v2a changes look fine to me, though I'm not sure how much value there is to avoiding repeated method calls by caching results in local variables.  In a few places this helps with readability, which is good.  In a few other places, though, it slightly hurts readability; and I doubt that we would see any noticeable performance improvement from it.  I'll leave it to your discretion which ones to keep, though -- they all seem more or less fine.


Also, thanks for listing the changes in each patch, and for maintaining these two parallel patches!

Comment 12

[Chris Peterson [:cpeterson]]

Some answers:

> Based on the line below, it seems like (2 * kFrameMargin) should be subtracted
> from the width; and that the height needs a similar adjustment.  Does that
> result in too small a rect?

If I understand your question correctly, I think you may be confusing the tableViewFrame and locationFrame sizes.

The tableViewTrackingRect's width is the *tableViewFrame's* width, not the *locationFrame's* width. The tableViewFrame's width is the locationFrame's width minus the locationFrame's left and right frame margins (2*1 pixels).

The tableViewTrackingRect's origin.x is the *locationFrame's* origin.x plus the locationFrame's left frame margin width (1 pixel).


> I'm probably just being obtuse, but why "- 1" here?

Good question. The -1 was to nudge the mouse location's Y value up screen 1 pixel. I admit I don't exactly WHY, but it was necessary to select the correct row when the mouse "hotspot" transitions between between two rows. While CTRL+zooming my screen, I was comparing when the row selection changed for application menus and Camino location bar menu. Presumably there is an off-by-one difference between mouse tracking and row coordinates that I don't know about.


> I see that this is not what Safari does, but I think we should -deselectAll:
> when hovering over a header row.  Thoughts from others?

I agree. When the mouse is hovering over the header row, Safari keeps the previous location row selected. But it feels a little unnatural because application menus don't do that (when the mouse is hovering over a menu separator).

Comment 13

[Chris Peterson [:cpeterson]]

Attachment: autocomplete-v3.patch

autocomplete-v3.patch fixes Ilya's nits, removes row selection when hovering over a header row, and reverts some superfluous refactoring.

I'll leave r?=ishermandom.

Comment 14

[Ilya Sherman]

(In reply to comment #12)
> If I understand your question correctly, I think you may be confusing the
> tableViewFrame and locationFrame sizes.

Ah, yes, I absolutely was.  Thanks for pointing that out :)

> > I'm probably just being obtuse, but why "- 1" here?
> 
> Good question. The -1 was to nudge the mouse location's Y value up screen 1
> pixel. I admit I don't exactly WHY, but it was necessary to select the correct
> row when the mouse "hotspot" transitions between between two rows. While
> CTRL+zooming my screen, I was comparing when the row selection changed for
> application menus and Camino location bar menu. Presumably there is an
> off-by-one difference between mouse tracking and row coordinates that I don't
> know about.

I wonder if the off-by-one is coming from the |intercellSpacingHeight| below -- now that I understand what that calculation is doing, that piece looks out of place to me.  Have you checked that the first row is indeed offset by |intercellSpacingHeight| from the top of the table?

>+  tableViewTrackingRect.origin.y = locationFrame.origin.y - locationFrame.size.height - kVerticalPadding - intercellSpacingHeight;

Comment 15

[Chris Peterson [:cpeterson]]

Attachment: autocomplete-v4.patch

Good questions, Ilya. I now better understand what is happening.

Patch v4 removes the mysterious magic number -1 and calculates tableViewTrackingRect from the popup menu's dimensions. Since the popup menu frames the tableViewTrackingRect, it is a more sensible point of reference than the location textfield. <:)

One unanswered question: is there an off-by-one bug when calculating the table height using row heights and row intercellSpacing? Intuitively, I would imagine N rows have N-1 intercell spaces. I can't find a good answer, but if I assume N-1 intercell spaces, then layout math doesn't look right. And I've found some example code online that seems to assume N rows have N intercell spaces.

Maybe a Cocoa guru around here knows the real answer?

Comment 16

[Ilya Sherman]

Comment on attachment 507045 [details] [diff] [review]
autocomplete-v4.patch

>+  tableViewTrackingRect.origin.y = popupOrigin.y + popupViewMargin + kTableVerticalPadding + tableHeight;

nit: Could you add a comment explaining this calculation?  It's not obvious to me why "+ kTableVerticalPadding + tableHeight;" is included here -- is the frame of reference changing?


r=ilya with this nit addressed

Comment 17

[Ilya Sherman]

(In reply to comment #15)
> One unanswered question: is there an off-by-one bug when calculating the table
> height using row heights and row intercellSpacing? Intuitively, I would imagine
> N rows have N-1 intercell spaces. I can't find a good answer, but if I assume
> N-1 intercell spaces, then layout math doesn't look right. And I've found some
> example code online that seems to assume N rows have N intercell spaces.

I too thought that was rather odd, but didn't call it out as it was unrelated to this patch.  Glad I'm not the only one confused by that :)

Comment 18

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 507045 [details] [diff] [review]
autocomplete-v4.patch

I tested this for Ilya, and it works as expected :D

Comment 19

[Chris Peterson [:cpeterson]]

Attachment: autocomplete-v5.patch

Address Ilya's nits: clean up and comment |resizePopup| to better explain the calculation of table view dimensions relative to the popup window.

Comment 20

[philippe (part-time)]

The functionality of this patch (tested with autocomplete-v4.patch) is fine. However, I'm a bit uncomfortable with the design change it introduces. The opacity is change is fine, I think. However, the corner radius change feels wrong. I think it is too narrow now. Or too 'square'. And it is different from all the windows / menus / context menus etc on my system - in most apps I use (ok - Photoshop/Illustrator cs4 floating palettes have a 4px radius). 

(from comment 5)
> * Reduce autocomplete menu's corner radius to 2. I think this looks more
> "anchored" to the location bar and visually matches the corner radius of
> Camino's tabs (and Chrome's autocomplete menu corners).

In all the months I've used the current autocomplete window, it never felt un-anchored.

When I design web pages (or dead-wood, printed matter) I tend to use a larger radius for large / tall boxes than for a small box. For a small box, like a button on a webpage, 2~3 pixels radius is fine. For large box, I rather go with 4~5px - if the idea is to soften the sharpness of the edge / corner.

Caminos tabs are small boxes in the analogy above, whereas for most people, the autocomplete window will be a largish box. Most people have, I think, a rather large/long location bar — unless they have really lots of toolbar icons — and thus a largish autocomplete window.

screenshot of the (left) corners with this patch:
http://dev.l-c-n.com/camino/auto-complete.png

Comment 21

[Chris Peterson [:cpeterson]]

Attachment: autocomplete-v6.patch

That makes sense. Patch v6 incorporates Philippe's suggestion and reverts the corner radius from 2px to 5px (the original value).

Comment 22

[Stuart Morgan]

Comment on attachment 507801 [details] [diff] [review]
autocomplete-v6.patch

>+  // The popup window's origin is relative to the lower-left of the screen.
>+  // The popup window's height is popupViewMargin + kPopupVerticalPadding + 
>+  // tableHeight + kPopupVerticalPadding + popupViewMargin.
>+  NSPoint popupOrigin = [mPopupWin frame].origin;
>+  float popupViewMargin = [mPopupWin viewMargin];
>+
>+  // tableViewTrackingRect's coordinates are relative to upper-left of mTableView.
>+  // Locate mTableView's origin offset from the popup window's origin.
>+  NSPoint tableViewOrigin;
>+  tableViewOrigin.x = popupOrigin.x + popupViewMargin;
>+  tableViewOrigin.y = popupOrigin.y + popupViewMargin + kPopupVerticalPadding + tableHeight;
>+
>+  // Track the mouse over the TableView.
>+  NSRect tableViewTrackingRect;
>+  tableViewTrackingRect.size.width = tableViewFrame.size.width;
>+  tableViewTrackingRect.size.height = tableViewFrame.size.height;
>+  tableViewTrackingRect.origin = tableViewOrigin;

Don't do all this math yourself; this is what the NSWindow and NSView point/size/rect conversion calls are for. (Note that in addition to being more code, and more that can get out of sync over time, this is also wrong when there's a user scale factor applied.)

>+  [self startTrackingMouse:tableViewTrackingRect];

It's weird that the text field is doing this tracking rather than the table view itself. I think we should make a simple subclass for the autocomplete table--in fact, how about we make a subclass for the window, in a new file, and have the table subclass just in the implementation file for the window, as an implementation detail.

> - (void)closePopup
> {
>+  [self stopTrackingMouse];

I especially think we need a table subclass because this makes me super nervous. Right now this does seem to be called when the window is torn down (maybe due to blur; I didn't check), but relying on that seems extremely dangerous given that ordering of operations during window teardown isn't always stable across OS releases, and if we miss this step we get unpleasant crashes. I'd like to have a cleanup step in viewWillMoveToWindow:nil so there's no chance of us messing this up.

>+  // Mouse coordinates are relative to lower-left of the screen.
>+  NSPoint mouseLocation = [NSEvent mouseLocation];
>+
>+  // TableView coordinates are relative to upper-left of the popup window.
>+  NSPoint tableViewLocation;
>+  tableViewLocation.x = mouseLocation.x - mTrackingRect.origin.x;
>+  tableViewLocation.y = mTrackingRect.origin.y - mouseLocation.y;
>+
>+  // Is the mouse over the TableView?
>+  if (tableViewLocation.x < 0 || tableViewLocation.x >= mTrackingRect.size.width ||
>+      tableViewLocation.y < 0 || tableViewLocation.y >= mTrackingRect.size.height) {
>+    [mTableView deselectAll:self];
>+    return;
>+  }

Again, don't do this math yourself. Check [mouseEvent window], to make sure it's the autocomplete window (if not, it's outside) then use locationInWindow and NSView's point conversion methods.

Comment 23

[Chris Peterson [:cpeterson]]

Attachment: autocomplete-v7.patch

Patch v7 changes:
* Create new file AutoCompleteWindow.mm containing window-handling code split from AutoCompleteTextField.mm.
* Remove some unnecessary #imports.
* Remove diffs from official MAAttachedWindow.m.

Known issues:
* AutoCompleteWindow.mm's window rect calculations are still tightly coupled with AutoCompleteTextField. There is probably a better way to calculate the window rect. It's tricky because AutoCompleteWindow's views and parent windows are not available during awakeFromNib and change during openPopup/closePopup.
* AutoCompleteTextField.mm still contains code unrelated to textfields. It seems like an AutoCompleteController class might be useful for mediating between AutoCompleteTextField, AutoCompleteDataSource, and AutoCompleteWindow.

Comment 24

[Chris Peterson [:cpeterson]]

Attachment: autocomplete-v9.patch

Rebase patch on latest AutoCompleteTextField.mm changes.

Comment 25

[Stuart Morgan]

Comment on attachment 515854 [details] [diff] [review]
autocomplete-v9.patch

Almost there, just some minor comments. Thanks for doing this refactoring; as you say, the design still isn't perfect, but this is a big step in the right direction.

>--- a/src/browser/AutoCompleteTextField.mm	Sun Feb 27 23:52:52 2011 -0500
>+++ b/src/browser/AutoCompleteTextField.mm	Mon Feb 28 23:40:57 2011 -0800
...
>+#import "AutoCompleteWindow.h"

Remove, since it's already being included in the header.

>+  

Trailing whitespace (please check the whole patch for it, as there are some other instances.

>-  if (mSearchString)
>-    [mSearchString release];
>+  [mSearchString release];
>   mSearchString = [aString retain];

As long as you are touching this, it should be autorelease instead of release.

>+- (NSRect)calcWindowRect:(int)windowHeight

Words generally shouldn't be abbreviated in method names, plus 'calculate' implies that it doesn't return anything. How about desiredPopupWindowRect or targetPopupWindowRect.

>   if (mCompleteResult && mCompleteWhileTyping) {
>-    PRInt32 defaultRow = 0;
>+    PRInt32 defaultRow = 1;

0 should be correct at this point.

>     // show the popup
>-    [self openPopup];

Remove the comment too.

>+  // show the popup
>+  [mPopupWin openPopup:[self window]];

Here as well.

>+@protocol AutoCompleteWindowDelegate
>+- (NSRect)calcWindowRect:(int)windowHeight;
>+- (void)onResultClicked:(AutoCompleteResult*)clickedResult;
>+@end

The size calculation shouldn't be part of the delegate API; since the resize is always triggered by the client (either by an open, which is currently indirect, or via the resize call), the client should just pass a frame in. You'll need to have the client set the size explicitly before opening, but you can abstract that in a helper method in the caller.

>+@interface AutoCompleteWindow : MAAttachedWindow
>+{
>+@private
>+  id<AutoCompleteWindowDelegate>  mDelegate;
>+  AutoCompleteDataSource*         mDataSource;

Mark these as weak.

>+- (void)openPopup:(NSWindow*)parentWindow;
>+- (void)resizePopup:(BOOL)forceResize;
>+- (void)closePopup;

Remove 'Popup', since it's not necessary now that these are methods on the window class.

>+- (void)noteNumberOfRowsChanged;

Call this something like dataSourceChanged instead. (The way the data source is split between the two classes is unfortunate, but I think this refactoring is a big enough step for now, and we can revisit that later.)

>+#import "NSString+Utils.h"
>+#import "NSString+Gecko.h"
>+#import "NSTextView+Utils.h"
>+
>+#import "AutoCompleteCell.h"
>+#import "AutoCompleteResult.h"
>+#import "AutoCompleteTextField.h"
>+#import "AutoCompleteDataSource.h"
>+#import "AutoCompleteWindow.h"
>+#import "BrowserWindowController.h"
>+#import "LocationBarPartitionView.h"
>+#import "PageProxyIcon.h"
>+#import "PreferenceManager.h"
>+#import "CHBrowserService.h"
>+
>+#import "BookmarkManager.h"
>+#import "Bookmark.h"
>+
>+#include "nsIWebProgressListener.h"

I suspect that almost everything here except Autocomplete* is not necessary (and maybe not even all of those).

>+  if ([super initWithView:tableViewContainerView
>+      attachedToPoint:NSZeroPoint
>+               onSide:MAPositionBottom
>+           atDistance:kPopupWindowOffsetFromLocationField])]

Fix :-alignment.

This init style doesn't follow the Camino conventions, but I guess since we need the table view first that's unavoidable.

>+// handling the popup /////////////////////////////////

Use #pragma mark Popup Handling
(Similar with the later divider.)

>+  if (![self isOpen])
>+  {

Brace on same line.

>+  // Superview has extra vertical padding.
>+  tableViewFrame.size.height += kPopupVerticalPadding * 2;
>+  [[mTableView superview] setFrame:tableViewFrame];

Why is the variable called tableViewFrame if it's not setting the frame of the table view?

>+  // Convert coordinates.
>+  NSRect trackingRect = [mTableView bounds];
>+  trackingRect.origin = [mTableView convertPoint:trackingRect.origin toView:nil];
>+  trackingRect.origin = [[mTableView window] convertBaseToScreen:trackingRect.origin];
>+  
>+  [self startTrackingMouse:trackingRect];

Since mTableView is all you need to compute this, how about just moving it into startTrackingMouse.

Why the conversion to screen coordinates? addTrackingRect:... operates in the coordinate system of the receiver, as I recall, so [mTableView bounds] should be all you need.

>+- (void)closePopup
>+{
>+  [self stopTrackingMouse];

I'd feel better if you did this in an orderOut: override so it's impossible to not do it.

>+- (BOOL)isOpen
>+{
>+  return [self isVisible];
>+}

Why not just have clients use isVisible, and avoid the extra interface method?

>+  if ( aRow == -1 )
>+    [self deselectAll];

Remove spaces in (), and add braces since the else has them.

>+    [mTableView scrollRowToVisible:aRow];

You can remove this; it's cruft from when the suggestion list scrolled. We don't have any plans to bring that behavior back.

>+    row = numberOfRows-1;
>+  } else if (row == numberOfRows-1 && aRows == 1) {
...

While you are moving the whole block of code in selectRowBy, please clean up the style by adding spaces around the +/- operators, and changing inline comments to two spaces before instead of just one. 

>+  mTrackingRect = trackingRect;

Why is this a member variable? You never use it.

>diff -r 7221357ed20b src/extensions/MAAttachedWindow.m
>--- a/src/extensions/MAAttachedWindow.m	Sun Feb 27 23:52:52 2011 -0500
>+++ b/src/extensions/MAAttachedWindow.m	Mon Feb 28 23:40:57 2011 -0800

Thanks for unforking this file; I didn't even know we'd forked it!

Comment 26

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #25)
> Comment on attachment 515854 [details] [diff] [review] [review]
> autocomplete-v9.patch
> 
> Almost there, just some minor comments. Thanks for doing this refactoring;
> as you say, the design still isn't perfect, but this is a big step in the
> right direction.

Stuart, are your comments on Chris's last patch here something that can easily be fixed before we ship 2.1?

Comment 27

[Stuart Morgan]

Probably; paging this back in and making the remaining changes has been on my to-do list for a while.

Comment 28

[Mehmet]

Camino is no longer supported. Closing my old bug report.