ISHIKAWA, Chiaki Reporter
	Description • 15 years ago

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; ja; rv:1.9.2) Gecko/20100115 Firefox/3.6 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; ja; rv:1.9.1.8) Gecko/20100216 Thunderbird/3.0.2

I am not exactly sure when this happens, but sometimes
after choosing the rule in filter rule list doesn't give the
chosen rule the full dark highlight. Instead only a modest dark frame is shown
and so the user is not quite sure if the rule is chosen or not.
(I think the mixing of hitting Down/Up buttons and choosing arule by mouse 
results in this behavior.)

It seems that the rule IS chosen, but this erratic display behavior
is not quite convincing and quite CONFUSING.

I will upload the screen image(s) when this happens.


Reproducible: Sometimes

Steps to Reproduce:
1. (Not sure when this happens, but hitting [Down]/[Up] button and
   choosing a rule by mouse seems to trigger this behavior.]
2. Choosing a rule. Usually the rule is shown in dark highlighted background.
3.
Actual Results:  
Sometimes, but not always, the chosen rule is surrounded in a thick bold rectangle, but not in the highlighted background.

Expected Results:  
The chosen rule should appear in a dark highlighted background to shows the
chosen status.

There are two bugs that mention similar problems:
Bug 202036  : Scroll a criteria list and a dark highlight appears (Filter Rules, Search Messages, Search Addresses)

Bug 476535 : Initial highlight doesn't work for filtered lists in Download manager

 (But I am not sure if that these bugs are talking about the same thing. Superficially they seem to talk about similar problems.)

	ISHIKAWA, Chiaki Reporter
	Comment 1 • 15 years ago

(I forgot to mention that I am using Japanese locale, and so the
buttons and such contains Japanese  strings.)

The zip file contains three png files.
x1.png
x2.png
x3.png

Note that the rule name INDIA is not highlighted with dark background when chosen. Only the rectangle border is highlighted in bold lines. (x2.png)

The rules above and below it are highlighted with dark background. (x1.png, x2.png)

This happens after I tried to move down the rule, INDIA, by hitting [Down] button several times and it went across the bottom border and the 
repositioning of the list display occurs. Once this strange state occurs
the particular rule can't be highlighted. I have to clear the display by closing the filter menu by clicking [x] button in the upper-right corner and
restarts this filter menu afresh by tools -> filters.
(In the picture, 
Down button is the one with "...(D)" in the right-most column, the
sixth button from the top.) 

Now come to think of it, because the symptom appears after the chosen rule
goes across the display boundary at the bottom, this bug may have something
to do with Bug 549577

	ISHIKAWA, Chiaki Reporter
	Comment 2 • 15 years ago

I am re-submitting since the data file (.zip) didn't seem to get the
data type correctly when I chose auto-detect for the content type.
429732: PNG files that shows the different highlighting status of the rule.

	Nathan Tuggy (:tuggyne)
	Comment 3 • 15 years ago

I suggest you attach each PNG file separately, rather than all together inside a ZIP; lack of disk space on Bugzilla is not an issue that I am aware of, while convenience is.

In the ZIP file I am unable to see the bug.

Also, is this reproducible with the default theme?

	ISHIKAWA, Chiaki Reporter
	Comment 4 • 15 years ago

Sorry, I must have screwed up something. This new set of files (three of them)
will show what the failure of high-lighting I was talking about.

The first file: y1.png ... A filter rule is "highlighted" in blue background.

	ISHIKAWA, Chiaki Reporter
	Comment 5 • 15 years ago

Another filter rule after I move down the focus one line is no longer 
shown in BLUE BACKGROUND. Instead only the boundary is shown in 
somewhat bolder lines, but it is hardly called "highlighting" compared to
the previous figure y1.png and the following y3.png.

This subtle bolder border is hardly noticeable on a notebook LCD under
bad lighting.

Something is wrong. As I mentioned privously, this rule 7F-ML was moved down the
list using "Down" until it went down the lower bottom at which time the replacement of the whole list happens and at that instant, the highlighting of
this rule is no longer working. I moved this up again to this position to the list, but it is still not high-lighted in a proper manner.

	ISHIKAWA, Chiaki Reporter
	Comment 6 • 15 years ago

