280365 - <wizard>.canAdvance doesn't en/disable Finish button

Status: NEW

(Core :: XUL, defect)

Description

[Ben Bucksch (:BenB)]

<http://www.xulplanet.com/references/elemref/ref_wizard.html#prop_canAdvance>

This property is set to true if the user can press the Next button to go to the
next page. ... This has the effect of enabling or disabling the Next button, or,
on the last page of the wizard, the Finish button.

The latter doesn't match the implementation - canAdvance has no effect on the
Finish button. I think it should, it would be logical for me.

Patch attached. I haven't checked, if that breaks anything in the suite or
conflicts with any other documented behaviour.

Comment 1

[Ben Bucksch (:BenB)]

Attachment: Fix, v1

I don't have to set the newly added field, because initWizardButton does that.

Comment 2

[Stephen Donner [:stephend] Not actively reading bugmail]

Can you clarify the summary a bit?  It seems contradicatory, at best.

"<wizard>.canAdvance should doesn't en/disable Finish button"

Comment 3

[[e-mail]]

This isn't going to work on the Mac, because it expects to be able to disable
the Next button on the last page rather than hiding it. (Weirdly, when you try
to disable the Finish button on Windows it changes it to a Next > button!)

Comment 4

[Ben Bucksch (:BenB)]

> This isn't going to work on the Mac, because it expects to be able to disable
> the Next button on the last page rather than hiding it.

I don't follow. I don't change the Next button behaviour at all.
If you're on the last page and the Next button is hidden and you set
canAdvance = false, the hidden Next button would be disabled, but that doesn't
hurt. The next canAdvance = true (which happens onEnter every page, I think)
will enable it again.

I have only tested this on Linux so far. Will test on Windows. Don't have a Mac.

I'm using 1.7, BTW.

Comment 5

[[e-mail]]

(In reply to comment #4)
>I don't follow. I don't change the Next button behaviour at all.
You're making it so people can use canAdvance on the last page, and canAdvance
affects the Next button, which is never hidden on the Mac.

>The next canAdvance = true will enable it again.
Which is exactly what will go wrong on the Mac.

We don't normally disable buttons on the last page, so the problem has never
occured. I think the create profile wizard special-cases the finish button.
Should we have a canFinish property that only makes sense on the last page?

Comment 6

[Ben Bucksch (:BenB)]

I still don't understand the problem, but OK.

> Should we have a canFinish property that only makes sense on the last page?

Doesn't make much sense to me. How about:

if (!this.lastPage)
  this._nextButton.setAttribute('disabled', !val); 
else
  this._finishButton.setAttribute('disabled', !val);
?
That would be your canFinish, just that it's called canAdvance :) Or is there a
problem with that?

Comment 7

[[e-mail]]

Sure, that would work.

Aside: I wonder why these setters don't return their set value.

Comment 8

[Mark C]

I just hit this problem today with Firefox 3b5 on Windows and Linux:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.9b5) Gecko/2008032620 Firefox/3.0b5

Mozilla/5.0 (X11; U; Linux i686; en-GB; rv:1.9b5) Gecko/2008041514 Firefox/3.0b5


When canAdvance is unset or set to true on the wizard, clicking the "Finish" button will close the wizard window. When it is set to false, clicking the "Finish" button will _not_ close the window and onwizardfinish doesn't fire. In that sense the button is disabled, however it has the same appearance as in its enabled state (unlike the "Next" button which appears properly disabled), which is confusing.

Comment 9

[Mark C]

Attachment: Screenshot of the wizard button appearance

This image shows the button appearance on Windows XP with the Classic theme, and Ubuntu 8.04 with its theme. In both cases the bottom-right image is the problem - the finish button should appear disabled, like the "Next" button above it.

Comment 10

[alex-mozbugs]

Hit this in FF 3.5.13 and FF 4.0b6.  The button is not visually disabled, though it is functionally disabled (when canAdvance is false clicking on it does nothing).