Closed
Bug 202036
Opened 21 years ago
Closed 12 years ago
Scroll a criteria list and a dark highlight appears (Filter Rules, Search Messages, Search Addresses)
Categories
(MailNews Core :: Filters, defect)
MailNews Core
Filters
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 17.0
People
(Reporter: nbaca, Assigned: aceman)
References
(Blocks 1 open bug)
Details
(Keywords: polish)
Attachments
(1 file, 3 obsolete files)
3.00 KB,
patch
|
neil
:
review+
mconley
:
review+
|
Details | Diff | Splinter Review |
Trunk build 2003-04-10: WinXP, Mac 10.1.5, Linux RH 8 Overview: Scroll a criteria list and one appears with a dark highlight. This happens in a variety of dialogs such as Filter Rules, Search Messages, Search Addresses. Steps to reproduce: 1. Open the Filter Rules 2. Place a cursor in the first text box on a criteria line and notice its background is a light background color. 3Add More criteria so a scrollbar appears 4. Scroll up Actual Results: Now the light background color has turned to a dark color. Now click inside the text box and the background color becomes light again. Click in the text box and now it is a light highlight. Expected Results: I would expect the background color to remain light before and after scrolling. Additional Information: 1. Summary of how each platform/themes behave: - Modern theme on all platforms: Problem occurs. - Classic theme on windows and linux: Problem occurs. - The problem does Not occur on the Mac using the Classic theme. 2. The problem also occurs in other dialogs (i.e. Search Messages, Search Addresses)
Reporter | ||
Comment 1•21 years ago
|
||
Marking nsbeta1 so that this is eventually fixed.
Comment 3•21 years ago
|
||
The dark/light highlight indicates whether the focus is on the box itself, or on some other field (including one of its contained fields); see bug 230904. I haven't found any other listboxes in Mozilla which contain individually focusable fields (altho there are several where the items are toggleable checkboxes). Bug 216212 points out that there are other issues with the listbox containing focusable fields -- in particular, tabbing amongst the fields within the box does not cause the box to scroll. One possible solution for these particular windows would be to tweak them such that the listbox itself does not get the focus, but the fields within it do. This would ensure that the highlight is always light-colored. Whether or not that is done, the program could be enhanced to set the selection of the listbox according to which contained field has (or most recently had) the focus. Additional code for keyboard navigation within the box -- in particular, for scrolling amongst the rules, which is an accessibility issue -- is needed, but that is bug 216212. CC'ing jglick, altho I don't know whether she has the time or interest for Mozilla bugs anymore.
Updated•20 years ago
|
Product: Browser → Seamonkey
Updated•19 years ago
|
Assignee: sspitzer → mail
Comment 4•16 years ago
|
||
Note that, in the current version of the filter dialog, the Actions list is now structured similarly to the Criteria list: a listbox with rows of controls. However, the focus-appearance for the Actions list is different: with TB3a1-1207, Classic (aka qute) theme, I don't see a per-row 'selection' highlight. When the focus is on the Actions list (as opposed to one of its child controls), there is no focus indication at all.
Updated•16 years ago
|
QA Contact: nbaca → search
Updated•15 years ago
|
Assignee: mail → nobody
QA Contact: search → message-display
The gray/blue listitem highlight could be removed e.g. by setting disabled=true on the listbox. The same could be done on the listbox in the action list. Yes, it causes the background of the whole list to become gray, but all the needed functions of the listbox seem to work fine (editable fields, tabbing, buttons). Setting disabled state probably disables all the special features of a listbox (e.g. the possibility to select a line) but that seems fine in this case as they are not wanted. In the actions list this would fix this error message appearing when doubleclicking an action line (between any fields): Error: TypeError: this.currentItem._fireEvent is not a function Source File: chrome://global/content/bindings/listbox.xml Line: 605 And it also makes the scrolling feel faster (tested on 100 terms)! This would also affect Thunderbird as the filter editor XUL is shared. What do you think?
Hardware: x86 → All
This is how it would look like on Win XP.
Attachment #649205 -
Flags: ui-review?(bwinton)
Attachment #649205 -
Flags: feedback?(kent)
Attachment #649205 -
Flags: feedback?(philip.chee)
Comment 7•12 years ago
|
||
(In reply to Mike Cowperthwaite from comment #3) > Additional code for keyboard navigation within the box -- in particular, for > scrolling amongst the rules, which is an accessibility issue -- is needed, > but that is bug 216212. Although currently you can of course tab to the listbox itself and use the arrow keys to scroll, which you wouldn't be able to do if it was disabled.
It seems you can still scroll using arrow keys once you click inside a textbox, but you do not see where you are as there is no highlight now. But wasn't that the point of this bug?
Comment 9•12 years ago
|
||
I tried using the arrow keys in a textbox but I couldn't get them to work reliably. I also tried the page up/page down keys, which were a bit better, but even then they sometimes strangely stopped working. Note that in the Filter Editor the "Match All Messages" radiobutton already attempts to disable the listbox and the other two enable it. Also the non-standard background colour of the listboxes looks odd to me (obviously bwinton has the final say here).
Assignee | ||
Comment 10•12 years ago
|
||
(In reply to neil@parkwaycc.co.uk from comment #9) > I tried using the arrow keys in a textbox but I couldn't get them to work > reliably. I also tried the page up/page down keys, which were a bit better, > but even then they sometimes strangely stopped working. Yes, I confirm that it acts unreliable. Like if you scroll one page away from the starting textbox the focus is lost and no more scrolling is possible with the keys. Will investigate. > Note that in the Filter Editor the "Match All Messages" radiobutton already > attempts to disable the listbox and the other two enable it. Yes, I noticed that. It would be easy to fix. > Also the non-standard background colour of the listboxes looks odd to me > (obviously bwinton has the final say here). It now looks like part of the dialog border (same colour) :) Of course, we could still have it disabled but override the colour in our stylesheet.
Assignee | ||
Comment 11•12 years ago
|
||
OK, there seems to be another way: setting -moz-binding: none on the search term listbox. Currently it is at standard listbox.xml. The action listbox/listitems already override the binding so do not have the highlight.
Assignee | ||
Comment 12•12 years ago
|
||
I mean #searchTermList > listitem { -moz-binding: none; } It now gets the same error when clicking between fields: Error: TypeError: this.currentItem._fireEvent is not a function Source File: chrome://global/content/bindings/listbox.xml Line: 605 But the highlight is away and the speed increase seem to be there too.
Comment 13•12 years ago
|
||
(In reply to aceman from comment #12) > I mean > #searchTermList > listitem { > -moz-binding: none; > } > > It now gets the same error when clicking between fields: > Error: TypeError: this.currentItem._fireEvent is not a function > Source File: chrome://global/content/bindings/listbox.xml > Line: 605 This is easily solved by providing a replacement binding that implements stub method(s) as necessary. Furthermore we can then use this binding as the base of the existing ruleaction listitem binding to fix that issue there too.
Assignee | ||
Comment 14•12 years ago
|
||
Can there be more bindings on an element? Or do I somehow need to use the <binding extends=> attribute?
Comment 15•12 years ago
|
||
Yes, you would make the ruleaction binding extend your new binding.
Assignee | ||
Comment 16•12 years ago
|
||
This seems to work (suppresses the highlight by changing to a binding that only implements a method that silences the error message). Would it be enough?
Attachment #649228 -
Flags: feedback?(neil)
Comment 17•12 years ago
|
||
Comment on attachment 649228 [details] [diff] [review] WIP patch (Untested) seems reasonable, although I would put the listitem binding before the ruleaction binding, and I don't think _fireEvent needs to return a value.
Attachment #649228 -
Flags: feedback?(neil) → feedback+
Assignee | ||
Comment 18•12 years ago
|
||
Thanks for your help, I'll produce a proper patch in several hours. If there is no code in the method, should I just declare it as <body></body> (no CDATA block)?
Comment 19•12 years ago
|
||
I seem to remember seeing a bug that <body/> (or <body></body>) didn't work properly, but I can't remember whether it's fixed, so you might find you have to put in a lone return; statement instead.
Assignee | ||
Comment 20•12 years ago
|
||
Comment on attachment 649205 [details]
screenshot of proposed look
Screenshot no longer relevant. The new solution looks differently UI-wise.
Attachment #649205 -
Attachment is obsolete: true
Attachment #649205 -
Flags: ui-review?(bwinton)
Attachment #649205 -
Flags: feedback?(philip.chee)
Attachment #649205 -
Flags: feedback?(kent)
Attachment #649228 -
Flags: feedback?(kent)
Assignee | ||
Comment 21•12 years ago
|
||
Being in the searchWidgets.xml, this also affects all the other search dialogs. I have tested Search messages and Search addresses too and it seems to work.
Component: MailNews: Message Display → Filters
Product: SeaMonkey → MailNews Core
Assignee | ||
Comment 22•12 years ago
|
||
Proper patch, also with Seamonkey version.
Attachment #649228 -
Attachment is obsolete: true
Attachment #649228 -
Flags: feedback?(kent)
Attachment #649316 -
Flags: ui-review?(bwinton)
Attachment #649316 -
Flags: review?(neil)
Comment 23•12 years ago
|
||
Comment on attachment 649316 [details] [diff] [review] patch v2 >+ <binding id="searchListitem"> I preferred the previous id="listitem" because it was more generic, given that "ruleaction" extends it... >+ <implementation> >+ <method name="_fireEvent"> >+ <parameter name="aName"/> >+ <body> >+ <!-- This binding exists solely to disable the default binding of a listitem >+ in the filter editor. And to provide a dummy _fireEvent function that >+ is called when a click occurs on the empty space of a listitem. >+ See bug 202036. --> [Looks odd to put a comment on the binding inside one of its methods. Maybe it would look less odd if you used JS comment syntax.] Nit: "solely" and "And" don't work well together ;-) Suggested wording: This binding exists to disable the default binding of a listitem in the search terms. It provides a dummy _fireEvent function that the listbox expects to be able to call. [It's not called by the click event, in case you're wondering.]
Attachment #649316 -
Flags: review?(neil) → review+
Comment 24•12 years ago
|
||
Comment on attachment 649316 [details] [diff] [review] patch v2 I can see why we would want the selected row to appear differently, but I agree that there doesn't seem to be a lot of sense in selecting a row in the filter editor. And since this seems to fix the problem, ui-r=me. Thanks, Blake.
Attachment #649316 -
Flags: ui-review?(bwinton) → ui-review+
Assignee | ||
Comment 25•12 years ago
|
||
Attachment #649316 -
Attachment is obsolete: true
Attachment #651548 -
Flags: review?(neil)
Comment 26•12 years ago
|
||
Comment on attachment 651548 [details] [diff] [review] patch v3 Sorry to keep you waiting.
Attachment #651548 -
Flags: review?(neil) → review+
Attachment #651548 -
Flags: review?(mconley)
Comment 27•12 years ago
|
||
Comment on attachment 651548 [details] [diff] [review] patch v3 Review of attachment 651548 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me, too. Thanks aceman!
Attachment #651548 -
Flags: review?(mconley) → review+
Keywords: checkin-needed
Comment 28•12 years ago
|
||
https://hg.mozilla.org/comm-central/rev/3003e8559792
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite-
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 17.0
You need to log in
before you can comment on or make changes to this bug.
Description
•