Closed Bug 315731 Opened 15 years ago Closed 15 years ago

When importing remote webdav calenders, you have to select "caldav" and then "webdav" to activate the "next" button

Categories

(Calendar :: Internal Components, defect)

Sunbird 0.3a1
x86
Windows XP
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: phillip.hahn, Assigned: robin.edrenius)

Details

Attachments

(1 file, 1 obsolete file)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.12) Gecko/20050915 Firefox/1.0.7
Build Identifier: Sunbird 0.3a1 german

I wanted to import my remote calenders in Sunbird. I pasted the url, and to activate the next button, you have to click "caldav" and then "webdav" again. Then you can procede

Reproducible: Always

Steps to Reproduce:
1.open import remote calender
2.paste url
3.click "caldav" and then "webdav" to activate "next"
Version: unspecified → Sunbird 0.3a1
 94                     <textbox id="calendar-uri"
 95                              required="true" onchange="checkRequired();" onkeyup="checkRequired();"/>

We've got all these nice checks, but no oninput?  I'm guessing paste doesn't trigger either of these event listeners and oninput is the way I've been tending to fix these, even though (i think) it creates some asserts in debug builds.  If anyone has a few spare minutes, it'd be nice to confirm this and get a quick patch up.  We should probably be fixed for the calendar name textbox as well, so you can paste there too.
Confirmed. But:
After pasting the URL the Next button stays dimmed but can be pressed. So this is just a 'appearance' problem.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(In reply to comment #1)
> We've got all these nice checks, but no oninput?  I'm guessing paste doesn't
> trigger either of these event listeners and oninput is the way I've been
> tending to fix these, even though (i think) it creates some asserts in debug
> builds.  If anyone has a few spare minutes, it'd be nice to confirm this and
> get a quick patch up.  We should probably be fixed for the calendar name
> textbox as well, so you can paste there too.
> 

It works to paste in the calendar name text-box.
I'll add oninput and get a patch up very soon.
Attached patch add oninput (obsolete) — Splinter Review
Actually, Joey was right, it is needed in the name-textbox as well.
Assignee: shaver → robin.edrenius
Status: NEW → ASSIGNED
Attachment #202393 - Flags: first-review?(jminta)
Side note: 
I also played a little bit around: If you paste the URL onchange is triggert, checkRequired() is called and Next button is enabled - but the UI is not updated. 
If I click somewhere (title bar, another window, ...) the UI is updated.

Robin: 
With your patch onchange and oninput are triggert, checkRequired() is called twice. Same for onkeyup and oninput. So I think only oninput is enought.
(In reply to comment #2)
> Confirmed. But:
> After pasting the URL the Next button stays dimmed but can be pressed. So this
> is just a 'appearance' problem.
> 

> Side note: 
> I also played a little bit around: If you paste the URL onchange is triggert,
> checkRequired() is called and Next button is enabled - but the UI is not
> updated. 
> If I click somewhere (title bar, another window, ...) the UI is updated.
> 

This really makes me think there's some underlying xulbug going on here.  Are we setting disabled properly and it's still letting the user advance or are we doing something wrong here too?

(In reply to comment #5)
> Robin: 
> With your patch onchange and oninput are triggert, checkRequired() is called
> twice. Same for onkeyup and oninput. So I think only oninput is enought.
> 
Fewer events calls are always good.  Otherwise we're doing double-work, which slows everything down.  I think oninput might cover the onkeyup events too.  CVS blame might have more info on why the particular ones we have now were added.
Comment on attachment 202393 [details] [diff] [review]
add oninput

See previous comment.  We shouldn't create a situation where we do double work.
Attachment #202393 - Flags: first-review?(jminta) → first-review-
Here's a new patch. Sorry for the delay.
Attachment #202393 - Attachment is obsolete: true
Attachment #202725 - Flags: first-review?(jminta)
Comment on attachment 202725 [details] [diff] [review]
Add oninput and remove un-necessary calls

OK, xulplanet explains why you have to click somewhere else with the current setup:

onchange

This event is sent when the value of the textbox is changed. The event is not sent until the focus is moved to another element.

oninput definitely looks like the right choice here. r=jminta
Attachment #202725 - Flags: first-review?(jminta) → first-review+
Thanks for the patch! checked in
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.