Last Comment Bug 202036 - Scroll a criteria list and a dark highlight appears (Filter Rules, Search Messages, Search Addresses)
: Scroll a criteria list and a dark highlight appears (Filter Rules, Search Mes...
Status: RESOLVED FIXED
: polish
Product: MailNews Core
Classification: Components
Component: Filters (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Thunderbird 17.0
Assigned To: :aceman
:
Mentors:
Depends on:
Blocks: 444793 777287
  Show dependency treegraph
 
Reported: 2003-04-14 16:44 PDT by Ninoschka Baca
Modified: 2012-08-27 11:17 PDT (History)
8 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
screenshot of proposed look (7.87 KB, image/png)
2012-08-06 00:47 PDT, :aceman
no flags Details
WIP patch (1.27 KB, patch)
2012-08-06 03:24 PDT, :aceman
neil: feedback+
Details | Diff | Splinter Review
patch v2 (3.02 KB, patch)
2012-08-06 10:36 PDT, :aceman
neil: review+
bwinton: ui‑review+
Details | Diff | Splinter Review
patch v3 (3.00 KB, patch)
2012-08-13 15:30 PDT, :aceman
neil: review+
mconley: review+
Details | Diff | Splinter Review

Description Ninoschka Baca 2003-04-14 16:44:54 PDT
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)
Comment 1 Ninoschka Baca 2003-04-14 16:46:08 PDT
Marking nsbeta1 so that this is eventually fixed.
Comment 2 Samir Gehani 2003-05-05 17:40:58 PDT
adt: nsbeta1-
Comment 3 Mike Cowperthwaite 2004-01-14 10:18:36 PST
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.
Comment 4 Mike Cowperthwaite 2008-01-09 08:07:17 PST
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.
Comment 5 :aceman 2012-08-06 00:07:52 PDT
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?
Comment 6 :aceman 2012-08-06 00:47:40 PDT
Created attachment 649205 [details]
screenshot of proposed look

This is how it would look like on Win XP.
Comment 7 neil@parkwaycc.co.uk 2012-08-06 01:11:02 PDT
(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.
Comment 8 :aceman 2012-08-06 01:20:20 PDT
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 neil@parkwaycc.co.uk 2012-08-06 01:30:45 PDT
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).
Comment 10 :aceman 2012-08-06 01:54:27 PDT
(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.
Comment 11 :aceman 2012-08-06 02:04:45 PDT
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.
Comment 12 :aceman 2012-08-06 02:15:37 PDT
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 neil@parkwaycc.co.uk 2012-08-06 02:26:28 PDT
(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.
Comment 14 :aceman 2012-08-06 02:59:33 PDT
Can there be more bindings on an element? Or do I somehow need to use the <binding extends=> attribute?
Comment 15 neil@parkwaycc.co.uk 2012-08-06 03:15:46 PDT
Yes, you would make the ruleaction binding extend your new binding.
Comment 16 :aceman 2012-08-06 03:24:56 PDT
Created attachment 649228 [details] [diff] [review]
WIP patch

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?
Comment 17 neil@parkwaycc.co.uk 2012-08-06 03:34:15 PDT
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.
Comment 18 :aceman 2012-08-06 03:42:56 PDT
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 neil@parkwaycc.co.uk 2012-08-06 03:50:37 PDT
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 20 :aceman 2012-08-06 04:33:02 PDT
Comment on attachment 649205 [details]
screenshot of proposed look

Screenshot no longer relevant. The new solution looks differently UI-wise.
Comment 21 :aceman 2012-08-06 05:26:47 PDT
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.
Comment 22 :aceman 2012-08-06 10:36:56 PDT
Created attachment 649316 [details] [diff] [review]
patch v2

Proper patch, also with Seamonkey version.
Comment 23 neil@parkwaycc.co.uk 2012-08-07 17:03:52 PDT
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.]
Comment 24 Blake Winton (:bwinton) (:☕️) 2012-08-08 11:37:08 PDT
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.
Comment 25 :aceman 2012-08-13 15:30:58 PDT
Created attachment 651548 [details] [diff] [review]
patch v3
Comment 26 neil@parkwaycc.co.uk 2012-08-17 13:09:25 PDT
Comment on attachment 651548 [details] [diff] [review]
patch v3

Sorry to keep you waiting.
Comment 27 Mike Conley (:mconley) - (needinfo me!) 2012-08-27 09:43:51 PDT
Comment on attachment 651548 [details] [diff] [review]
patch v3

Review of attachment 651548 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, too. Thanks aceman!
Comment 28 Ryan VanderMeulen [:RyanVM] 2012-08-27 11:17:59 PDT
https://hg.mozilla.org/comm-central/rev/3003e8559792

Note You need to log in before you can comment on or make changes to this bug.