241855 - Once the import wizard is opened, it cannot be closed/canceled

Status: RESOLVED FIXED

(Firefox :: Migration, defect, P2)

Description

[Martin Creutziger [:MMx]]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7) Gecko/20040426 Firefox/0.8.0+ (MMx2000)
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7) Gecko/20040426 Firefox/0.8.0+ (MMx2000)

When I click "Import" in a recent Firefox nighty (even in my built-from-CVS
MOZILLA_1_7_BRANCH Firefox), I cannot abort/close the inport wizard. I have to
close the browser.

Reproducible: Always
Steps to Reproduce:
1.click "Import..."
2.try to abort/close the wizard


Actual Results:  
I have to quit Firefox to get rid of the window

Expected Results:  
provide a close button ("x" in title bar) or "Cancel"

Comment 1

[Martin Creutziger [:MMx]]

Attachment: Screenshot illustrating the problem

attached screenshot of the wizard

Comment 2

[riscky]

I can confirm this... I have seen this on my workstation... OS X 10.3.3
Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8a) Gecko/20040427
Firefox/0.8.0+

You can *only* see this, in my experience when ~/Library/Firefox/ doesn't
exist... old data isn't imprted from ~/Library/Phoenix/

Note: I haven't used Firefox on this system for some time as a result my profile
was in ~/Library/Phoenix/

Comment 3

[riscky]

You can quit the migration manager... and Firefox does start as one would expect.

Comment 4

[Martin Creutziger [:MMx]]

In reply to #2:
This is not about the migration wizard that launches on first launch when it
detects a ~/Library/Phoenix folder. This is about the one you can launch
manually from the "File" menue.
Therefore I do not quite understand #3 ...

Comment 5

[Martin Creutziger [:MMx]]

Attachment: another screenshot, window unusable

In this screenshot, there was no ~/Library/Phoenix folder and no other
browser's profile availiable. This results in a completely unusable window. The
"Next" button does not work and there is no way to close it... :-(
Try it with a fresh user account on OS X, and you should see the same.

Comment 6

[Martin Creutziger [:MMx]]

Just to make shure, I tried it using my fresh nighty under OS X.2.8, with the
same result as in Comment #5. (No screenshot here, we all know how Jaguar looks
like ;-) )
Build: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7)
Gecko/20040427 Firefox/0.8.0+ (MMx2000)

Comment 7

[Ali Ebrahim]

This is a fairly major polish bug that affects usability of the browser, and one
that should not require a very complicated fix (I could be wrong). Therefore, I
think we should try and get this in for 0.9. It doesn't seem like there should
be any major recoding required to get this in.

If this ends up as blocking0.9-, whoever sets that please either set
blocking1.0? or blocking1.0+.

Comment 8

[Asaf Romano (gone)]

As far as I remember, ESC close the import wizard,

No we just need to enable the close windows button..

Comment 9

[Martin Creutziger [:MMx]]

well, yes, ESC closes it. I did not think of that. But neither will most of the
users, so we *have* to enable this button.

Comment 10

[Bruce Davidson]

Happy to take this bug. I have a patch coming up, though it made need some
refining to make it acceptable.

Comment 11

[Bruce Davidson]

Attachment: Messy patch to illustrate proposed change - doesn't apply cleanly

This is my first cut patch. It removes the unused top button panel for Wizards
in MacOS X (which contained the cancel and finish buttons). It moves the cancel
button to the left side of the panel containing next and previous. It removes
the finish button altogether. This may be controversial, personally I think it
has no meaning (except on the last page where it should really be used in place
of "Next"), but I'm happy to listen to points either way.

The second bit of the patch simply removes the grey background from the button
area on MacOS X, so that it looks more like a mac dialog. Unfortunately
something is really screwy in the file (line endings?) and the diff is a mess.
Honestly I've only added a #ifndef XP_MACOSX. I'll aim to provide a clean diff
tomorrow.

Comment 12

[Bruce Davidson]

Okay, scrap the previous patch - it doesn't allow you to close the wizard when
you get to the end!

The current design for wizards on Mac appears to use buttons as follows:

                           [ Cancel ] [ Finish ]
                       [ < Prev ] [#] [ Next > ]

The top line (cancel and finish) are only displayed on the last page of the
wizard (the one that tells you what the wizard just did), when the "Finish"
button is enabled and the "Cancel" button is disabled.

This is messy. It uses lots of screen real estate and means that the Cancel
button is only displayed on a page where it is disabled. I propose to re-jig
this design as follows:

Pages other than the final page:

               [ Cancel ]               [ < Prev ] [#] [ Next > ]

Final page of Wizard:

               [ Cancel ]               [ < Prev ] [#] [ Finish ]

Obviously the Cancel and Previous buttons will be disabled on this final page.

Patch coming up when I work out how to do this in XBL.

Comment 13

[Bruce Davidson]

*** Bug 227890 has been marked as a duplicate of this bug. ***

Comment 14

[Bruce Davidson]

Attachment: Revised patch - properly fixes Mac Wizards

This patch properly fixes the problem with the cancel button on Mac toolkit
wizards. It changes the button design to that mentioned in my previous comment
on this bug. In making this change I have deliberately made it more similar to
the non-Mac code in terms of implementation.

I have stripped out a couple of pointless XUL boxes. I have verified that these
make no difference whatever to the layout. I have simplified the Mac skin
stylesheet accordingly. However this bit of the patch still looks really odd so
I will attach that file in is entirety separately.

Comment 15

[Bruce Davidson]

Attachment: Mac skin wizard.css - Unix line endings

In case there are problems applying the patch for this file.

Comment 16

[Bruce Davidson]

Comment on attachment 149105 [details] [diff] [review]
Revised patch - properly fixes Mac Wizards

Requesting review.

Comment 17

[Asaf Romano (gone)]

(In reply to comment #16)
> (From update of attachment 149105 [details] [diff] [review])
> Requesting review.
> 

Bruce, I think you shuld ask review from Ben Goodger, or someone else who has
access to mac. As far as I remember mike doesn't :-\

Comment 18

[Mike Connor [:mconnor]]

Comment on attachment 149105 [details] [diff] [review]
Revised patch - properly fixes Mac Wizards

the original structure had the bindings for OS X all together and the bindings
for other platforms all together.  I don't see why we'd change that, this
actually makes the code a little harder to follow

I'd like to see a new patch for this with that addressed, but the review should
come from someone who can actually test this on Mac (which would be ben, unless
someone feels like donating me a Mac :)

Comment 19

[Bruce Davidson]

Attachment: Updated patch addressing Mike's comment

Thanks for those comments Mike. Good point about the mac - I'm afraid I haven't
got one spare to donate :-(

Here's an updated patch that leaves the Mac and other platform code separate.
Tested it and it has exactly the same effect.

Comment 20

[Ben Goodger (use ben at mozilla dot org for email)]

Your patch does not apply - the changes to wizard.css are malformed. Please try
again. 

Comment 21

[Bruce Davidson]

Attachment: Updated patch that excludes wizard.css

Yeah, wizard.css has really messed up line endings (some Mac, some unix and
some DOS). It really screws up diff. (See what LXR does to it too!). This is a
new patch that excludes wizard.css. You can copy the new wizard.css that's also
attached to this bug over the top of the old one, to get those changes in a
proper unix line ending file. (No changes have ever been made before to that
wizard.css so there's no problem with clashes).

Comment 22

[Ben Goodger (use ben at mozilla dot org for email)]

I actually have these changes in my mac tree so I can check them in in the next
couple of days. 

Comment 23

[Ben Goodger (use ben at mozilla dot org for email)]

missing the boat, bigger bugs have just come up

Comment 24

[Mike Connor [:mconnor]]

fixed on branch, the wizard.xml changes were already in, we needed the
wizard.css ones still though.

Will land this on trunk soon.

Comment 25

[Asaf Romano (gone)]

check with 0.9rc and this hasn't been fixed at all :(

Comment 26

[Bruce Davidson]

Looks to be fixed in 0.9 final release that I downloaded. However I don't think
Ben has checked this in on the trunk yet. Any reason (bar lack of time) for this
Ben/Mike?

Comment 27

[Asaf Romano (gone)]

(In reply to comment #26)
> Looks to be fixed in 0.9 final release that I downloaded. However I don't think
> Ben has checked this in on the trunk yet. Any reason (bar lack of time) for this
> Ben/Mike?

It hasn't fixed for me... open the extension manager update wizard for example.
The close buttonn of the wizzard is still disabled.

Comment 28

[Asaf Romano (gone)]

I moved the blocking flag from 1.0 to 1.0mac.

Sorry for bugspam.

Comment 29

[Bruce Davidson]

Asaf, can you post a screenshot of this problem. It looks okay to me, but I
haven't really played with the extension manager much so might be looking in the
wrong place entirely.

Comment 30

[Asaf Romano (gone)]

Attachment: Screenshot 1: extension manager wizard

As you see, no title bar.

Comment 31

[Asaf Romano (gone)]

Attachment: Screenshot 2: Import Wizard

The close window button of the Import wizard is still disabled

Comment 32

[Bruce Davidson]

Okay, I see what you're getting at now. I think the lack of any title bar in the
update wizard should probably be a new bug. I'd completely forgotten about the
close widget in the title bar mentioned in the early comments of this bug. Now
that the patch on this bug is applied to the branch but not the trunk its
probably easier if we create a separate bug for this too. If you can do that
I'll have a look at fixing them over the weekend.

Comment 33

[Asaf Romano (gone)]

As you like, just be aware that it's not specific to the extension manager
update wizard,  as far as i remember, it's for every dependent window (not modal
- this becomes as a sheet).

Comment 34

[Asaf Romano (gone)]

Attachment: Just another example of no_title_bar window

Your'e probably right, this should be a (*big one*) separate bug.

Comment 35

[Asaf Romano (gone)]

What about checking this in for the trunk?

Comment 36

[Asaf Romano (gone)]

Fixed on trunk, thanks Bruce!