Description field in BookmarksInfoPanel.nib has no focus ring when active

RESOLVED FIXED in Camino1.5

Status

Camino Graveyard
Bookmarks
--
minor
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Smokey Ardisson (offline for a while; not following bugs - do not email), Assigned: froodian (Ian Leue))

Tracking

({fixed1.8.1, polish})

unspecified
Camino1.5
PowerPC
Mac OS X
fixed1.8.1, polish
Dependency tree / graph

Details

Attachments

(2 attachments, 4 obsolete attachments)

5.77 KB, patch
Mike Pinkerton (not reading bugmail)
: superreview+
Details | Diff | Splinter Review
11.76 KB, application/zip
Details
The Description text field in the Bookmarks Info panel (in both bookmark view and folder view) has no focus ring when it is focused.

Putting this blocking bug 325880 because it's been suggested it might just be a nib change....

Comment 1

11 years ago
This behaves correctly; NSTextViews don't get focus rings.

If it really needs to be "fixed", it would have to be changed to a word-wrapping NSTextField, which would require code changes as well as nib changes. That would also remove the scroll bar though, which may not be desirable if people have long descriptions.
Isn't that what, say, Safari's textareas are?  Or is that a custom widget?

Otherwise, this sounds like a WONTFIX.

Comment 3

11 years ago
You can't make an NSTextView (or more precisely its NSScrollView) display a focus ring without hackery--try a Google search for "NSTextView focus ring".
(Assignee)

Comment 4

11 years ago
Something we discussed on IRC is the "make it an NSTextField instead" option.  This has the advantages of a) giving it the requested focus ring and b) making the font match the other fields.  The disadvantage would of course be losing the scroll-bar.  Personally, I can't imagine ever writing more than 3 or 4 lines of description, and if somebody really wanted to write a lot, they could still access all the text via keyboard, resizing the panel, or dragging over the text.  Simon, how do you feel about this idea?

Comment 5

11 years ago
Sure
(Assignee)

Comment 6

11 years ago
Created attachment 228060 [details]
New BookmarkInfoPanel.nib
Assignee: nobody → stridey
Status: NEW → ASSIGNED
Attachment #228060 - Flags: review?(alqahira)
(Assignee)

Comment 7

11 years ago
Created attachment 228061 [details] [diff] [review]
Uses NSTextField instead of NSTextView
Attachment #228061 - Flags: review?(stuart.morgan)
(Assignee)

Updated

11 years ago
Blocks: 319746
Comment on attachment 228060 [details]
New BookmarkInfoPanel.nib

r- on something here; with this nib and patch, 1) I can't loop around in the folder side from Description back to Title with FKA off, and 2) with FKA on, the checkboxes are not in the keyboard loop at all.

Both of these things worked fine before this nib/patch combo.
Attachment #228060 - Flags: review?(alqahira) → review-

Comment 9

11 years ago
Comment on attachment 228061 [details] [diff] [review]
Uses NSTextField instead of NSTextView

>       // beware that [NSText string] can return a pointer to (mutable) internal strorage
>-      [mBookmarkItem setItemDescription:[NSString stringWithString:[mBookmarkDescField string]]];
>+      [mBookmarkItem setItemDescription:[NSString stringWithString:[mBookmarkDescField stringValue]]];

Since there are no more NSTexts being sent |string| anymore, remove the comment.

Also, you are missing a necessary change from |string| to |stringValue|:

> else if ((changedField == mBookmarkDescField) || (changedField == mFolderDescField))
>      [mBookmarkItem setItemDescription:[NSString stringWithString:[changedField string]]];
Attachment #228061 - Flags: review?(stuart.morgan) → review-
(Assignee)

Comment 10

11 years ago
Created attachment 228896 [details] [diff] [review]
Patch
Attachment #228061 - Attachment is obsolete: true
Attachment #228896 - Flags: review?(stuart.morgan)
(Assignee)

Comment 11

11 years ago
Created attachment 228897 [details]
New BookmarkInfoPanel.nib
Attachment #228060 - Attachment is obsolete: true
Attachment #228897 - Flags: review?(alqahira)
Comment on attachment 228897 [details]
New BookmarkInfoPanel.nib

Two issues here:
1) This nib still hates me
2) Since we have to mess with it for this bug, perhaps we should standardize the position of the checkboxes in the folder view?  The left edge of the left one is 33px from the left edge of the text field, and the right edge of the right one is 25px from the right edge of the text field.  I'm not really sure what we might want to do (esp. wrt to l10n); I just noticed that they had different measurements.