Another rule just below the problematic rule is properly highlighted.
Something is wrong with the scrolling and highlighting code.

This is on another computer where the original bug was reported.

Mozilla/5.0 (X11; U; Linux i686; ja; rv:1.9.1.8) Gecko/20100227 Thunderbird/3.0.3

	ISHIKAWA, Chiaki Reporter
	Comment 7 • 15 years ago

Hi,

I hope the three files, y1.png, y2.png, and y3.png show the problematic
highlighting behavior. The rule 7F-ML doesn't get highlighted properly once
it was scrolled down the screen and moved up again. (In y2.png, it is shown
inside a slightly dark blue rectangle, but this is NOT the highlighting
to which users are now accustomed. It is hard to see the rule is chosen/highlighted on a note book PC under bad lighting. The white background is very wrong IMHO.)

As far as I can tell, I am using default theme.

Tools -> addon -> them shows only one "default 3.6" entry.

TIA

	ISHIKAWA, Chiaki Reporter
	Comment 8 • 15 years ago

In comment 6, I said

>This is on another computer where the original bug was reported.

I meant to say "This is on another computer, different from the PC where
the original bug was reported".
(Peril of using a foreign language to report a technical problem.)

So this is not tied to a particular PC. I can recreate this bug on two different PCs now.

TIA.

	ISHIKAWA, Chiaki Reporter
	Comment 9 • 15 years ago

I just noticed that somehow my initial post set the platform
to windows, but this is about "linux" version.

	Nathan Tuggy (:tuggyne)
	Comment 10 • 15 years ago

