196755 - [mailviews] [modern skin only] Unable to create new Views (pull down menus do not retain changes)

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Message Display, defect)

Description

[Ninoschka Baca]

Trunk build 2003-03-10: WinXP
Interesting that the problem does not appear on MacX.

Overview: When creating a new Message View, try to change the first pull down
from "Subject" to any other option (i.e. Sender) and it reverts back to Subject.
This is also a problem with the second pull down...what ever the default is then
try to change it and it quickly reverts back (i.e. "is", change to "isnt",
reverts to "is")

Steps to reproduce:
1. In the Views pull down select Customize
2. Select the New button
3. Type a View Title
4. In the first pull down, change "Subject" to "Sender"
5. In the second pull down, change "contains" to "doesnt contain"

Actual Results: 
4 Results: first pull down quickly reverts to "Subject"
5 Results: second pull down quickly reverts to "contains"

Note: I have tried a variety of settings and it always reverts back to "Subject"
and "Contains" so I am unable to create different Views.

Comment 1

[Ninoschka Baca]

Marking nsbeta1, Creating Custom Views is severely broken and needs to be fixed
on Windows.

Comment 2

[(not reading, please use [e-mail] instead)]

regression.

neil or shuehan, can you help investigate?

Comment 3

[(not reading, please use [e-mail] instead)]

I see this in modern, but not classic.

Comment 4

[Ninoschka Baca]

2003-02-28: WinXP/Modern - ok
2003-03-02: WinXP/Modern - displays the problem so it started over the 3/1-3/2
weekend.

Comment 5

[(not reading, please use [e-mail] instead)]

ninosckha, this happens on all platforms, right?

Comment 6

[Ninoschka Baca]

The problem occurs on Mac and Linux using the Modern skin.

Comment 7

[[e-mail]]

Also affects address book and advanced mail searches.

Comment 8

[[e-mail]]

Very weird this - using the classic skin, the command event is targeted at the
menuitem but using the modern skin the event is targeted at the menupopup...

Comment 9

[[e-mail]]

Might be something to do with the scrollbox - if I remove the binding for
arrowscrollbox then the events target properly.

Maybe menulist.xml should be using event.originalTarget instead, or is that too
hacky?

Comment 10

[esther]

*** Bug 196392 has been marked as a duplicate of this bug. ***

Comment 11

[Mitch]

*** Bug 196718 has been marked as a duplicate of this bug. ***

Comment 12

[(not reading, please use [e-mail] instead)]

saving the triage team the hassle: this is clearly adt1, nsbeta1+

Comment 13

[Samir Gehani]

Mail triage team: nsbeta1+/adt1

Comment 14

[(not reading, please use [e-mail] instead)]

narrowing down the regression, the 2003-03-01-08-trunk win32 bits work

Comment 15

[(not reading, please use [e-mail] instead)]

2003-03-03-08-trunk appears to work as well.

Comment 16

[(not reading, please use [e-mail] instead)]

2003-03-05-08-trunk works as well, still looking for when it broke

Comment 17

[(not reading, please use [e-mail] instead)]

it appears to have broken between 2003-03-05-08-trunk and 2003-03-06-08-trunk

looking at checkins now.

Comment 18

[(not reading, please use [e-mail] instead)]

the checkin for bug #194429 looks possible, I'll try backing it out locally.

Comment 19

[(not reading, please use [e-mail] instead)]

backing out #194429 from my local tree fixed this.

re-assign to jkeiser.

#194429 also caused a crasher regression, see bug #196388

Comment 20

[John Keiser (jkeiser)]

Yes, it sounds like the problem is that event retargeting is working correctly
now and menulist was relying on it being broken.  The event is *supposed* to be
retargeted when it goes through an anonymous boundary, which is what happens
when there is a binding.

http://www.w3.org/TR/xbl/#anonymous-events

Changing it to .originalTarget will probably work properly.  I can't reproduce
yet, strangely (my tree has the mentioned fix); I'm pulling up-to-date now.

This was neil's change (from originalTarget to target) to work around the event
retargeting problem, and I suspect it extends beyond just this file. 
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/xpfe/global/resources/content/bindings/menulist.xml#28
was checked in 2 days after bug 104401 (the bug that erroneously made these
things not retarget).  The checkin does not include a bug #, unfortunately, and
Bugzilla is not showing any bugs owned by neil or cmanske being set to FIXED in
that time, but combing Bonsai will at least tell us what other workarounds were
checked in along with it.  neil, does this jog your memory at all?  A bug #
would probably help ...

Comment 21

[[e-mail]]

What I don't understand is why the event (correctly, in my opinion) does not
retarget when the menupopup's binding is popup.xml#popup-scrollbars but does
retarget (although not generated from within the menupopup's anonymous content)
when the menupopup's binding is popup.xml#popup - as menuitem is not an
anonymous child of the menupopup the event should not be retargeted by it. The
event is getting retargeted while bubbling up through the menupopup's anonymous
content - the anonymous box reports the target as being the menuitem but the
scrollbox thinks that it is the target (and the event then gets retargeted when
it bubbles out to the menupopup). Is the scrollbox somehow disrupting the event
flow?

Comment 22

[[e-mail]]

OK, so here is what I think is going on:
A <searchattribute> has anonymous content <menulist> <menupopup> <menuitem>
In Modern, a <menupopup> has anonymous content <arrowscrollbox> <children>
An <arrowscrollbox> has anonymous content <scrollbox> <children>
(plus incidentally a couple of <autorepeatbutton>s)
A <scrollbox> has anonymous content <box> <children>
Now when the event propogates from the box to the scrollbox, the code in
nsXULElement.cpp erroneously retargets the event, because its parent is in a
different scope. Unfortunately its grandparent is in the same scope...

Would it be more accurate (and simpler) for the code to say "if the target's
binding parent is my parent, then retarget the event before propagating it"?

Comment 23

[[e-mail]]

Attachment: Quick hack

This hack fixes the issue for me, but isn't tested for regressions :-/

Comment 24

[John Keiser (jkeiser)]

I still think .originalTarget is the right fix within the current XBL world.
According to my reading of the spec, the event *should* retarget when it goes
from <box> to <scrollbox>.  I think it is the spec that needs fixing, however,
and if we do that we should do it right:

<searchattribute> (<menulist> <menupopup> (<arrowscrollbox> (<scrollbox> (<box>
[<menuitem>]))))

In this example, if the event was targeted at <box>, then <menulist> should
rightfully see the target set to <menupopup>.  But if it's <menuitem>, then
<menulist> should not see the target set to <menupopup>; it should see
<menuitem>.  With the current spec it is *supposed* to see <menupopup>, if I
read it correctly.

hyatt's input would be valuable here.

It is highly odd, but with WinXP 2003031216 I *still* cannot reproduce; nor with
my debug build.  Am I missing steps?  I go to Customize... create a new View,
type a title, select "Sender" and "doesn't contain", and the selects stay as
they are.  Actually causing the creation of the view doesn't help either, when I
go back to the view they still show up as "Sender" and "doesn't contain".

Comment 25

[David Hyatt]

Is menuitem in this example explicit content?  If so, you should know about the issues with explicit content that gets interleaved with anonymous content.  The first is that events just flow up the parent chain and not along the altered anonymous chain.  I view that as a bug, but I don't think it needs to be fixed to resolve this issue.

Given that currently events just go up the parent chain, that means explicit content should *skip* any anonymous content that got interleaved and will go right up to its parent.  That means the retargeting check should only kick in if you really are anonymous content and you are crossing scopes.

I see nothing obviously wrong with my code as originally written (just looking at that snippet). 

Comment 26

[David Hyatt]

Neil, your patch looks logically correct and simpler than what I was doing.  
However, I still can't see why my code is logically really any different, and 
that disturbs me.  I'd love to see an example that really illustrates the flaw 
in my code so that we know *why* it doesn't quite work.

Comment 27

[David Hyatt]

Ok, your check is close, and i get what's happening now.  It isn't *quite* 
right though.  I totally forgot that Dan Rosen did actually fix events to flow 
up the real binding chain, and that wasn't quite right when it was landed.

Here's what needs to be done:

(1) Get the binding parent of event.target.  Really use target! Right now the 
code bails and uses the current node if the DOM event isn't created yet.  That 
is *wrong*.
(2) Get the real parent of this node.  That is insertionParent if defined, 
mParent otherwise.
(3) Compare the real parent to the binding parent and retarget if they are the 
same.

Comment 28

[(not reading, please use [e-mail] instead)]

jkeiser, this is modern skin only.

another way to reproduce is to do an advanced message search (Tools | Search
Messages...) on sender.  

you should find that you can't pick sender (the second item).

search, filters and views all use the same xbl widgets.

thanks to neil, jkeiser, and hyatt for the immediate attention.

Comment 29

[[e-mail]]

I'm still happy with my patch as it is, so here's my reply to comment 27:

(1) Get the binding parent of event.target.  Really use target! Right now the 
code bails and uses the current node if the DOM event isn't created yet.  That 
is *wrong*.

I don't know why it might be wrong, but I didn't touch that code anyway :-P

(2) Get the real parent of this node.  That is insertionParent if defined, 
mParent otherwise.

Not that it's too much trouble to get the real parent, because that's where the
event propagates to later, but IMHO document.getAnonymousNodes(parent)[0..n]
never has an anonymous parent, only the real parent, so I didn't bother.

(3) Compare the real parent to the binding parent and retarget if they are the 
same.

OK, so this is what I am already trying to do.

Comment 30

[Matthias Versen [:Matti]]

*** Bug 198008 has been marked as a duplicate of this bug. ***

Comment 31

[John Keiser (jkeiser)]

Neil, hyatt showed me a case that your code would break--that retarget =
PR_FALSE; stuff is in there for a reason.  The other code looked reasonable.  We
suspect the root problem is the fact that we don't check the target's parent if
there is no DOMNode.  If you can't get to this soon, I will, after I clear a
couple of other bugs.

Comment 32

[R.K.Aa.]

*** Bug 196430 has been marked as a duplicate of this bug. ***

Comment 33

[[e-mail]]

Seeing as I don't actually know what it is that my patch is supposed to break I
won't be able to look at it at all :-/

Comment 34

[Matthias Versen [:Matti]]

*** Bug 198253 has been marked as a duplicate of this bug. ***

Comment 35

[Matthias Versen [:Matti]]

*** Bug 198277 has been marked as a duplicate of this bug. ***

Comment 36

[R.K.Aa.]

*** Bug 198365 has been marked as a duplicate of this bug. ***

Comment 37

[John Keiser (jkeiser)]

The JS workaround is .originalTarget, btw--I believe that will work fine.

Comment 38

[[e-mail]]

Attachment: Patch to workaround problem in JS

Comment 39

[(not reading, please use [e-mail] instead)]

instead of "XXX", can you do "XXX bug #196755"

jkeiser / jag, what do you guys think?

Comment 40

[John Keiser (jkeiser)]

Comment on attachment 118030 [details] [diff] [review]
Patch to workaround problem in JS

I think it looks fine.	I have a fundamental fix brewing in my tree, but it
will take time to work out kinks and look for regressions.

Comment 41

[(not reading, please use [e-mail] instead)]

Comment on attachment 118030 [details] [diff] [review]
Patch to workaround problem in JS

sr=sspitzer, but please get r (or sr) jag, too.

Comment 42

[jag (Peter Annema)]

Comment on attachment 118030 [details] [diff] [review]
Patch to workaround problem in JS

Mention the bug# after the XXX and sr=jag

Comment 43

[(not reading, please use [e-mail] instead)]

I added the bug number to the XXX comments, tested the fix, and landed it.

thanks for the fix, neil.

jkeiser, should we open a new bug for the fix you've got brewing your tree, and
make a note to back out this change when you land?

Comment 44

[R.K.Aa.]

*** Bug 198857 has been marked as a duplicate of this bug. ***

Comment 45

[Sam Hulick]

*** Bug 198810 has been marked as a duplicate of this bug. ***

Comment 46

[(not reading, please use [e-mail] instead)]

landed, so not blocking 1.4 alpha.

Comment 47

[R.K.Aa.]

*** Bug 199136 has been marked as a duplicate of this bug. ***

Comment 48

[R.K.Aa.]

*** Bug 194801 has been marked as a duplicate of this bug. ***

Comment 49

[Ninoschka Baca]

Trunk build 2003-03-25: Mac 10.1.5, WinME, Linux RH 8
Verified Fixed. Using the Modern skin, now creating new MailViews is working as
well as Search Messages and Search Addresses.

Comment 50

[WADA:World Anti-bad-Duping Agency]

With 2003032508-trunk/Win-Me(Modern Theme), Tools/Message Filter worked almost
very well, but still has somo problems.

On "Message Filters" UI panel:
(1) Enable button of exisiting filter does not worked.
 If filter is newly created one, enable button worked normally.
(2) For disabled exisiting filter, which was disabled before 2003-03-25 build,
edit button was grayed out.
Edit button worked on already enabled filters.

Should I re-create filters or edit filter file? 

Comment 51

[Mitch]

In response to comments #50 

> (1) Enable button of exisiting filter does not worked.

Works fine for me on Linux with cvs build 20030325

Comment 52

[james davis]

re: #50 and 51 - with 032508 on Win2k the buttons work fine for me. I tried
several different ops and had no problems. WADA - you may want to move off your
filters dat file and recreate your filters using the UI and retest.

Comment 53

[WADA:World Anti-bad-Duping Agency]

Since problem occurs only in my environment, my filter file may be corrupted.
(My Mozilla experienced crash too many times, forced by new nightlies. Pitiful
my Mozilla...)
I'll create new filter and compare old and new filter file.
Thanks a lot, Baker-san & Davis-san.

Comment 54

[WADA:World Anti-bad-Duping Agency]

2003032508-trunk/Win-Me with Modern Theme.

I found that enable/edit button problem occurs when filter name contains
Japanese characters. 

Recreation procedure:
(1) Create a new filter of English filter name (eg. Test)
(2) Click enable ("X" on enable button is cleard)
(3) Press Edit button
(4) Rename filter to a string contains Japanese characters
(5) Press OK and save change
=> Enable button does not work. Edit button is grayed out.

Workaround:
(a) For already enabled filter only
 Rename filter to English name from UI
(b) For both already enabled and disabled filter
 Edit msgFilterRules.dat
 Change name="With Japanese chars" to name="Without Japanese chars"

Comment 55

[WADA:World Anti-bad-Duping Agency]

Problem I described in Comment 50 and Comment 54, which is Japanese folder name
case problem, still occurs on Mozilla 1.4a released version, Mozilla/5.0
(Windows; U; Win 9x 4.90; en-US; rv:1.4a) Gecko/20030401.
Should I open a new bug? 

Comment 56

[(not reading, please use [e-mail] instead)]

was this in 1.3?  I don't see it in my debug 1.3.1 build.

can someone else confirm that it's not in 1.3?

if it is, I can land for 1.3.1

Comment 57

[WADA:World Anti-bad-Duping Agency]

Confirmed that it's NOT in 1.3(1.3 Release Version/Modern Theme/Win-Me).
Creation/Change etc. of message filer worked very well even on Japanese filter
name. 

Comment 58

[(not reading, please use [e-mail] instead)]

thanks WADA.

Comment 59

[Sander]

*** Bug 196631 has been marked as a duplicate of this bug. ***

Comment 60

[WADA:World Anti-bad-Duping Agency]

We Japanese have found that Modern Theme has no relation to "Enable" checkbox
problem (described in  comment #54) and opend bug 202729 for this new problem.
Sorry for my SPAMs and thamks for advice. 


    

Comment 61

[[e-mail]]

Attachment: Updated for bitrot

/me wonders whether jkeiser's brew has stewed by now :-)

Comment 62

[Brian Ryner (not reading)]

Comment on attachment 128961 [details] [diff] [review]
Updated for bitrot

So, with this patch, what target would the <scrollbox> see for an event
targeted at a <menuitem>?  If I'm reading it correctly, it would see the
original target.  Is that the case, and if so, is that the correct behavior?

Comment 63

[[e-mail]]

Well, it's the same behaviour as for scrollboxes in non-anonymous menulists...

Comment 64

[Brian Ryner (not reading)]

Comment on attachment 128961 [details] [diff] [review]
Updated for bitrot

r=bryner, let's get this in early in 1.7a so we can have time to shake out any
regressions.

Comment 65

[Brian Ryner (not reading)]

Comment on attachment 128961 [details] [diff] [review]
Updated for bitrot

Also, this same change should be made to nsGenericElement, for consistency.

bz should probably review this as well, he's done some XBL and event
retargeting stuff lately.

Comment 66

[[e-mail]]

Attachment: Patch to nsGenericElement too

Comment 67

[[e-mail]]

Attachment: Fix document element retargetting issue

My previous patch erroneously thought that document elements were anonymous :-(

Comment 68

[Johnny Stenback (:jst)]

Comment on attachment 137409 [details] [diff] [review]
Fix document element retargetting issue

sr=jst

Comment 69

[[e-mail]]

Comment on attachment 118030 [details] [diff] [review]
Patch to workaround problem in JS

Ok, so can I back this (and the other workaround in another bug) out now?

Comment 70

[jag (Peter Annema)]

Comment on attachment 118030 [details] [diff] [review]
Patch to workaround problem in JS

Yes please.

I believe we also applied a work-around for this in bug 206668.

Comment 71

[[e-mail]]

Comment on attachment 118030 [details] [diff] [review]
Patch to workaround problem in JS

jst, we should be able to back out this workaround and bug 206668 now.

Comment 72

[Johnny Stenback (:jst)]

Comment on attachment 118030 [details] [diff] [review]
Patch to workaround problem in JS

r=jst

Comment 73

[[e-mail]]

I've backed out the workarounds.

Comment 74

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: toolkit port of backout (checked in)

Comment 75

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 160837 [details] [diff] [review]
toolkit port of backout (checked in)

also contains a few whitespace fixes.

Comment 76

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #75)
> (From update of attachment 160837 [details] [diff] [review])
> also contains a few whitespace fixes.

this needs relanding

Comment 77

[Ian Neal]

Relanding of patch following landing of aviary branch