Closed Bug 311239 Opened 19 years ago Closed 18 years ago

Fixes for F6/Ctrl+Tab navigation in 3-Pane

Categories

(Thunderbird :: Mail Window Front End, defect)

defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mcow, Assigned: mscott)

References

Details

(Keywords: fixed1.8.1)

Attachments

(6 files, 2 obsolete files)

Bug 203386 (suite) tweaked the handling of pane navigation in the 3-pane window, 
using F6 or Ctrl+Tab, so that the quicksearch field was not included in that 
navigation; Ctrl+tabbing would cycle from Folder Pane to Thread Pane to Message 
Pane and back to Folder Pane.

This bug is to port that patch to Thunderbird.
A side effect of that patch was to fix a problem where c-tabbing into the 
Message pane failed to clear the focusring from the pane that previously had the 
focus.  Altho TB is not displaying the focus ring for the panes, there is a 
similar issue when leaving the Thread pane: the 'current' indicator, which 
should only show when the tree has focus, continues to be displayed when 
c-tabbing from the Thread pane.  And for those of us who prefer the focus ring 
(see the CSS at bug 309696 comment 7), the patch will fix it so that is erased 
properly as well.

The original patch requires a supplemental fix to searchBar.js, to fix the same 
sort of problems that bug 272856 fixed in the suite after 203386's patch went 
in.
OS: Windows 2000 → All
Hardware: PC → All
This first part of patch is copied pretty much line-for-line from 203386's.
The second part is more complex than the patch at 272856, because TB's search
bar is more complex.  The GetSearchInput() calls are used to initialize
gSearchInput; I've removed two calls from further down the event chain and
backed them up so they're handled with the first events the search bar can get:
mouse down, or focus.  I've also had to add a null-check on gQuickSearchFocusEl
because gLastFocusedElement might be left null due to the changes in
FocusRingUpdate_Mail().
Attachment #198609 - Flags: review?(mscott)
Comment on attachment 198609 [details] [diff] [review]
diff-uw (non-CVS, against trunk 1003 nightly)

David, could I get a review from you on this?
Attachment #198609 - Flags: review?(mscott) → review?(bienvenu)
Attached patch diff -u (vs 1.6a1-0105 distro) (obsolete) — Splinter Review
I discovered there was a third event handler that required a call to GetSearchInput() -- onQuickSearchNewMsgLoaded().  Also, this diff has a few minor whitespace changes which I thought were worth preserving.
Attachment #198609 - Attachment is obsolete: true
Attachment #208871 - Flags: review?(bienvenu)
Attachment #198609 - Flags: review?(bienvenu)
Updated to fix bitrot from bug 324194.  This fix nicely complements that one, and I hope I can finally get some review action here -- after all, Scott, it 
was working on this that led me to point out that problem with 324193 
yesterday.

In addition to restoring functionality of previous patch, fixes a problem where the onSearchInputBlur handler relied on the gLastFocusedElement to be set when focus shifts from the QS field.  Also provided a few minor improvements to the quicksearch field event handling in general: removed a few superfluous lines of code, and combined the two near-identical methods ClearQSIfNecessary() and clearQuickSearchAfterFolderChange().
Attachment #208871 - Attachment is obsolete: true
Attachment #210639 - Flags: superreview?(mscott)
Attachment #210639 - Flags: review?(bienvenu)
Attachment #208871 - Flags: review?(bienvenu)
OK, I'm going to try to apply this patch by hand and see how it works. Thx for the patch, Mike.
how does one use the keyboard to get to the quick search window, then? We need that for accessibility, I would think.
David: ctrl-K
same as in firefox too.
Yes, Ctrl-K, or Alt+I plus Tab.  (See also bug 259587.)
I meant to add: you can also <tab> into the field from the message pane, or <shift-tab> from the folder pane.  Now that the view/QS fields are definitely *in* a toolbar, I don't think *that's* particularly good UI either; I don't think you can point to another program that allows tabbing from the application into a toolbar.  But this patch was to eliminate the fields from the F6-tabbing scheme, as the suite has had for a while.
Comment on attachment 210639 [details] [diff] [review]
diff -u vs. 1.6a1-0203

looks good, seems to work so far. I'll attach a cvs diff in a sec.
Attachment #210639 - Flags: review?(bienvenu) → review+
Attached patch cvs diffSplinter Review
Comment on attachment 210639 [details] [diff] [review]
diff -u vs. 1.6a1-0203

I agree, we shouldn't be tabbing through the search box now that it's an optional toolbar buton.

