Bookmark info panel does not allow a user to tab out of the description field

VERIFIED FIXED

Status

Camino Graveyard
Bookmarks
VERIFIED FIXED
13 years ago
11 years ago

People

(Reporter: Samuel Sidler (old account; do not CC), Assigned: Håkan Waara)

Tracking

({fixed1.8})

Details

Attachments

(1 attachment, 2 obsolete attachments)

STR:
1. Open the bookmark manager
2. Get info on any bookmark
3. Select the "Title" text field
4. Tab down into the description field
5. Attempt to tab (or shift tab) out of it

Actual Results:
Tabbing in the description field makes literal text "tabs". Shift-tabbing does nothing.

Expected results:
While I understand the desire users may have to tab in the description field, we should at least allow them to shift-tab out of it. Is this possible? Is there a way we can allow tabs in the description field and still allow tabs to be in content? Is it even worth having tabs in the content?
(Assignee)

Comment 1

13 years ago
Created attachment 205893 [details] [diff] [review]
proposed patch
Assignee: sfraser_bugs → hwaara
Status: NEW → ASSIGNED
(Assignee)

Comment 2

13 years ago
I'd like reviews on this patch, but don't know who are best to request so please just go ahead if you're interested.
(Reporter)

Updated

13 years ago
Attachment #205893 - Flags: superreview?(sfraser_bugs)
Attachment #205893 - Flags: review?(nick.kreeger)

Comment 3

13 years ago
Comment on attachment 205893 [details] [diff] [review]
proposed patch

Looks Ok, but there are a couple of things I noticed.

+-(BOOL)textView:(NSTextView*)textView doCommandBySelector:(SEL) command
+{
+  BOOL result = false;
+  
+  // we don't want tabbing to stop in the bookmark description textview
+  if (command == @selector(insertTab:)) {
+    [[self window] selectNextKeyView:[[self window]];
+    result = true;
+  }
+  return result;
+}

In Objective-C, use NO and YES instead of false and true.

Also rather than declaring a BOOL here, just return NO at the bottom of the method and return YES in the if statement.

[[self window] selectNextKeyView:[[self window]];

Here you have to many braces, should look like this:

[[self window] selectNextKeyView:[self window]];
Attachment #205893 - Flags: review?(nick.kreeger) → review-
(Assignee)

Comment 4

13 years ago
Created attachment 205948 [details] [diff] [review]
proposed patch v2

Updated patch with comments by Nick addressed.
Attachment #205893 - Attachment is obsolete: true
Attachment #205948 - Flags: superreview?
Attachment #205948 - Flags: review?(nick.kreeger)
Attachment #205893 - Flags: superreview?(sfraser_bugs)
(Assignee)

Updated

13 years ago
Attachment #205948 - Flags: superreview? → superreview?(sfraser_bugs)
Comment on attachment 205948 [details] [diff] [review]
proposed patch v2

please add a function-level comment for textView:doCommandBySelector: to explain why we're doing this.

also, is there no way to set the delegate in IB? Do we have to do it in code?

Comment 6

13 years ago
Yes, I agree with comment #5, there should be a way to set the next-key value for the field.
i wasn't talking about the nextKeyView, i was talking about the delegate.

Comment 8

13 years ago
Comment on attachment 205948 [details] [diff] [review]
proposed patch v2

The text view's delegate is already set to the controller in the nib, so remove the setDelegate: line (and test to make sure it still works).

The rest looks fine, though I'd probably just pass nil as the sender arg in selectNextKeyView:.
Attachment #205948 - Flags: superreview?(sfraser_bugs) → superreview+
(Assignee)

Comment 9

13 years ago
Unfortunately, neither delegate nor nextKeyView are available in IB because the bookmark description field is a NSScrollView.

Comment 10

13 years ago
(In reply to comment #9)
> Unfortunately, neither delegate nor nextKeyView are available in IB because the
> bookmark description field is a NSScrollView.

Sure they are (and are already set); you just have to click down until the NSTextView is selected.
(Assignee)

Comment 11

13 years ago
Ah yes I realized that in IB now -- still a newbie! :-)

I made the changes you suggested sfraser, works like a charm. Just lacking review+ for checkin.

Comment 12

13 years ago
Comment on attachment 205948 [details] [diff] [review]
proposed patch v2

Looks good to me, other than the needless setDelegate: (which you've already removed) and the funny inconsistent spacing in the method arguments.

r=me with those changed.
Attachment #205948 - Flags: review?(nick.kreeger) → review+
(Assignee)

Comment 13

13 years ago
Created attachment 206084 [details] [diff] [review]
final patch

Patch that was checked in
Attachment #205948 - Attachment is obsolete: true
(Assignee)

Comment 14

13 years ago
fix checked into trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
what about the branch?
landed on branch.
Keywords: fixed1.8

Comment 17

13 years ago
I fixed this to allow shift-tabbing out of the field too (selector insertBacktab:).

Comment 18

13 years ago
Like most good NSTextView's option-tab still allows user to enter a tab into the description field. (which is nice) dec 17 nightly. It might not be well known to users, but FYI.
this also needs to land on MOZILLA_1_8_0_BRANCH
(Reporter)

Updated

11 years ago
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.