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)

defect
Not set
normal

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)

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)
Marking nsbeta1 so that this is eventually fixed.
Keywords: nsbeta1, polish
QA Contact: esther → nbaca
adt: nsbeta1-
Keywords: nsbeta1nsbeta1-
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.
Product: Browser → Seamonkey
Assignee: sspitzer → mail
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.
QA Contact: nbaca → search
Assignee: mail → nobody
QA Contact: search → message-display
Depends on: 777287
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
Attached image screenshot of proposed look (obsolete) —
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)
(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?
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).
(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.
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.
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.
(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.
Can there be more bindings on an element? Or do I somehow need to use the <binding extends=> attribute?
Yes, you would make the ruleaction binding extend your new binding.
Attached patch WIP patch (obsolete) — Splinter Review
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)
Assignee: nobody → acelists
Blocks: 444793, 777287
Status: NEW → ASSIGNED
No longer depends on: 777287
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+
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)?
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.
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)
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
Attached patch patch v2 (obsolete) — Splinter Review
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 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 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+
Attached patch patch v3Splinter Review
Attachment #649316 - Attachment is obsolete: true
Attachment #651548 - Flags: review?(neil)
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 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
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.

Attachment

General

Creator:
Created:
Updated:
Size: