Closed Bug 78290 Opened 23 years ago Closed 3 years ago

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

Categories

(Core :: Internationalization, defect)

defect
Not set
normal

Tracking

()

RESOLVED INACTIVE
Future

People

(Reporter: jbetak, Assigned: waterson)

References

Details

Attachments

(5 files, 1 obsolete file)

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
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
If you run the attached test case in viewer, and ``Dump Content'', you can see 
that the content model appears correct.
Nominating for nsbeta1 (for the intl bug 73881).
Keywords: nsbeta1
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.1
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...
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...
Attached file expanded XUL test case (part 2 of 2) (obsolete) —
taking from pink
Assignee: pinkerton → waterson
Status: ASSIGNED → NEW
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?
(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?
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?
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.
(Sigh, and I can't bring up the context menu to test this. Adding dependency to
bug 78725.)
Depends on: 78725
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
hyatt, pink: any thoughts on an |oncreatecomplete| event for XP menus? (See 
above.)
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?
Changing QA contact to teruko for now, please re-assign further as appropriate.
QA Contact: andreasb → teruko
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.
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
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.
The charset menu checkmark problem (bug 73881) does not happen on Macintosh somehow.
Filed bug 82025, requesting for onmenucomplete.
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 → ---
Blocks: 104166
Keywords: nsbeta1
Target Milestone: --- → Future
Status: NEW → ASSIGNED
Blocks: 98625
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
Updated test case (url of attachment changed)
Attachment #33627 - Attachment is obsolete: true
The updated test case is WFM.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → WORKSFORME
> 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 → ---
Status: REOPENED → NEW

RDF is not supported anymore

Status: NEW → RESOLVED
Closed: 14 years ago3 years ago
Resolution: --- → INACTIVE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: