327584 - Expose abstract interface for input[type="date"]

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[alexander :surkov (:asurkov)]

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

Patch is coming up

Reproducible: Always

Comment 1

[alexander :surkov (:asurkov)]

I want to split input type="date" on two widgets as least. One of them is calendar, other is input. Input shows and hides calendar. More I guess it will be usefull if calendar will be able to work with xforms model wigthout input. It means as calendar as input works with model. In instance: calendar widget is input[type="date"][appearance="calendar"] and input is just input[type="date"]. What about the wild idea? There is one advantage is good flexibility. Imo widgets should be splited on many parts if all of them can be used independently.

Comment 2

[Allan Beaufour]

(In reply to comment #1)
> I want to split input type="date" on two widgets as least. One of them is
> calendar, other is input. Input shows and hides calendar. More I guess it will
> be usefull if calendar will be able to work with xforms model wigthout input.
> It means as calendar as input works with model. In instance: calendar widget is
> input[type="date"][appearance="calendar"] and input is just input[type="date"].
> What about the wild idea? There is one advantage is good flexibility. Imo
> widgets should be splited on many parts if all of them can be used
> independently.

I think it is one mighty good idea! Maybe input[type="date"][appearance="full"] to be more "XFormish".

With a split we could also do a readonly version of the calendar, and bind it to output[type="date"][appearance="full"]. That would be neat I think.

Comment 3

[alexander :surkov (:asurkov)]

Attachment: first patch

Comment 4

[Doron Rosenberg (IBM)]

Comment on attachment 216382 [details] [diff] [review]
first patch

>+              handleEvent: function(event) {
>+                var target = event.originalTarget;
>+                switch (event.type) {
>+                case "blur":
>+                  if (this.ownerDocument == target) {
>+                    this.inputElm.hidePicker();
>+                  } else {
>+                    this.shouldHandleFocus = this.isCalendarNode(target);
>+                  }
>+                  break;
>+                case "focus":
>+                  if (this.shouldHandleFocus && !this.isCalendarNode(target) &&
>+                      target != this.dropmarkerElm) {
>+                    this.inputElm.hidePicker();
>+                  }
>+                  break;
>+                }
>+              },
>+              isCalendarNode: function(node) {
>+                var walker = this.ownerDocument.createTreeWalker(
>+                  this.pickerElm, NodeFilter.SHOW_ELEMENT, null, false);
>+                var child = null;
>+                while (child = walker.nextNode()) {
>+                  if (child == node)
>+                    return true;
>+                }
>+                return false;
>+              }

Why not walk up node's parent tree and see if it is inside the calendar element?  I think it would make us do less work that way.

>diff -uNrap mozilla.orig/extensions/xforms/resources/content/widgets.xml mozilla/extensions/xforms/resources/content/widgets.xml
>--- mozilla.orig/extensions/xforms/resources/content/widgets.xml	2006-03-20 19:00:10.000000000 +0800
>+++ mozilla/extensions/xforms/resources/content/widgets.xml	2006-03-27 16:46:36.000000000 +0900
>-          if (!aCurrentDate)
>+          if (!aCurrentDate || String(aCurrentDate) == String(new Date(undefined)))
>             aCurrentDate = new Date();
> 

why this change? please add a comment

Comment 5

[Allan Beaufour]

You should fix the W-errors that jst-review is complaining about.

Comment 6

[Allan Beaufour]

(In reply to comment #3)
> Created an attachment (id=216382) [edit]
> first patch

I would like to see a brief description about what you do in the patch. And if you introduce new behaviour / change behaviour please attach testcase(s).

And the "String(new Date(undefined))" line that Doron commented on, please make that a constant, there's no need to re-create the String object all the time.

Comment 7

[alexander :surkov (:asurkov)]

Attachment: patch2

(In reply to comment #4)

> Why not walk up node's parent tree and see if it is inside the calendar
> element?  I think it would make us do less work that way.

Do you have in view that it is better to walk from event target till calendar element? If it is so then I'm not sure because if document will be big then walk from target to document will bee longer than walk from calendar along its children.

> why this change? please add a comment

I added a comment. I do a check on invalid date like current realization of input-date do.

(In reply to comment #5)
> You should fix the W-errors that jst-review is complaining about.

I'll fix them excluding one in jar.mn because the same lines have tabs too.

(In reply to comment #6)
> And the "String(new Date(undefined))" line that Doron commented on, please make
> that a constant, there's no need to re-create the String object all the time.

I did a field for that expression.

Comment 8

[alexander :surkov (:asurkov)]

Attachment: testcase

(In reply to comment #6)
> 
> I would like to see a brief description about what you do in the patch. And if
> you introduce new behaviour / change behaviour please attach testcase(s).

Current input-date has own calendar widget. Almoust of all what I do is input-date should use exiting calendar widget. Behaviour I changed too (a little): now calendar is closed when I click on document ('hidePickerHandler' object serves for it). Also I create skin directory (as I wanted to do in bug 310122) and put some styles for calendar into it.

Comment 9

[Allan Beaufour]

Comment on attachment 216617 [details] [diff] [review]
patch2

Skin does not seem to be working for me:
Couldn't convert chrome URL: chrome://xforms/skin/widgets.css

Comment 10

[Allan Beaufour]

(In reply to comment #9)
> (From update of attachment 216617 [details] [diff] [review] [edit])
> Skin does not seem to be working for me:
> Couldn't convert chrome URL: chrome://xforms/skin/widgets.css

Please skip the skin-stuff, and do it in bug 310122. You also need to modify contents.rdf to support SeaMonkey.

Comment 11

[alexander :surkov (:asurkov)]

Attachment: patch3

Comment 12

[Allan Beaufour]

(In reply to comment #11)
> Created an attachment (id=216731) [edit]
> patch3

Two functionality things:
1) If I press click on the button twice (keyboard / mouse), to open and close the calendar, the focus ends up in the input field. I would expect it to stay on the button. Especially since, if you use the spacebar, a " " is inserted in the value.

2) If I choose a date and tab from the input field to the button, the instance data node is updated. This is wrong as the focus has not left the whole <xf:input/> control.

These two things might always have been there, so feel free to just say that, and we'll create follow-up bugs

Comment 13

[Allan Beaufour]

Comment on attachment 216731 [details] [diff] [review]
patch3

> +      <handler event="keypress" keycode="VK_RETURN">
> +        var target = event.originalTarget;
> +        if (target == this.inputField) {
> +          this.dispatchDOMUIEvent('DOMActivate');
> +        } else if (this.isPickerNode(target)) {
> +          var date = this.picker.getDate();

Wouldn't it be a lot easier to just listen for keypresses on the span, instead of all this?

The same for the hidePickerHandler. Do we need isPickerNode() at all?

> +
> +input[mozType|type="http://www.w3.org/2001/XMLSchema#date"] html|span[mozType|calendar] {
>    -moz-binding: url('chrome://xforms/content/widgets-xhtml.xml#calendar-full');
>  }

Should this not also be prefixed by html|*:root ?

Comment 14

[alexander :surkov (:asurkov)]

Attachment: patch4

Comment 15

[Allan Beaufour]

Comment on attachment 217381 [details] [diff] [review]
patch4

Nice with some comments!

>+      <method name="showPicker">
...
>+          // move the picker, aligning it's right side with the calendar
>+          // button's right side
>+          var dropmarkerBox = this.ownerDocument.
>+            getBoxObjectFor(this.dropmarker);
>+          var pickerWidth = this.ownerDocument.
>+            getBoxObjectFor(this.picker).width;
>+          var windowWidth = this.ownerDocument.defaultView.innerWidth;
>+
>+          var position = dropmarkerBox.x - pickerWidth + dropmarkerBox.width;
>+
>+          // reset position if it will bleed to the left or right
>+          if (position < 0) {
>+            position = 0;
>+          } else if ((position + pickerWidth) > windowWidth) {
>+            // we use window.innerWidth because XHTML documents are not always
>+            // 100% width, and innerWidth will always give use the browser size.

This comment should be moved up to "var windowWidth = ..."

>+            position = windowWidth - pickerWidth;
>+          }
>+
>+          this.picker.style.left = position + "px";
>+          this.picker.focus();
>+
>+          this.ownerDocument.
>+            addEventListener("blur", this.hidePickerHandler, true);
>+          this.ownerDocument.
>+            addEventListener("focus", this.hidePickerHandler, true);

Again, why all this traversing? Why wouldn't "this.picker.addEventListener(..., this.hidePicker, true)" work?

If I understand your code, you want to figure out when the input or the picker gets a focus or a blur event. Why attach the listener on the document and traverse the nodes? Why not just attach the listener directly on the picker?

>+      <!-- 'hidePickerHandler' object serves to hide date picker when date
>+        picker looses a focus. When date picker is shown then
>+        'hidePickerHandler' object is attached to the window to handle 'focus'
>+        and 'blur' events as event listener. When date picker is hidden then
>+        event listener 'hidePickerHandler' is removed.
>+        -->

When using multi-line comments keep "<!--" on a seperate line, like in C++

>+      <property name="hidePickerHandler" readonly="true">

Comment 16

[alexander :surkov (:asurkov)]

Attachment: patch5

(In reply to comment #15)

> Nice with some comments!
> 
> 
> This comment should be moved up to "var windowWidth = ..."

Fixed.
> 
> Again, why all this traversing? Why wouldn't "this.picker.addEventListener(...,
> this.hidePicker, true)" work?
> 
> If I understand your code, you want to figure out when the input or the picker
> gets a focus or a blur event. Why attach the listener on the document and
> traverse the nodes? Why not just attach the listener directly on the picker?

I guess, no. When I get 'blur' event for a picker node then I should know what element will get a focus. If focused elememnt will be a picker node then I shouldn't hide a picker, if focused element will be a non-picker node then I should hide it. Therefore I handle focus event on a document. I use 'blur' event listener on a document for picker hidding when f.x. window is minimized/maximized or address bar gets a focus.

> When using multi-line comments keep "<!--" on a seperate line, like in C++

Fixed.

Comment 17

[Allan Beaufour]

Comment on attachment 217982 [details] [diff] [review]
patch5

(In reply to comment #16)
> > If I understand your code, you want to figure out when the input or the picker
> > gets a focus or a blur event. Why attach the listener on the document and
> > traverse the nodes? Why not just attach the listener directly on the picker?
> 
> I guess, no. When I get 'blur' event for a picker node then I should know what
> element will get a focus. If focused elememnt will be a picker node then I
> shouldn't hide a picker, if focused element will be a non-picker node then I
> should hide it. Therefore I handle focus event on a document. I use 'blur'
> event listener on a document for picker hidding when f.x. window is
> minimized/maximized or address bar gets a focus.

I keep missing that point. It just seems so expensive to do this... I have not good solution for it just now, though, so r=me.

Comment 18

[Allan Beaufour]

Fixed on trunk