The "Checked" RDF attribute is not honored by XUL layout template

RESOLVED INACTIVE

Status

()

Core
Internationalization
RESOLVED INACTIVE
17 years ago
3 days ago

People

(Reporter: jbetak@netscape.com (away - not reading bugmail), Assigned: Chris Waterson)

Tracking

Trunk
Future
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 1 obsolete attachment)

it appears to be impossible to set an appropriate attribute in an RDF source 
and have it come out as a checkmark in XUL.

It seems that existing attempts to set the attribute 
checked="rdf:http://home.netscape.com/NC-rdf#Checked" to "true" end 
unsuccessfully. The checkmark typically has to be set "manually" from JS.

http://lxr.mozilla.org/seamonkey/source/intl/uconv/src/nsCharsetMenu.cpp#622
http://lxr.mozilla.org/seamonkey/source/xpfe/components/search/resources/search-
panel.js#311
(Assignee)

Comment 1

17 years ago
This is a generic problem with menus; specifically, a <menupopup> subtree 
with <menuitem type="radio"> built using the DOM APIs doesn't seem to honor 
the checked attribute. I'll attach a test case that demonstrates.
Assignee: waterson → pinkerton
Component: RDF → XP Toolkit/Widgets: Menus
QA Contact: tever → jrgm
Summary: The "Checked" RDF attribute is not honored by XUL layout template → radio <menuitem>s built via DOM APIs do not set checked menu
(Assignee)

Comment 2

17 years ago
Created attachment 32893 [details]
test case that builds menu using DOM APIs
(Assignee)

Comment 3

17 years ago
If you run the attached test case in viewer, and ``Dump Content'', you can see 
that the content model appears correct.

Comment 4

17 years ago
Nominating for nsbeta1 (for the intl bug 73881).
Keywords: nsbeta1
Created attachment 33087 [details]
corrected test-case, added 'menuitem.setAttribute("type", "radio")'
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.1
Created attachment 33623 [details]
added rdf-based charset menu
nhotta, waterson,

it seems like the original test case was missing menuitem.setAttribute("type",
"radio"). I don't think there is a problem with building a menu the DOM API. At
least not as originally outlined.

I've attempted to enhance the test case to better illustrate the problem I filed
the bug for, however I don't seem to be able to access the charset menu RDF
source when running inside the browser content window (see latest attachment).

Maybe I can at least briefly outline the addition I had in mind. I wanted to set
the checkmark in an RDF-based menu by adding an assertion to the RDF graph. In
my private build, I replaced "menuitem.setAttribute('checked', 'true');" in
UpdateCurrentMailCharset() from charsetOverlay.js with this code snippet:

var rdf =
Components.classes["@mozilla.org/rdf/rdf-service;1"].getService(Components.interfaces.nsIRDFService);
var localStore = rdf.GetDataSource("rdf:local-store");
var charsetMenuSRC = rdf.GetResource("NC:ComposerCharsetMenuRoot", true);
var checkedProperty = rdf.GetResource("http://home.netscape.com/NC-rdf#checked",
true);
var detectorID = menuitem.getAttribute("id");
var detectorSRC = rdf.GetResource(detectorID, true);
localStore.Assert(charsetMenuSRC, checkedProperty, detectorSRC, true);

The idea was that this should be equivalent with setting the checkmark via the
DOM API, but then I might be missing something...
(Assignee)

Comment 8

17 years ago
Oh brother, what an idjit! Yes, my DOM test case was broken, so there's no
XPMenu bug here. I'll take the bug back, but am still not convinced the problem
is in the toolkit. I'll attach yet another version of the menu bar that has an
RDF-generated menu to illustrate...
(Assignee)

Comment 9

17 years ago
Created attachment 33626 [details]
RDF/XML test case, simple sequence (part 1 of 2)
(Assignee)

Comment 10

17 years ago
Created attachment 33627 [details]
expanded XUL test case (part 2 of 2)
(Assignee)

Comment 11

17 years ago
taking from pink
Assignee: pinkerton → waterson
Status: ASSIGNED → NEW
(Assignee)

Comment 12

17 years ago
jbetak: a couple comments on your test case.

1. The XUL <template> looks fine. By that I mean, it will ask the datasource
   for the |http://home.netscape.com/NC-rdf#checked| property, and set the
   |checked| attribute.

2. A checkmark (or dot) only appears on a XUL <menuitem> if the |checked|
   attribute is set to |true|. In other words,

     <menuitem type="radio" checked="blarg" />

   will not do anything.

3. The JS that you wrote will create the following link in the RDF graph,
   which I don't think is what you want:

   [NC:ComposerCharsetMenuRoot]
        |
        +--[http://home.netscape.com/NC-rdf#checked]--+
                                                      |
           [some random charset resource]<------------+

   The reason that I think this isn't what you want is that you're trying
   to match a pattern like _this_ in your template:

   [some random charset resource]
        |
        +--[http://home.netscape.com/NC-rdf#checked]--> "true"

So, I'd suggest that you write your JS this way:

  var rdf =
    Components.classes["@mozilla.org/rdf/rdf-service;1"].
    getService(Components.interfaces.nsIRDFService);

  var ls = rdf.GetDataSource("rdf:local-store");

  // where |menuitem| is presumably the RDF-generated menu that we
  // want to select...
  var detectorID = menuitem.getAttribute("id");

  ls.Assert(rdf.GetResource(detectorID),
            rdf.GetResource("http://home.netscape.com/NC-rdf#checked"),
            rdf.GetLiteral("true"),
            true);

Alternatively, we could re-write the XUL template so that it would, in fact, 
match a structure like the JS you wrote is trying to create. What do you think?
(Assignee)

Comment 13

17 years ago
(Restoring title to its original wording; however, the test case of 05/08/01 
22:01 is a counter-example that illustrates that the XUL template builder is in 
fact generating a menu from RDF/XML that properly sets the |checked| state.)
Summary: radio <menuitem>s built via DOM APIs do not set checked menu → The "Checked" RDF attribute is not honored by XUL layout template
waterson,

now it's my turn to say !!??!? - my bad this time. I tried the suggested
modification and somehow that still didn't yield the expected result. I also
checked to see, what Cata originally wrote in nsCharsetMenu.cpp and I believe it
matches your last comment:

res = Assert(node, kNC_Checked, checkedLiteral, PR_TRUE);
http://lxr.mozilla.org/seamonkey/source/intl/uconv/src/nsCharsetMenu.cpp#620

If memory serves, Cata was not able to get this to work and then came up with
the kludge in UpdateCurrentMailCharset(), which recently fell apart. Would you
have an idea, what's still going astray?

Comment 15

17 years ago
Adding meyself and ftang to this bug. 73881 depends on this one.
waterson:

I used your RDF test menu and tried something I've attempted with the charset 
menu a while back:

localStore.HasAssertion(rdf.GetResource("Choice 1"),
rdf.GetResource("http://home.netscape.com/NC-rdf#checked"),             
rdf.GetLiteral("true"), true);

This assertion query returns 'false' and 

localStore.Assert(rdf.GetResource("Choice 2"),
rdf.GetResource("http://home.netscape.com/NC-rdf#checked"),             
rdf.GetLiteral("true"), true);

doesn't move the checkmark from the first to the second item. 

Although apparently the XUL template, which was Tao's and mine prime suspect, 
seems to be fine, something is still not working right. Would you have an idea, 
where to look?
(Assignee)

Comment 17

17 years ago
Okay, a couple of comments. First, on your test:

  localStore.HasAssertion(rdf.GetResource("Choice 1"),
    rdf.GetResource("http://home.netscape.com/NC-rdf#checked"),
    rdf.GetLiteral("true"), true);

This isn't going to find an assertion, because the graph from my test
case looks like this:

[urn:root]
    |
    +--[http://www.w3.org/1999/02/22-rdf-syntax-ns#instanceOf]
    |                           |
    |                           v
    |     [http://www.w3.org/1999/02/22-rdf-syntax-ns#Seq]
    |
    |
    +--[http://www.w3.org/1999/02/22-rdf-syntax-ns#_1]-->[-]
    |                                                     |
    | "Choice 1"<--[http://home.netscape.com/NC-rdf#name]-+
    |                                                     |
    |  "true"<--[http://home.netscape.com/NC-rdf#checked]-+
    |
    |
    +--[http://www.w3.org/1999/02/22-rdf-syntax-ns#_2]-->[-]
    |                                                     |
    | "Choice 2"<--[http://home.netscape.com/NC-rdf#name]-+
    |
    |
    +--[http://www.w3.org/1999/02/22-rdf-syntax-ns#_3]-->[-]
                                                          |
      "Choice 3"<--[http://home.netscape.com/NC-rdf#name]-+

Where [-] connotates an anonymous resource that's created for a
<Description> element without an |ID| or |about| attribute. So, your
call to |rdf.GetResource("Choice 1")| is getting a node that's not
really in this graph. We can talk about this more off-line if you're
interested.

Second, the code you referred me to in nsCharsetMenu.cpp. I think
there's a bug in |nsCharsetMenu::SetCharsetCheckmark()|. Specifically,
that method generates a resource URI as follows:

  char csID[256];
  aCharset->ToCString(csID, sizeof(csID));
  res = mRDFService->GetResource(csID, getter_AddRefs(node));

However, in |nsCharsetMenu::AddMenuItemToContainer()|, it uses more
complicated logic:

  nsAutoString cs;
  res = aItem->mCharset->ToString(cs);
  if (NS_FAILED(res)) return res;

  nsAutoString id;
  if (aIDPrefix != NULL) id.AssignWithConversion(aIDPrefix);
  id.Append(cs);

  // Make up a unique ID and create the RDF NODE
  char csID[256];
  id.ToCString(csID, sizeof(csID));
  res = mRDFService->GetResource(csID, getter_AddRefs(node));

So, are we sure that |aIDPrefix| is always null? Without tracing
through things all that carefully, I see several cases where it is
not.
(Assignee)

Comment 18

17 years ago
(Sigh, and I can't bring up the context menu to test this. Adding dependency to
bug 78725.)
Depends on: 78725
(Assignee)

Comment 19

17 years ago
I've investigated this a bit more. The problem appears to have been caused by 
a re-ordering of the menu's |oncreate| handler and the time at which RDF is 
building the content. Specifically, the |oncreate| handler is firing before RDF 
gets a chance to build any children. (This may not be a bad thing; maybe 
you'd want to change the `ref' attribute or something.) Here's a nasty hack in 
charsetOverlay.js that will fix the immediate problem for the main charset menu; 
other menus would need to be hacked similarly.

 function UpdateMenus(event)
 {
-    UpdateCurrentCharset();
+    setTimeout("UpdateCurrentCharset();", 0);
     UpdateCharsetDetector();
 }


A more elegant fix might be to add an |oncreatecomplete| event. Maybe we could 
create an RFE on XPToolkit for that.

Kicking back to intl.
Assignee: waterson → nhotta
Component: XP Toolkit/Widgets: Menus → Internationalization
QA Contact: jrgm → andreasb
(Assignee)

Comment 20

17 years ago
hyatt, pink: any thoughts on an |oncreatecomplete| event for XP menus? (See 
above.)

Comment 21

17 years ago
waterson- the reason you kick back to intl is to ask use to put the temp hack 
for now, right ? Should we 1. put your hack into our charset menu code and check 
in, 2. reassign this bug back to you to work on complete fix later?

Comment 22

17 years ago
Changing QA contact to teruko for now, please re-assign further as appropriate.
QA Contact: andreasb → teruko
(Assignee)

Comment 23

17 years ago
Either way, we're going to need to change the intl code. I'll talk to pink and
hyatt today to see how they feel about an |onmenucomplete| event.

Comment 24

17 years ago
I tried the setTimeout() workaround and it seems to work. 
But I am not sure how this make it work.
waterson, why setTimeout("UpdateCurrentCharset();", 0); fix the problem?
Status: NEW → ASSIGNED
(Assignee)

Comment 25

17 years ago
Here is what happens when a menu is created:

1. User clicks on the menu
2. Menu's |oncreate| event handler is run.
3. RDF is told to construct menu contents

By doing a setTimeout(), you're scheduling an event that occurs in a callback,
which fires after RDF has constructed the menuitems.

So there are a couple of ways that we could fix this:

1. Change the ordering so that |oncreate| fires after RDF constructs
   the menus.

2. Leave |oncreate| alone, and add a new event, |oncreatecomplete|, that
   fires after RDF has constructed its content.

3. Use |setTimeout| to simulate (2).

I don't think we should change the ordering, because I'm suspicious that some
people may depend on having |oncreate| run _before_ RDF so that they can change
what RDF builds.

hyatt and pink seem amicable to creating an |onmenucomplete| event (or something
like it; they're planning to rename all the event handlers for XUL 1.0), which
would be the ``cleanest'' way to solve the problem.

Certainly using |setTimout| would be the most expedient way to get this stuff
working; we could go back and clean up that code when we're changing over to the
new menu event syntax. So, I guess I'd recommend (3) for now, and then file a
FUTURE bug for using the |onmenucomplete| event.
setTimeout won't work on macos, as we're deep w/in OS code and not processing the 
event loop.

Comment 27

17 years ago
The charset menu checkmark problem (bug 73881) does not happen on Macintosh somehow.

Comment 28

17 years ago
Filed bug 82025, requesting for onmenucomplete.

Comment 29

17 years ago
Clear the milestone since we have a workaround patch in bug 73881.
Reassign to waterson since this bug is not i18n specific.
Assignee: nhotta → waterson
Status: ASSIGNED → NEW
Target Milestone: mozilla0.9.1 → ---

Updated

17 years ago
Blocks: 104166

Updated

17 years ago
Keywords: nsbeta1
(Assignee)

Updated

17 years ago
Target Milestone: --- → Future
(Assignee)

Updated

17 years ago
Status: NEW → ASSIGNED

Updated

16 years ago
Blocks: 98625

Comment 30

15 years ago
I hope it is somehow related :  In addition to never been able to used the
checked attribute, I never successfuly (with Mozilla 1.4) make work those kind
of xul/rdf code :

<vbox id="algorithm" flex="1" datasources="blabla.rdf" ref="urn:root">
<template>
	<groupbox flex="1" uri="rdf:*">
	<caption label="rdf:http://ltrim.com/AGO-rdf#label"/>
	
	  <radiogroup selectedIndex="rdf:http://ltrim.com/AGO-rdf#select">
	    <radio label="Simple"/>
	    <radio label="Steady State"/>
	    <radio label="Incremental"/>
	    <radio label="Deme"/>
	  </radiogroup>
	</groupbox>
</template>
</vbox>

The caption shows me that I am not so fool...

Thanks
QA Contact: teruko → i18n

Comment 31

8 years ago
Created attachment 450666 [details]
[updated] expanded XUL test case (part 2 of 2)

Updated test case (url of attachment changed)
Attachment #33627 - Attachment is obsolete: true

Comment 32

8 years ago
The updated test case is WFM.
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → WORKSFORME

Comment 33

8 years ago
> The updated test case is WFM.

I removed the setTimeout in Bug 73881 and the problem is still there so the test case doesn't cover all cases.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---

Updated

8 years ago
Status: REOPENED → NEW

Comment 34

3 days ago
Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.
Status: NEW → RESOLVED
Last Resolved: 8 years ago3 days ago
Resolution: --- → INACTIVE
You need to log in before you can comment on or make changes to this bug.