I'm worried about the searchBar.js changes as that code is always fragile. Only one way to find out though...
Attachment #210639 - Flags: superreview?(mscott)
Attachment #210639 - Flags: superreview+
Attachment #210639 - Flags: approval-branch-1.8.1+
David, would you be so kind as to check this in?
Yes, I'll check it in when I get a chance...
fix checked into trunk and 1.8.1 branch. Thx, Mike.
Status: NEW → RESOLVED
Closed: 18 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
(In reply to comment #14)
> I'm worried about the searchBar.js changes as that code is always fragile.
> Only one way to find out though...

Those changes fixed bug 261331 and bug 267765, btw.  The total code size 
for all that special handling has been reduced, including elimination of an
entire method.  The techniques used to clear the field have been reduced from three to two (no longer explicitly setting |gSearchInput.showingSearchCriteria| from without).  So, even if I broke something, it'll be a little easier to fix next time thru.
(In reply to comment #14)
> (From update of attachment 210639 [details] [diff] [review] [edit])
> I agree, we shouldn't be tabbing through the search box now that it's an
> optional toolbar buton.

In userChrome.css, I tried setting { -moz-user-focus: none; } for the field items in the toolbar.  Selecting on #viewPicker works to keep the focus off the View dropdown, but the QS field is harder -- altho I can get that style set on #searchInput and .quick-search-textbox, the actual focus-grabbing widget is the
.textbox-input item within, and I can't construct a selector that gets to that 
-- whether I use ".textbox-input" or "html|.textbox-input".  This may need to 
be done programmatically.
David, I'm sure this is my fault for not using CVS in the first place, but 
there are several parts of my original patch that are missing or mis-transcribed, and the behavior is subsequently broken.  There's also a bit of code that I had intended to include but failed to, in my own patch.

The fix here is for the missing changes from searchBar.js.  The first added 
line in onSearchInputMouseDown() was in the original patch, but the rest of 
that method's changes I'd left out originally.  The negated sense in onSearchInputBlur() was in the original patch.  The comment in the last change [in onClearSearch()] was also in my original patch, and I think it's a useful comment.
Scott, you should note this:

There was also a change in my patch to mail3PaneWindowCommands.js where I had a tweak in SearchBarToggled().  That also didn't get included in David's patch.
But on further testing, I've discovered that SearchBarToggled() is never 
called, presumably because the SearchBar no longer exists.  

That code un-applied any MailView/QS filtering on the view when the SearchBar was hidden.  Currently, when the Mail Toolbar is hidden, or when the customization of the toolbar removes an active QS or MailView, the filtering is maintained, when it probably should be cleared.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
OK, Mike, I've checked in the fixes for my mis-application of your diffs. Sorry about that!
Thanks, David.

Now I have to submit another patch to fix a regression.  The problem is seen 
if you shift the focus to the field using the keyboard, then tab out -- the field wasn't being reset to show the search criteria.

After fixing this, I discovered two other cases where onSearchInputBlur() was getting called where I wouldn't have expected it, so I needed to add additional code (marked ##HACK##) to work around these.  Also, one of those HACKs required an additional fix to search.xml, a method returning the wrong attribute.

This patch also restores a couple of lines in |onSearchInputBlur()| to what 
they were before my original patch.  I think my patched version works just 
as well, but having looked more in-depth at the differences between 
|setSearchCriteriaText()| and |showingSearchCriteria|, I think that change is better left to some future rewrite.

I removed the mouse-down call to |setSelectionRange()| because it was superfluous.

And finally, two lines in the file had trailing spaces stripped off of them; that wasn't deliberate on my part, just a side-effect of a function in my editor; I figured these were OK.
Comment on attachment 212761 [details] [diff] [review]
diff -u   vs.  trunk distro (0222 nightly)

Whoops, forgot to set review flags.
Attachment #212761 - Flags: superreview?(mscott)
Attachment #212761 - Flags: review?(bienvenu)
Attached patch cvs diffSplinter Review
here's the cvs diff I have in my tree...
Attachment #212761 - Flags: review?(bienvenu) → review+
Thanks, David.  I looked over that CVS diff and it matches my non-CVS diff.
Attachment #212761 - Flags: superreview?(mscott) → superreview+
Thanks, Scott.  David, could I trouble you to check that last patch in?
fixed on trunk; have to work a little to get it fixed on the 1.8.1 branch since the diffs don't apply cleanly there...
Status: REOPENED → RESOLVED
Closed: 18 years ago18 years ago
Resolution: --- → FIXED
ok, fixes checked into 1.8.1 branch as well.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: