106749 - Auto-detector can not be selected

Status: VERIFIED FIXED

(Core :: Internationalization, defect)

Description

[Yuying Long]

Build: 10-25-09 trunk build on Win2000-CN

Steps to reproduce:
1. Launch browser.
2. Go to any page, try those pages that doesn't has meta-charset will be beter,
e.g. http://www.goo.ne.jp
3. Select auto-detector from View | Character Coding | Auto-detect, e.g. East
Asian ...etc.

Actual result:
You will see the auto-detector name is selected and marked as a charset, e.g.
marked as "cjk_parallel_state_machine" (and it will be added under Character
Coding menu) when select auto-detect Asian...etc.  Looks like it treated as
charset instead of charset detector.

Expect result:
The actual charset like x-jis will be selected but not the detector name.

Comment 1

[Yuying Long]

This only exists on recently trunk build, I haven't see it on 0.9.4 branch build.

Comment 2

[Yuying Long]

Nominate as nsbeta1

Comment 3

[Roy Yokoyama]

Seen in MacOS X.  Changing to All platform

Comment 4

[Roy Yokoyama]

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

Comment 5

[Shanjian Li]

This bug need to be addressed sooner. 

Comment 6

[Roy Yokoyama]

Last I talked to jbetak, there are number of changes he 
is making in this area.  I think I should assign this jbetak. 
jbetak: please take a look.

Comment 7

[Roy Yokoyama]

jbetak: I may have a solution.  
MultiplexHander() in charsetOverlay.js is getting only "Shift_JIS" as a charset
instead of 'charset.Shift_JIS'.  There may be changes in XUL file.
Investigating.
-> yokoyama

Comment 8

[Roy Yokoyama]

alecf:   node name is always returning as 'charsetGroup' instead of
'detectorGroup' in MultiplexHandler(event) in charsetOverlay.js
any ideas, alec?
--- cc'ing alecf

Comment 9

[Alec Flett]

I really don't know the code - can you point me at an LXR URL with the code in
question? My only guess is that the way the DOM does event bubbling is somehow
tripping you up

Comment 10

[Roy Yokoyama]

alecf: here is function MultiplexHandler(event) in chasetOverlay.js
name = node.getAttribute('name'); <- name is always 'charsetGroup'

http://lxr.mozilla.org/seamonkey/source/xpfe/global/resources/content/charsetOve
rlay.js#1

Comment 11

[Alec Flett]

my guess is that this is a dupe of bug 107786, and that the quick fix would be
to duplicate this event handler in each <menu> node

Comment 12

[Roy Yokoyama]

alecf: 
We are seeing two issues in MultiplexHandler(event):
function MultiplexHandler(event)
{
  var node = event.target;
  var name = node.getAttribute('name');
  .........
  charset = node.getAttribute('id');

1) 'name' used to return 'detectorGroup' for Auto-Detect menu item;
   but now, 'name' returns the 'charsetGroup'.
2) 'charset' used to return a string prefixed with 'charset.' 
   (eg. 'charset.Shift_JIS').  Now we see only 'Shift_JIS'

Can you explain to us why/how the node.getAttribute(*) 
is returning different result?
What do you mean by "duplicate this event handler in each <menu> node"?
Thanks

Comment 13

[Alec Flett]

cc'ing jst and hyatt to see if something broke (or got fixed) with event
bubbling recently..

Comment 14

[Roy Yokoyama]

alec/jst/hyatt: any comments?

Comment 15

[Roy Yokoyama]

I can't reproduce 107074; partially due to this bug.
making 107074 as a depend

Comment 16

[Shanjian Li]

this bug is blocking all my effort with charset detector. Change 
severity to blocker. 
I spent more time on this hoping to identify the responsible party. 
Duplicating "MultiplexHandler" to each submenu does not work. Even 
after I change the name to be a different one, in my case "MultiplexHandler2",
it still did not help. The additional event handler did get fired and 
I can confirm this with debug output. I am suspecting that it is "name"
attribute cause all these problem. From XUL reference, "name" identifies 
a group of radio items. Apparently, it does not work. It looks like submenu
item could not override "name" attribute specified in its parent's template.
It is also possible that multiple "name" attributes has been set and only 
the first one can be accessed. Are all those attributes stored in DOM node?
How could we check if attributes has been correctly set or not?  

Another observation might worth mentioning. While I set 
oncommand="MultiplexHandler(event)" in charset root menu and set 
oncommand="MultiplexHandler2(event)" in charset detector submenu, both 
events got fired. Should attribute specified later override existing parent's 
template?

Comment 17

[Johnny Stenback (:jst)]

Hyatt, is this due to your XUL proto attr changes?

Comment 18

[Johnny Stenback (:jst)]

Hmm, I guess not, this bug was filed almost two weeks ago.

Comment 19

[Roy Yokoyama]

We have spent several days to identify the cause of this 
behaviour and we came out with empty handed.
I would really appreciate if someone in the DOM team take look at
this bug.  
-> DOM team

Comment 20

[Johnny Stenback (:jst)]

Show me the *DOM* is doing something wrong and I'll have a look, if you can't do
that then have someone who knows the menu/XUL code dig into this.

Back to Internationalization.

Comment 21

[Roy Yokoyama]

I am running out of options. Assigning to ftang
->ftang

Comment 22

[nhottanscp]

I have builds 10/22 and 10/23. 10/22 build does not have the problem but 10/23 has.
I looked at check ins between 10/22 and 10/23 and found there was a XUL change
by waterson (from the check in comment, "Bug 68871. Permit partial sharing of
XUL attributes, ....").
This might be related because what we have is that the name attribute
"detectorGroup" and "charsetGroup" are mixed up.
Cc to waterson, do you think your change is related?

Comment 23

[Shanjian Li]

I will take this bug for now before I identify the responsible party.

Comment 24

[Shanjian Li]

I traced into nsXULContentBuilder.cpp, in function "BuildContentFromTemplate", 
the attribute got from template kid is incorrect. We need to look back further 
to see how tempate kid is created. 

Comment 25

[Shanjian Li]

It looks like menu item from detectorGroup are charsetGroup are created from the 
same template. This is very likely to be a regression of bug 68871. I could not 
make rapid progress, but I will continue to look into this problem. Reassign to 
waterson. 

Chris, Could you take a look? This bug blocks many of our i18n efforts. 

Comment 26

[Chris Waterson]

Attachment: test case

Comment 27

[Chris Waterson]

Attached a minimal test case. Removing the |datasources="rdf:null"| attribute on
the outer-menu makes the inner-menu display.

Comment 28

[Chris Waterson]

This was indeed a regression from bug 68871, but not in any obvious way that I
would've suspected! I changed the order in which document observers were
notified on AttributeChanged and exposed a latent bug in the XUL content builder
code with nested templates. Specifically, in the case of nested templates, if
the _outer_ template builder is notified before the _inner_, then the outer will
attempt to handle the OpenContainer call and mark the inner's element's lazy
state  as eContainerContentsBuilt.

The fix is to improve the ``responsibility checking'' that's done in
OpenContainer and CloseContainer. Currently, they simply check to make sure that
the element on which the attribute has changed is _contained within_ the root of
the template. We also need to verify that the template builder is _us_.

Patch coming up.

Comment 29

[Chris Waterson]

Attachment: proposed fix

This patch eliminates the IsElementInWidget call (which is redundant with
IsElementContainedBy), and adds an IsElementInBuilder method. This method uses
the nsIXULDocument interface to determine whether the element in question is
under the nearest content builder's purview.

Comment 30

[Chris Waterson]

cc'ing partners in crime on this one.

Comment 31

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 57259 [details] [diff] [review]
proposed fix

I feel faint, distant shame.  sr=shaver.

Comment 32

[Robert John Churchill]

Whee, r=rjc

Comment 33

[Chris Waterson]

Comment on attachment 57259 [details] [diff] [review]
proposed fix

adding the r=rjc to the patch

Comment 34

[Chase Tingley]

This fix appears to have caused blocker bug 109505.

Comment 35

[Chris Waterson]

Attachment: As above, but with fix for bug 109505

The previous patch had a dumb typo (I'm a slob), and caused bug 109505. This
patch incorporates that fix -- intendend for mozilla-0.9.6 branch.

Comment 36

[Chase Tingley]

Comment on attachment 57369 [details] [diff] [review]
As above, but with fix for bug 109505

r=tingley

Comment 37

[Robert John Churchill]

Comment on attachment 57369 [details] [diff] [review]
As above, but with fix for bug 109505

Same for me, rjc

Comment 38

[Christopher Blizzard (:blizzard)]

a=blizzard on behalf of drivers for 0.9.6

Comment 39

[Chris Waterson]

Fixed on mozilla-0.9.6 branch.

Comment 40

[Yuying Long]

Fixed verified on 11-14 trunk build.