Closed Bug 323850 Opened 19 years ago Closed 18 years ago

XForms widgets (xforms.xml) for xul

Categories

(Core Graveyard :: XForms, defect)

Product:
Core Graveyard
Component:
XForms
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: surkov, Assigned: surkov)

References

Details

(Keywords: fixed1.8.0.4, fixed1.8.1)

Attachments

(2 files, 3 obsolete files)

test case
3.78 KB, application/vnd.mozilla.xul+xml
Details
patch
19.96 KB, patch
smaug
: review+
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20051212 Firefox/1.6a1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20051212 Firefox/1.6a1

The patch coming up.

Reproducible: Always
Depends on: 323845
Blocks: 307627
Assignee: aaronr → surkov
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
*** Bug 323006 has been marked as a duplicate of this bug. ***
Attached file test case — 

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #211942 -
        Flags: review?(smaug)

    
  

        Attachment #211942 -
        Flags: review?(allan)

    
  
Blocks: 327236

    
    

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

Code-wise I mostly have nits, except for the use of the html:input.

> +++ mozilla/extensions/xforms/resources/content/xforms.css	2006-02-15 12:22:00.000000000 +0800
> +xul|*:root output {
> +  -moz-binding: url('chrome://xforms/content/xforms-xul.xml#xformswidget-output');
> +  display: -moz-box;
> +}

I'm a bit uncertain on whether all styling should go here, or directly on the control. It's a design issue I guess. We talked about it on on IRC, and for theming issues we might want to stash everything in CSS... Different opinions anyone?

> +xul|*:root secret html|input[readonly] {
> +  background-color: -moz-Dialog;
> +  color: -moz-DialogText;
> +}

No XUL "secret" control?

> +++ mozilla/extensions/xforms/resources/content/xforms-xul.xml	2006-02-15 12:22:00.000000000 +0800
> +   - The Original Code is Mozilla XForms support.
> +   -
> +   - The Initial Developer of the Original Code is
> +   - Novell, Inc.

Well, congrats on your job at Novell ;)

> +   - Portions created by the Initial Developer are Copyright (C) 2005
> +   - the Initial Developer. All Rights Reserved.

I konw you are in a different time zone, but in a different year?! ;)

> +<bindings id="xformsBindings"
> +          xmlns="http://www.mozilla.org/xbl"
> +          xmlns:html="http://www.w3.org/1999/xhtml"

remember that the xhtml namespace should not be needed, when you kill the input type="secret"

> +  <!-- LABEL: <DEFAULT> -->
> +  <binding id="xformswidget-label"
> +           extends="chrome://xforms/content/xforms.xml#xformswidget-label-base">
> +    <content>
> +      <xul:deck anonid="activecontent" flex="1">

I do not like the name "activecontent", it sort of implied that it is either that or "implicitcontent" that is shown or something... How about just "deck" for example, or "contentswitcher"?

> +  <binding id="xformswidget-input-base"
> +           extends="chrome://xforms/content/xforms.xml#xformswidget-input-base">