(In reply to comment #9)
> I just noticed that somehow my initial post set the platform
> to windows, but this is about "linux" version.

You were probably posting using Windows Firefox; in any case, TB's platform correctly shows as Linux. No problem.

So I gather the problem is that the items are only getting a focus rectangle, not the usual highlight + focus rectangle? This is more commonly associated with multiple-select list boxes (which this one actually is) and using the Ctrl+Arrow keys to select multiple items. I'm not sure why this would be showing up under the circumstances, though.
I can't reproduce it on Windows; perhaps I don't have enough filters (although there's more than one pageful). Or perhaps this is a Linux-only bug, which would be odd.

	ISHIKAWA, Chiaki Reporter
	Comment 11 • 15 years ago

(In reply to comment #10)
> (In reply to comment #9)
> > I just noticed that somehow my initial post set the platform
> > to windows, but this is about "linux" version.
> 
> You were probably posting using Windows Firefox; in any case, TB's platform
> correctly shows as Linux. No problem.

OK, thank you for the confirmation.

> So I gather the problem is that the items are only getting a focus rectangle,
> not the usual highlight + focus rectangle? 

Exactly!  This confuses a user pretty much. Since the highlight is absent,
one is not so sure of whether the rule is chosen or not indeed.

 
> This is more commonly associated
> with multiple-select list boxes (which this one actually is) and using the
> Ctrl+Arrow keys to select multiple items. I'm not sure why this would be
> showing up under the circumstances, though.
> I can't reproduce it on Windows; perhaps I don't have enough filters (although
> there's more than one pageful). Or perhaps this is a Linux-only bug, which
> would be odd.

I am not sure of these myself. I am unfamilar with multiple-selection to begin with.

Windows vs Linux might be for real. One can never tell if the underlying GUI library or graphics primitive have the same functionality.

In any case, I didn't have this problem in TB2 series, I think and 
hope this confusing behavior is fixed.

TIA

	ISHIKAWA, Chiaki Reporter
	Comment 12 • 14 years ago

(In reply to comment #11)

> > I can't reproduce it on Windows; perhaps I don't have enough filters (although
> > there's more than one pageful). Or perhaps this is a Linux-only bug, which
> > would be odd.
> 
> I am not sure of these myself. I am unfamilar with multiple-selection to begin
> with.
> 
> Windows vs Linux might be for real. One can never tell if the underlying GUI
> library or graphics primitive have the same functionality.


I installed TB 3.1.1 under Windows XP SP3 and found the problem is also there.
So if someone has enough number of rules (I suspect one must have at least two screenful or something.), then the problem manifests under Windows, too.

Also, under Windows I noticded something which may be also true under linux.
Once a highlighting doesn't occur on a filter (and high-lighting does occur on the rule above and below), using arrow keys to scroll the listing up and down and returning to the original rule still doesn't hightlight it. It seems this "not highlight a rule" property seems to stick to the particular rule.

Also the nefarious VERY SLOW visual update when we shift a filter up and down by move up and move down button is also seen under windows.
Bug 518305  
http://getsatisfaction.com/mozilla_messaging/topics/reordering_message_filter_is_slow

	:aceman Assignee
	Comment 13 • 13 years ago

Is this bug 642810? Can you please confirm?

Or do you get the problem NOT after editing the filter?

	:aceman Assignee
	Comment 14 • 13 years ago

Actually I found another case: Move a filter down (repeatedly pressing Down button) to the end of the list. It loses the background when it is the last or next to last item in the list. About 10 filters were enough to see this.

	ISHIKAWA, Chiaki Reporter
	Comment 15 • 13 years ago

(In reply to :aceman (away 27.-2.) from comment #13)
> Is this bug 642810? Can you please confirm?
> 
> Or do you get the problem NOT after editing the filter?

More likely I obtain the problematic behavior AFTER editing, so probably what I experienced is succinctly reproduced by 642810 !

As for your comment 14, I am not sure if this were the case with me.
My filter list is so long that I had to scroll down the list and by the time the end of the list comes into view, and my cursor reaches the end, the item *IS* high-lighted. Maybe comment 14 case happens only when the filter is short enough to be visible in the initial window.

Let's home someone can fix the easy to reproduce bug 642810 case!

	ISHIKAWA, Chiaki Reporter
	Comment 16 • 13 years ago

It's funny that we find buggy reproducible cases when we focus on trying to check the status of a different bug.

While I was trying to check the seemingly fixed Bug 549577,
it suddenly occurred to me why didn't I see this missing high-lighting there. Well, actually I was seeing it, but simply didn't notice it because I was focusing on the speed of scrolling.

I can reproduce the loss of high-lighting in the following  manner with TB 7.0.1  under Windows 7 64 bits.
But I think you need to have a rather longish list of message filters that won't fit into the initial list window.
(The following modifies the position of the initial entry in the list, so be warned.)

When you  open the message filter list,
the top-most entry is high-lighted.
by "move down", I lower the position of this entry in the whole list by a slot.
Hit "move down" many times, and then the selected filter goes to near the bottom of the screen as it is lowered in the list.

Here is a twist. I move down the entry to the second to the last position in the visible window. 
I move it down another slot thinking that it will be the
last entry in the window. It will not. 

Somehow, the scrolling occurs and the whole list is shifted up by one row. Thus the
first entry in the initially non-visible portion of the filter list becomes the LAST entry in the visible window, and the selected item is still the second to the last entry in the window. 

(The loss of high-lighting mentioned below occurs when the selected windows go to the position of this newly visible entry.)

Now,  the next "move down", the filter suddenly loses high-lighting.
Further moving down doesn't restore the high-lighting.

But here is the kicker. Hit "move up" a few times so that
the selected item comes back to the position where it lost high-lighting. It is still not high-lighted. But another
"move up" to the position where it still was highlighted.
Voila, it is now high-lighted again.
You can move down and up the selected item around the position.
It is as if there is some kind of a wall there that takes the high-lighting off and puts back on!

I hope this behavior gives a clue to the cause of the bug.

	:aceman Assignee
	Comment 17 • 13 years ago

That is what I described in comment 14.
But I do not understand the strange scrolling you describe (maybe some screen video would be helpful). Is this scrolling actually bug 549577?

So let's keep this bug for the bug when the highlighting is lost when moving the item down to the (second to) last position in the list.
I am confirming that on Win XP and Linux, TB10.

	:aceman Assignee
	Comment 18 • 12 years ago

Can anybody still see this in TB18? I can't now.

	ISHIKAWA, Chiaki Reporter
	Comment 19 • 12 years ago

Hi,

indeed, on linux, I do not see it any more!

I have no idea what change fixed the problem, but it is not there.

TIA

	:aceman Assignee
	Comment 20 • 12 years ago

I suspect it could have been fixed by bug 780473 where the list rebuilding was reworked (to not delete and create all the items anew) to that the XBL bindings work better now. But let's wait some time to get more confirmations.

	ISHIKAWA, Chiaki Reporter
	Comment 21 • 12 years ago

(In reply to :aceman from comment #20)
> I suspect it could have been fixed by bug 780473 where the list rebuilding
> was reworked (to not delete and create all the items anew) to that the XBL
> bindings work better now. But let's wait some time to get more confirmations.

I thought this was indeed fixed in the *current* comm-central tree.
(18.0.1 which I checked out about a week ago.)

Well, after creating a longish list of filters about a 15 filters and tinkering.
I could create a situation where a chosen filter (with clear blue-ish  background highlighting)
was moved up and down using MOVE UP and MOVE DOWN button,
and was NOT high-lighted properly only when it came to the 
third row position from the bottom of the visible filter window.
When I move it up and down, it is again properly high-lighted.

Very strange.

I thought I would capture the image by moving the message filter window in a clean background,
and resizing the message filter window (for testing with a short list of filters,
I had shrunk the vertical window size a little. Then gradually I added filters.)
BUT, then the symptom disappeared!
So I could not capture the image :-(

The condition to let the bug appear seems be very flakey.

Also, it seems to me as if there is some kind of display optimization code to
disable unnecessary repaint, but that the optimization code forgot about this
highlighting. But it is a wild guess.

So, it is not quite 100% eliminated yet :-(

TIA
(FYI, the current public 15.0.1 still clearly has this problem, and I can easily
re-create the problem with my current setup.)

	:aceman Assignee
	Comment 22 • 12 years ago

(In reply to ISHIKAWA, chiaki from comment #21)
> The condition to let the bug appear seems be very flakey.
> Also, it seems to me as if there is some kind of display optimization code to
> disable unnecessary repaint, but that the optimization code forgot about this
> highlighting. But it is a wild guess.
> 
This is exactly what Neil says. It is some XBL optimization that it not completely initializes/repaints items that are out of view. Then selecting them has problems. So the fix is to first generate the list, repaint and then select them (e.g. setTimeout(selectItem, 0) seems enough).

So I'll add this workaround as another level of defense :)

> (FYI, the current public 15.0.1 still clearly has this problem, and I can
> easily
> re-create the problem with my current setup.)

Yes, the problem may be fixed only in 18nightly, not sooner.

	:aceman Assignee
	Comment 23 • 12 years ago

Ishikawa, bwinton, can you try this out? Steps to reproduce the bug are in comment 21. In my testing I had 10000 filters and when moving the last one up by 1 I always found a position (about 20 times up) where the highlight got lost.

	:aceman Assignee
	Comment 24 • 12 years ago

The patch uses the solution from bug 736661.

	ISHIKAWA, Chiaki Reporter
	Comment 25 • 12 years ago

:aceman,
my main work environment is on Linux (32 bits).

But I am also using TB under Windows7 on a PC in non-work environment.
Without adding the patch, I tested. [Wow, I just realied TB was still 16.x version. I just touched the upgrade button shown by Help -> About to upgrade it to 17.0, and then re-tested.]

I count 198 filters.
 
Now, under 16.x, the testing result was hilarious.
I think it was worse in the Windows version.

I scrolled down to the bottom of the list (the last filter in the whole list), and the list is shown with the last filter at the bottom.
I select the last filter. It is shown in a high-lighted manner.
But as soon as I move it up by MOVE UP, it loses the high-lighting of the whole rectangle, and ONLY the FRAME is shown in a bluish tint to show that it is
somewhat different from the rest. Strange.

But what is really strange is this. As I moved this selected item one by one using MOVE UP button, it is in this frame-only highlighting state UNTIL I come t the 3rd place from the top. (Or was it 4th? Now that I upgraded to 17.0 and it doesn't happen, it is not a big deal.), it suddenly REGAIONS the full high-lighting again! (That is, the whole area of rectangle is shown in a shade to show that it is a selected item.) 

Moving UP again until it hits the upper edge won't lose this high-lighting. But again, if I move it down using MOVE DOWN, it loses he full high-lighting status when it moves past this 3rd row from the top position, and never regains the full high-lighting status even if it goes down to the bottom. It remains in the frame-only high-lighting state.
Unless I close the message filter menu window, and re-opens it, I can not show it
in fully high-lighted manner (unless I move it up again using MOVE UP many times to reach the 3rd position from the top or something. 

Spooky. 
It was TB 16.x

With 17.0 under Win7 (64bits) I don't see any such strange behavior easily.

Your mention of losing the high-light after moving it up 20 times sounds about the opposite of my previous experience under 16.x, doesn't it?
[Due to my screen size, 26 filters are shown in the list of the filter window in my case.]

Hmm, could it be that the problem is fixed in v17 for Win7?
I will try to see if this problem is still there in the TB under linux.
I browsed the discussion in 736661, and could not understand why simple list processing can not be implemented simply but there must be some baggage on it due to historical reasons :-(

TIA

	:aceman Assignee
	Comment 26 • 12 years ago

(In reply to ISHIKAWA, chiaki from comment #25)
> With 17.0 under Win7 (64bits) I don't see any such strange behavior easily.
> 
> Your mention of losing the high-light after moving it up 20 times sounds
> about the opposite of my previous experience under 16.x, doesn't it?
No, it is mostly the same what you say. I had to move up by 20 positions, you had to move up by 195 ;) The number may be different. May depend on dialog size (how many filters fit on the screen) and such.

> Hmm, could it be that the problem is fixed in v17 for Win7?
It should be better in TB17 but it is not 100% fixed. I could still see it in 17 when I tried hard. I can't with the patch.

> I will try to see if this problem is still there in the TB under linux.
> I browsed the discussion in 736661, and could not understand why simple list
> processing can not be implemented simply but there must be some baggage on
> it due to historical reasons :-(
Looks like some unsafe optimization.

	ISHIKAWA, Chiaki Reporter
	Comment 27 • 12 years ago

Sorry for my late reply. Day job kept me busy :-)

Anyway, with v17 under linux (32bits), I don't see any such strange behavior easily again.
I have lots of filters, but could not reproduce the problem easily as on the older platform.

I have a question. Where should I place the patched FilterListDialog.js?

I know a copy is in a jar file for v17, i.e.,

~/thunderbird/omni.ja (~/thunderbird is my private install directory)

  -rw-rw-r--     29964   1-Jan-2010  00:00:00  chrome/messenger/content/messenger/FilterListDialog.js

(My unzip scheme printed the above timestamp for the file, which looks a little strange.
Actually, I used emacs dir-mode which invokes some unzip magic to list the files inside.)

Should I replace the copy of FilterListDialog.js in the omni.ja and test it? (I suppose so, but I just wanted to check.)

As I mentioned above, I could not easily reproduce the problematic case now in v17, but
at least I can probably say the patched FilterListDialog.js does not seem to introduce new bugs, etc. after some testing.

Then I realized a problem.
My source file copies are from comm-central, and I think the files are really for the future version (I am not quite following the version nomenclature very well).

I compared the FilterListDialog.js in the distributed v17 copy, and comm-central version.
They are very different(!), and patching failed miserably for v17 copy. The patch worked perfectly for comm-central version.

Do you have a customized new FilterListDialog.js for v17?
Or can I simply insert the patched comm-central version into v17 omni.ja and expect it to work?

	:aceman Assignee
	Comment 28 • 12 years ago

Yeah, the FilterListDialog.js in TB20 differs from TB17. (See
http://hg.mozilla.org/comm-central/log/a51b0d9414fd/mail/base/content/FilterListDialog.js)

So if the patch does not apply cleanly to the version from TB17 you can try downloading the TB20 file from http://hg.mozilla.org/comm-central/file/a51b0d9414fd/mail/base/content/FilterListDialog.js and apply the patch. Then put the file into omni.ja in the TB install folder. It is still not guaranteed to work as other files from TB17 in the archive may be out of sync with this new file. But you can try it.

I do not have a version of the patch for TB17, but I could make one. But you must first find reproducible steps to see the problem without the patch.

	ISHIKAWA, Chiaki Reporter
	Comment 29 • 12 years ago

(In reply to :aceman from comment #28)
> I do not have a version of the patch for TB17, but I could make one. But you
> must first find reproducible steps to see the problem without the patch.

Under linux (32bit) v17, I could not find any way to show the problematic loss
of highlighting,
(somehow linux version is more robust in this regard)
but for windows version 17.0 (windows 7 64bits), I could reliably
reproduce the problem now.
Open the filter manager window.
Choose the topmost entry in the filter list.
Move it down by [Down] TWENTY times (in my case),
and then move it to THE END OF THE LIST using
the [move to the end] button (I am using Japanese locale, so
I am not sure what the English title of this message is.)

Anyway, by this procedure, it works on my set up today.

(Move down by 20 times, must be "20" times. If this is 19, the symptom does not appear!!!
Such a subtle condition to reproduce the bug. Sigh...)

Anyway this method to show the problem works reliably today, and so I am now trying to replace the omni.ja
file with the FilterListDialog.js after applying the patch, and
 - see if the modified omni.ja works with v17 thunderbird, and  
 - see if the problematic symptom is gone.
Stay tuned.

	ISHIKAWA, Chiaki Reporter
	Comment 30 • 12 years ago

I am afraid that placing FilterListDialog.js (from comm-central)
after the patch in omni.ja of Windows TB install dir did not fix the
problem I observed.  The filter when moved to the last entry suddenly
loses high-lighting. Tested under windows7 64bits with TB 17.0.

Of course, we didn't know if this is supposed to work, and
what we want to do now is to check
if the problem is fixed in the future TB (correct?),

So I think I should check the operation of TB from comm-central.
(However, I am building TB under linux, and it seems the later version of TB
under linux seems to be very robust regarding this bug and it is hard to generate
the problematic symptom under linux as opposed to under linux...)
Also, on this test PC, I have not created a very longish list of
filter rules yet. But I will try to see if I can reliably
re-create the problem under my comm-central version of TB under linux.
Then I can test your patch to see if it fixes the problem.

By the way, I noticed that there is a strange error message in the
error console during testing.  This message persists even if I
reverted the omni.ja to the original v17 version, and so I am afraid
that there is something wrong with the official omni.ja?

*CAUTION* The following messages in the error console was printed
using Japanese equivalent of DateTime, Error, Source File, and Line.
So the English version of this message may use slightly different word
or phrase for the legend.

DateTime: 2012/12/20 23:56:29
Error: ReferenceError: officialSearchBox is not defined
Source File: chrome://fofs/content/overlaydialog.js Line: 6

Anyway something is wrong with the jar file (?).

	:aceman Assignee
	Comment 31 • 12 years ago

(In reply to ISHIKAWA, chiaki from comment #30)

Thanks for testing. Looking forward to you new findings.
 
> DateTime: 2012/12/20 23:56:29
> Error: ReferenceError: officialSearchBox is not defined
> Source File: chrome://fofs/content/overlaydialog.js Line: 6
> 
> Anyway something is wrong with the jar file (?).

fofs??? That looks like some extension. Try to disable it.

	ISHIKAWA, Chiaki Reporter
	Comment 32 • 12 years ago

OK, surprisingly, TB built from comm-central tree (of about a few weeks old) 
shows the same error symptom I described in comment 29 for windows version of TB.

Changing FilterListDialog.js after patching it in omni.ja in my ~/thunderbird
directory (I am not sure if this is relevant), and below MOZOBJ directory where somehow it is stored under MOZOBJ/mozilla/dist/bin/chrome/messenger/content/messenger/
is also changed. But still no luck, the buggy behavior is there.

I am not sure if the correct JS file is being replaced for my testing...

Anyway, I have captured video of the buggy behavior described here to show
that the bug is for real.
https://www.dropbox.com/s/iug63ensr2ryvxd/tb-filter-bug.zip

[I can't promise it stays forever but until the bug is fixed, sure.]

Please down load the zip file and unzip it to a folder.
It will create three files:
t-filter-bug.htm
t-filter-bug.js
t-filter-bug.swf

View t-filter-bug.htm using firefox and enable javascript, and presumably you need Adobe plugin to see the captured video. (I am afraid that the right-hand
side is cut a little short, but you see the problematic symptom.
When the filter is moved at the bottom, it loses the high-lighting, and
when I clicked it twice to show that the filter is not highlighted although
chosen, I opened the filter dialog without intention.)

But again, I am not sure if the TB is looking at the new patched FilterListDialog.js at all :-(
I wonder WHERE TB just after a fresh compile (and without installing)
finds the copy of FilterListDialog.js.
I have a suspicion that TB is still looking at the unpatched file somehow and
causing the buggy behavior.
I intentionally put what I thought was a syntax error in the replaced file(s), but they did not get flagged(?) in error console? [Or such syntax error is not reported in error console?

	ISHIKAWA, Chiaki Reporter
	Comment 33 • 12 years ago

Hi,

finally the patch worked under TB built under linux 32bits.

I have no idea why, but after I recompiled TB (comm-central),
and re-invoked the new binary, this time I get the error for the
artificially introduced syntax error in FilterListDialog.js.
The error console prints out the syntax error, and
filter manager dialog had a blank rectangle where the filter list
should be shown.

Eventually, I found out that the new binary is reading the .js file
under MOZOBJ/mozilla/dist/bin/chrome/messenger/content/messenger/
and once I fixed the syntax error there, and restart TB,
I no longer see the problematic behavior described in
comment 29.

Great improvement!

(Not sure though why my earlier attempt to let TB binary read
the patched .js failed. It is possible that I left the original source
in MOZSRC directory in the original unpatched state in my first attempt,
and I changed it and re-compiled and thus, new binary "knew" the
patched file in the first place? Well, it is more like a configuration
problem. Anyway, the patch itself now works! )

	:aceman Assignee
	Comment 34 • 12 years ago

You also need to delete the startupCache file in your profile when you change code files like this.

	:aceman Assignee
	Comment 35 • 12 years ago

Thanks ISHIKAWA chiaki, can you mark feedback+ on the patch?

	ISHIKAWA, Chiaki Reporter
	Comment 36 • 12 years ago

Comment on attachment 688889 [details] [diff] [review]
patch

(In reply to :aceman from comment #34)
> You also need to delete the startupCache file in your profile when you
> change code files like this.

It must have been it. (I wonder when this cache was introduced. When I debugged
comm-central's mozilla/toolkit/mozapps/downloads/nsHelperAppDlg.js, it was copied
MOZOBJ/mozilla/dist/bin/components/nsHelperAppDlg.js and TB picked the content up
every time from there, so I was not aware of this caching feature.

(In reply to :aceman from comment #35)
> Thanks ISHIKAWA chiaki, can you mark feedback+ on the patch?

I am about to do it with this post.

	Blake Winton (:bwinton) (:☕️)
	Comment 37 • 12 years ago

Comment on attachment 688889 [details] [diff] [review]
patch

I think I'm happy enough with this to give it a ui-r=me, although for the actual review, I would really like to see a mozmill test that fails without the patch.  (That would also give me a repeatable set of steps to reproduce…  ;)

Thanks,
Blake.

	ISHIKAWA, Chiaki Reporter
	Comment 38 • 12 years ago

(In reply to Blake Winton (:bwinton) from comment #37)
> I would really like to see a mozmill test that fails without
> the patch.  (That would also give me a repeatable set of steps to reproduce…
> ;)

Thank you for approval. 

Between ":aceman" and myself, I think we have shown that repeating this buggy behavior is very difficult :-)
And another thing is I wonder how a mozmill test can recognize whether proper
highlighting takes place or not (?).

TIA

	Magnus Melin [:mkmelin]
	Comment 39 • 12 years ago

You should be able to check the computed styles for the element in question, and se if it's those of a highlighted element or not.

	:aceman Assignee
	Comment 40 • 12 years ago

Yes, if the element does not lie to us. As this is a bug in the toolkit widget it may display something else then its internal state says. I'll see what DOM Inspector says.

	:aceman Assignee
	Comment 41 • 12 years ago

According to DOM inspector the element returns the truth, the background is transparent and the listitem also does not have the "selected" attribute set (but "current" is set). But I am not able to create a mozmill test for the filter list dialog, I don't see any tests existing there that I could use.

	Magnus Melin [:mkmelin]
	Comment 43 • 12 years ago

Comment on attachment 707203 [details] [diff] [review]
patch v2

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

I think this is fairly ugly (esp. the selectedItems). If we need to can't we just unset and reset the gFilterListbox selected rows?

	:aceman Assignee
	Comment 44 • 12 years ago

I am not sure what you mean.

	Magnus Melin [:mkmelin]
	Comment 45 • 12 years ago

Comment on attachment 707203 [details] [diff] [review]
patch v2

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

Instead of keeping the selection in selectedItems, just use the gFilterListbox selection as current trunk, but if it needs a "refresh" just unset them all, then set them again?

	:aceman Assignee
	Comment 46 • 12 years ago

How does that accomodate the usage in onDeleteFilter() where we want to set a new selection, not preserve the current one?

	Magnus Melin [:mkmelin]
	Comment 47 • 12 years ago

You'd set the selected index first, like we do now?

So to make my suggestion clear, if it works just inside updateViewPosition unset whats selected and then select it again. Or maybe i don't understand the workaround-bugfix.

	:aceman Assignee
	Comment 48 • 12 years ago

Did you mean something like this? But it does not work correctly. Do I have  a bug there?

	Magnus Melin [:mkmelin]
	Comment 49 • 12 years ago

Comment on attachment 716671 [details] [diff] [review]
alternative patch

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

yeah something like this... at least it keeps the workaround reasonable contained

::: mail/base/content/FilterListDialog.js
@@ +666,4 @@
>  
>      // The current item should be the first selected item, so that keyboard
>      // selection extension can work.
> +    gFilterListbox.currentItem = gFilterListbox.getSelectedItem(0);

why the .getSelectedItem(0) change?

	:aceman Assignee
	Comment 50 • 12 years ago

Did I forgot to mention that the patch does not work? :)

	Magnus Melin [:mkmelin]
	Comment 51 • 11 years ago

(In reply to :aceman from comment #50)
> Did I forgot to mention that the patch does not work? :)

You did, so now we just have to find out why. Bascially it should do the same thing, right?

	Magnus Melin [:mkmelin]
	Comment 52 • 11 years ago

Comment on attachment 716671 [details] [diff] [review]
alternative patch

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

::: mail/base/content/FilterListDialog.js
@@ +643,5 @@
>  
>  function updateViewPosition(firstVisibleRowIndex)
>  {
> +  function real_updateViewPosition(firstVisibleRowIndex) {
> +    let selectedItems = gFilterListbox.selectedItems.slice(0);

Why the slice here? I that the bug?

	Magnus Melin [:mkmelin]
	Comment 53 • 11 years ago

Comment on attachment 707203 [details] [diff] [review]
patch v2

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

Marking this r- per previous comments, at least for now.

	:aceman Assignee
	Comment 54 • 11 years ago

I put the slice() there intentionally to make that DOM list not live. So the array preserves contents even when the real selection is cleared.

	ISHIKAWA, Chiaki Reporter
	Comment 56 • 11 years ago

I am afraid that I lost the video mentioned in comment 32
when I cleaned up my Dropbox directory.
But I hope this does not affect this debug effort negatively.

TIA