r=me if you can get someone else on 10.3.9 to verify that the tab chain still works in the folder side (forward and backward, FKA on and off), with or without point 2.
(Assignee)

Updated

11 years ago
Blocks: 325845

Comment 13

11 years ago
Comment on attachment 228896 [details] [diff] [review]
Patch

Unfortunately, this bitrotted when the dock-menu changes landed. If you unbit-rot it though, r=me (I applied it manually, and it works fine).
Attachment #228896 - Flags: review?(stuart.morgan) → review-

Comment 14

11 years ago
Comment on attachment 228897 [details]
New BookmarkInfoPanel.nib

The nib has issues though.  I don't see the key loop stuff, but the text field needs to have the layout changed from scrollable to wrapping (with word-wrap) or tying a lot of text just gives you one really long line and a lot of empty space.
Attachment #228897 - Flags: review-
(Assignee)

Comment 15

11 years ago
Created attachment 232767 [details] [diff] [review]
r=smorgan patch

This is the bitrot police.
Attachment #228896 - Attachment is obsolete: true
Attachment #232767 - Flags: superreview?
(Assignee)

Comment 16

11 years ago
Created attachment 232768 [details]
New BookmarkInfoPanel.nib

This addressed smorgan's comments.

Smokey, as for the tabbing thing, would you be OK with landing this nib, on the condition that we back it out if other 10.3 users see the same issues you're seeing?  I'm having trouble finding someone who can/will build 10.3, and my debug builds are too damn big for me to upload anywhere for somebody to download and test...
Attachment #228897 - Attachment is obsolete: true
Attachment #232768 - Flags: review?(stuart.morgan)
Attachment #228897 - Flags: review?(alqahira)
That seems reasonable, I guess.
backing out nibs can suck rocks, esp if someone comes along and changes them. can you do a release build and upload that for other 10.3 testers before it gets landed?

Comment 19

11 years ago
> backing out nibs can suck rocks, esp if someone comes along and changes them.
> can you do a release build and upload that for other 10.3 testers before it
> gets landed?

If someone tells me what I'm supposed to test, I'm happy to build and test this.
Torben: apply the patch and add the nib
In the bookmark manager, select a bookmark folder and Get Info.

1) Make sure that you can tab and shift-tab all the way around through the textfields
2) Turn on full keyboard access in System Prefs: Keyboard & Mouse: Keyboard Shortcuts, and make sure that the tab chain also includes the two checkboxes (in both tab/shift tab direction)

You should check both 1 and 2 on regular bookmark, too, to be sure, but things work fine for me with regular bookmarks....

Comment 21

11 years ago
I just tested this and I see issues with tabbing :-( However, they're not identical to Smokey's comment 8 and there are issues with tabbing also without this patch (slightly less though):

Without the patch I can tab Name -> Description -> Name, the Keyword-field is ignored. Shift-tab from Description takes me (correctly) to Keyword, which I can't Shift-tab out of (tab takes me back to Description though). From Name Shift-tab takes me (correctly again) to Description. With FKA on the check-boxes are correctly in between Description and Name (tabbing and Shift-tabbing).

The only change with this patch is that I no longer can tab out of Description, Shift-tabbing takes me to Keyword (with the same problem as above). With FKA on I can Shift-tab from Name to the check-boxes (and from there to Description) but still not out of Description.

The Get Info-window for normal bookmarks works as expected in all instances.

Since the tab-loop is broken on 10.3 anyway, I suggest we take this patch and handle the rest in a new bug. 
Yeah, I filed a bug about tab skipping Keyword and shift-tab getting stuck in Keyword before, but everyone said I was on crack.

I really don't know what to do about that nib. :(
(Assignee)

Updated

11 years ago
Attachment #232768 - Flags: review?(stuart.morgan)
(Assignee)

Comment 23

11 years ago
Comment on attachment 232767 [details] [diff] [review]
r=smorgan patch

Pinkerton, are you ok with this?

The code changes out the scrolling NSTextView with an NSTextField for description fields.

The nib increases the minimum size.

However, it may have some weirdness in keyboard support (see comment 8 and comment 21) on 10.3.  However, it *already* has some weirdness on 10.3.  My proposal is basically that of torben in comment 21 - check this in, and file a followup bug for making the nib right on 10.3
Attachment #232767 - Flags: superreview? → superreview?(mikepinkerton)
Comment on attachment 232767 [details] [diff] [review]
r=smorgan patch

sr=pink
Attachment #232767 - Flags: superreview?(mikepinkerton) → superreview+
(Assignee)

Comment 25

11 years ago
Checked in on 1.8branch and trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.