> +      <handler event="input">
> +        <![CDATA[

why the CDATA?

> +  <binding id="xformswidget-input"
> +           extends="chrome://xforms/content/xforms-xul.xml#xformswidget-input-base">

I do not think you need an absolute path. Isn't "#xformswidget-input-base" enough? This might be elsewhere too.

> +    <implementation>
> +      <method name="getControlElement">
> +        <body>
> +          var control = this.ownerDocument.
> +              getAnonymousElementByAttribute(this, 'anonid', 'control');
> +
> +          return {
> +            control: control,

please add an empty line here. Makes it easier to read

> +    <handlers>
> +      <handler event="keypress" keycode="VK_RETURN">
> +        <![CDATA[

again, CDATA, why?

> +  <!-- SECRET: <DEFAULT> -->
> +  <binding id="xformswidget-secret"
> +           extends="chrome://xforms/content/xforms-xul.xml#xformswidget-input-base">

> +      <method name="getControlElement">
> +        <body>
> +          return {
> +            __proto__: this.ownerDocument.
> +              getAnonymousElementByAttribute(this, 'anonid', 'control'),
> +
> +            set readonly(val) {
> +              if (val) {
> +                this.setAttribute('readonly', 'true');

Should be "readonly" instead of "true", but this will probably change when you use XUL control instead.

> +  <binding id="xformswidget-textarea"
> +           extends="chrome://xforms/content/xforms-xul.xml#xformswidget-input-base">
> +      <method name="getControlElement">
> +        <body>
> +          var control = this.ownerDocument.
> +              getAnonymousElementByAttribute(this, 'anonid', 'control');
> +
> +          return {
> +            control: control,

newline

> +  <binding id="xformswidget-trigger" display="xul:button"
> +           extends="chrome://xforms/content/xforms-xul.xml#xformswidget-trigger-base">

> +      <handle event="command">
> +        // XXX: we should fire 'DOMActivate' event for properly xforms:submit
> +        // work since xul:button don't do it (see a bug 323005

// XXX: we need to fire 'DOMActivate' event to get xforms:submit
// to work, since xul:button do not do it (see a bug 323005

Functionality-wise:

* I think the minimal trigger should have some styling that indicates that you can click on it

* Nothing seems to happen when I click a trigger. As is: I get no visual indication of my click. And the trigger does not get focus, which it should.

        Attachment #211942 -
        Flags: review?(smaug)

        Attachment #211942 -
        Flags: review?(allan)

        Attachment #211942 -
        Flags: review-

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch (obsolete)
        — 
          — Splinter Review
      

    


  
with allan's comments

        Attachment #211942 -
        Attachment is obsolete: true

        Attachment #212107 -
        Flags: review?(allan)

    
  

        Attachment #212107 -
        Flags: review?(smaug)

    
  
Blocks: 326556

    
    

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

Nice. I still want a visual indication of that you can click on a minimal trigger, without hovering over it. Can you not underline it or something?

with that, r=me

        Attachment #212107 -
        Flags: review?(allan) → review+

    
    

    
  
(In reply to comment #6)
> (From update of attachment 212107 [details] [diff] [review] [edit])
> Nice. I still want a visual indication of that you can click on a minimal
> trigger, without hovering over it. Can you not underline it or something?
> 
> with that, r=me
> 

Ok, XUL toolbar buttons do not have that normally, so forget that. r=me without any additional requirements.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        up-to-date (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #212107 -
        Attachment is obsolete: true

        Attachment #212462 -
        Flags: review?(smaug)

        Attachment #212107 -
        Flags: review?(smaug)

    
    

    
  
Comment on attachment 212462 [details] [diff] [review]
up-to-date

Hmm, is there something wrong in my build or why the input elements don't work... :(

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch
        — 
          — Splinter Review
      

    


  
(In reply to comment #9)
> (From update of attachment 212462 [details] [diff] [review] [edit])
> Hmm, is there something wrong in my build or why the input elements don't
> work... :(
> 

I'm sorry. I did wrong update. Now it's fixed.

        Attachment #212462 -
        Attachment is obsolete: true

        Attachment #212547 -
        Flags: review?(smaug)

        Attachment #212462 -
        Flags: review?(smaug)

        Attachment #212547 -
        Flags: review?(smaug) → review+

    
    

    
  
Checked in
Checking in jar.mn;
/cvsroot/mozilla/extensions/xforms/jar.mn,v  <--  jar.mn
new revision: 1.13; previous revision: 1.12
done
RCS file: /cvsroot/mozilla/extensions/xforms/resources/content/input-xul.xml,v
done
Checking in resources/content/input-xul.xml;
/cvsroot/mozilla/extensions/xforms/resources/content/input-xul.xml,v  <--  input-xul.xml
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/extensions/xforms/resources/content/xforms-xul.xml,v
done
Checking in resources/content/xforms-xul.xml;
/cvsroot/mozilla/extensions/xforms/resources/content/xforms-xul.xml,v  <--  xforms-xul.xml
initial revision: 1.1
done
Checking in resources/content/xforms.css;
/cvsroot/mozilla/extensions/xforms/resources/content/xforms.css,v  <--  xforms.css
new revision: 1.15; previous revision: 1.14
done
Whiteboard: xf-to-branch

    
  
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED

    
  
Blocks: 332853

    
  
Keywords: fixed1.8.0.3, 
          
            fixed1.8.1

    
  
Whiteboard: xf-to-branch
Product: Core → Core Graveyard

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: