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)
Thunderbird
Mail Window Front End
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: mcow, Assigned: mscott)
References
Details
(Keywords: fixed1.8.1)
Attachments
(6 files, 2 obsolete files)
7.80 KB,
patch
|
Bienvenu
:
review+
mscott
:
superreview+
mscott
:
approval-branch-1.8.1+
|
Details | Diff | Splinter Review |
7.13 KB,
patch
|
Details | Diff | Splinter Review | |
7.34 KB,
patch
|
Details | Diff | Splinter Review | |
1017 bytes,
patch
|
Details | Diff | Splinter Review | |
4.20 KB,
patch
|
Bienvenu
:
review+
mscott
:
superreview+
|
Details | Diff | Splinter Review |
6.23 KB,
patch
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•19 years ago
|
||
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
Reporter | ||
Comment 2•19 years ago
|
||
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)
Reporter | ||
Comment 3•19 years ago
|
||
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)
Reporter | ||
Comment 4•18 years ago
|
||
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)
Reporter | ||
Comment 5•18 years ago
|
||
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)
Reporter | ||
Comment 6•18 years ago
|
||
Comment 7•18 years ago
|
||
OK, I'm going to try to apply this patch by hand and see how it works. Thx for the patch, Mike.
Comment 8•18 years ago
|
||
how does one use the keyboard to get to the quick search window, then? We need that for accessibility, I would think.
Assignee | ||
Comment 9•18 years ago
|
||
David: ctrl-K same as in firefox too.
Reporter | ||
Comment 10•18 years ago
|
||
Yes, Ctrl-K, or Alt+I plus Tab. (See also bug 259587.)
Reporter | ||
Comment 11•18 years ago
|
||
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 12•18 years ago
|
||
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+
Comment 13•18 years ago
|
||
Assignee | ||
Comment 14•18 years ago
|
||
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+
Reporter | ||
Comment 15•18 years ago
|
||
David, would you be so kind as to check this in?
Comment 16•18 years ago
|
||
Yes, I'll check it in when I get a chance...
Comment 17•18 years ago
|
||
fix checked into trunk and 1.8.1 branch. Thx, Mike.
Reporter | ||
Comment 18•18 years ago
|
||
(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.
Reporter | ||
Comment 19•18 years ago
|
||
(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.
Reporter | ||
Comment 20•18 years ago
|
||
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.
Reporter | ||
Comment 21•18 years ago
|
||
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 → ---
Comment 22•18 years ago
|
||
OK, Mike, I've checked in the fixes for my mis-application of your diffs. Sorry about that!
Reporter | ||
Comment 23•18 years ago
|
||
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.
Reporter | ||
Comment 24•18 years ago
|
||
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)
Comment 25•18 years ago
|
||
here's the cvs diff I have in my tree...
Updated•18 years ago
|
Attachment #212761 -
Flags: review?(bienvenu) → review+
Reporter | ||
Comment 26•18 years ago
|
||
Thanks, David. I looked over that CVS diff and it matches my non-CVS diff.
Assignee | ||
Updated•18 years ago
|
Attachment #212761 -
Flags: superreview?(mscott) → superreview+
Reporter | ||
Comment 27•18 years ago
|
||
Thanks, Scott. David, could I trouble you to check that last patch in?
Comment 28•18 years ago
|
||
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 ago → 18 years ago
Resolution: --- → FIXED
Comment 29•18 years ago
|
||
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.
Description
•