Closed
Bug 332559
Opened 19 years ago
Closed 19 years ago
calendar widget rewrite
Categories
(Core Graveyard :: XForms, defect)
Core Graveyard
XForms
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: surkov, Assigned: surkov)
References
Details
(Keywords: fixed1.8.0.4, fixed1.8.1)
Attachments
(1 file, 1 obsolete file)
|
(deleted),
patch
|
allan
:
review+
|
Details | Diff | Splinter Review |
Calendar should be changed in some way to achive more easy fix of bug 327584.
|
Assignee | |
Updated•19 years ago
|
Status: NEW → ASSIGNED
|
|
Assignee | |
Comment 1•19 years ago
|
||
The main changes:
1) The current date is called as selected date, focused date is called as current. I changed the names because sometimes focused day can be not focused. It can confuse.
2) Current (previosuly named 'focused') date changing don't involve current day focusing.
3) When I click on back/forward button then focuse is saved on the button.
Attachment #217012 -
Flags: review?(allan)
|
|
Assignee | |
Updated•19 years ago
|
Attachment #217012 -
Flags: review?(doronr)
|
||
Comment 2•19 years ago
|
||
Comment on attachment 217012 [details] [diff] [review]
patch
imho, it's better to ask for one review at a time. we might end up doing the same work twice otherwise.
Attachment #217012 -
Flags: review?(allan)
|
||
Comment 3•19 years ago
|
||
Comment on attachment 217012 [details] [diff] [review]
patch
>diff -uNrap mozilla.orig/extensions/xforms/resources/content/widgets-xhtml.xml mozilla/extensions/xforms/resources/content/widgets-xhtml.xml
>--- mozilla.orig/extensions/xforms/resources/content/widgets-xhtml.xml 2006-03-20 19:00:10.000000000 +0800
>+++ mozilla/extensions/xforms/resources/content/widgets-xhtml.xml 2006-04-03 19:47:47.000000000 +0900
>@@ -48,7 +48,7 @@
> xmlns:xbl="http://www.mozilla.org/xbl">
>
>
>- <!-- CALENDAR WIDGETS -->
>+<!-- CALENDAR WIDGETS -->
>
> <!-- COMPACT CALENDAR -->
> <binding id="calendar-compact"
>@@ -61,113 +61,125 @@
> </content>
>
> <implementation>
>+ <!-- interface -->
>+ <method name="focus">
>+ <body>
>+ if (this.currentDayIndex != -1)
>+ this._dayElements[this.currentDayIndex].focus();
>+ </body>
>+ </method>
I think it is better to have the comment indented the same way as the method. It makes it easier to read:
>+ <!-- interface -->
>+ <method name="focus">
>+ <body>
>+ if (this.currentDayIndex != -1)
>+ this._dayElements[this.currentDayIndex].focus();
>+ </body>
>+ </method>
>
> <handlers>
>- <handler event="click">
>+ <handler event="keypress">
>+ <![CDATA[
>+ var target = event.originalTarget;
>+
>+ switch (event.keyCode) {
>+
>+ case event.DOM_VK_DOWN:
>+ if (target == this.backButton || target == this.fwdButton)
>+ this.focus();
>+ break;
>+
>+ case event.DOM_VK_UP:
>+ if (target.localName == "td" || target.namespaceURI == this.XHTML_NS) {
>+ if (this.currentDayIndex - 3 <= 0) {
>+ this.backButton.focus();
>+ } else if (this.currentDayIndex - 7 <= 0) {
>+ this.fwdButton.focus();
>+ }
>+ }
>+ break;
(target.localName == "td" || target.namespaceURI == this.XHTML_NS)
Why not just test for the namespaceURI - td will always be in the xhtml ns?
Attachment #217012 -
Flags: review?(doronr) → review+
|
|
Assignee | |
Comment 4•19 years ago
|
||
(In reply to comment #3)
> I think it is better to have the comment indented the same way as the method.
> It makes it easier to read:
>
I use the presented indentation for comment groups (f.x. to separate group of interface methods) and use the your asked for indentation for local comments (f.x. what do method do). F.x.
> >+ <!-- interface -->
> >+ <!-- Set focus on day of current date -->
> >+ <method name="focus">
> >+ <body>
> >+ if (this.currentDayIndex != -1)
> >+ this._dayElements[this.currentDayIndex].focus();
> >+ </body>
> >+ </method>
I added thease comments.
> > <handlers>
> >- <handler event="click">
> >+ <handler event="keypress">
> >+ <![CDATA[
> >+ var target = event.originalTarget;
> >+
> >+ switch (event.keyCode) {
> >+
> >+ case event.DOM_VK_DOWN:
> >+ if (target == this.backButton || target == this.fwdButton)
> >+ this.focus();
> >+ break;
> >+
> >+ case event.DOM_VK_UP:
> >+ if (target.localName == "td" || target.namespaceURI == this.XHTML_NS) {
> >+ if (this.currentDayIndex - 3 <= 0) {
> >+ this.backButton.focus();
> >+ } else if (this.currentDayIndex - 7 <= 0) {
> >+ this.fwdButton.focus();
> >+ }
> >+ }
> >+ break;
>
> (target.localName == "td" || target.namespaceURI == this.XHTML_NS)
>
> Why not just test for the namespaceURI - td will always be in the xhtml ns?
>
calendar-compact supposes that it can be extended by successor binding, successor binding can replace content (like it do calendar-full) and therefore I prefer to be sure that 'td' has xhtml namespace.
Attachment #217012 -
Attachment is obsolete: true
Attachment #217106 -
Flags: review?(allan)
|
|
||
Comment 5•19 years ago
|
||
Comment on attachment 217106 [details] [diff] [review]
patch2
r=me
Attachment #217106 -
Flags: review?(allan) → review+
|
|
||
Comment 6•19 years ago
|
||
Fixed on trunk
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Whiteboard: xf-to-branch
|
|
||
Updated•19 years ago
|
Keywords: fixed1.8.0.3,
fixed1.8.1
|
|
||
Updated•19 years ago
|
Whiteboard: xf-to-branch
|
||
Updated•8 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•