595432 - Invalid form popup should use an arrow panel

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Mounir Lamouri (:mounir)]

Unfortunately the invalid form popup is going to land without an arrow panel. We should change that as soon as bug 554937 is pushed.

Comment 1

[Mounir Lamouri (:mounir)]

Attachment: WIP Patch

Comment 2

[Mounir Lamouri (:mounir)]

Builds:
http://stage.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-42e8238bb164/

Comment 3

[Nochum Sossonko [:Natch]]

Looks good! Although I'd suggest much less glass border (or any border for that matter) since this isn't a full-sized arrow panel.

Comment 4

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

Limi, this is changing the style of the popup when the users try to submit an invalid form. I think that's better because it's now consistent with most other popups and the user will understand that's a message from the UA and not the website. In addition, it's nicer. But that's far from the initial draft you did for this popup so let me know if you are fine with the change.

Sent to the try server if you need to test something:
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-50c95438d689

(screenshots will come)

Comment 5

[Mounir Lamouri (:mounir)]

Attachment: Style change for MacOS X

On Windows too, the style will now be the default arrow panel default style (not the same whether Aero is enabled or not). I will try to attach screenshots when builds will be done.

On Linux, the style is the same given that arrow panels are not enabled.

The idea is to judge the general aspects. For small tweaks like those mentioned in comment 3, bug 602500 would be appropriate.

Comment 6

[:Margaret Leibovic]

(In reply to comment #5)
> On Linux, the style is the same given that arrow panels are not enabled.

Stephen Horlander is working on a patch for the linux arrow panel styling right now, so there should probably be a follow-up bug to get rid of that #invalid-form-popup styles in browser.css if it's left in for now. (See bug 577930 and bug 604257.)

Comment 7

[Mounir Lamouri (:mounir)]

Attachment: Style change for Windows

The arrow isn't exactly on the input field but this is the case for all arrow panels on Windows (only tested with Aero disabled). I should check if we have a bug filed for that.

Comment 8

[Mounir Lamouri (:mounir)]

Comment on attachment 486022 [details]
Style change for Windows

I thought the style would have been a bit nicer with Aero but it looks like it isn't... :(

Comment 9

[:Margaret Leibovic]

(In reply to comment #7)
> I should check if we have a bug filed for that.

Bug 606343?

Comment 10

[Mounir Lamouri (:mounir)]

(In reply to comment #9)
> (In reply to comment #7)
> > I should check if we have a bug filed for that.
> 
> Bug 606343?

Seems like it. Thanks :)

Comment 11

[Alex Limi (:limi) — Firefox UX Team]

Comment on attachment 485733 [details] [diff] [review]
Patch v1

As far as what it's attempting to do, this patch/styling seems fine to me.

I'm not entirely sold on the reason why we should use doorhanger-style notifications here as opposed to something that is closer to tooltips (do we really want to emphasize that it's the browser and not the web site telling you to do this when the website has control of the actual text?), but I'm fine with shipping this in the next beta(s) to get feedback.

Comment 12

[Mounir Lamouri (:mounir)]

(In reply to comment #11)
> Comment on attachment 485733 [details] [diff] [review]
> Patch v1
> 
> As far as what it's attempting to do, this patch/styling seems fine to me.
> 
> I'm not entirely sold on the reason why we should use doorhanger-style
> notifications here as opposed to something that is closer to tooltips (do we
> really want to emphasize that it's the browser and not the web site telling you
> to do this when the website has control of the actual text?), but I'm fine with
> shipping this in the next beta(s) to get feedback.

I think the doorhanger style is nicer than the tooltip style but the main reason is that authors can't style the panel so having a browser-style panel might be better because users may understand that this is a message from the browser and not the website. That way, authors might use it more because, right now I have the feeling that they are reluctant because they can't style it.

Comment 13

[Mounir Lamouri (:mounir)]

Do you think you can get this reviewed before b8 freeze? It would be nice to get feedbacks from web authors as soon as possible.

Comment 14

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 485733 [details] [diff] [review]
Patch v1

Why are you leaving the style rule in browser.css for gnomestripe?

Why did you add the orient=vertical to the panel?

Does the max-width rule for the description actually work?

Comment 15

[Mounir Lamouri (:mounir)]

(In reply to comment #14)
> Comment on attachment 485733 [details] [diff] [review]
> Patch v1
> 
> Why are you leaving the style rule in browser.css for gnomestripe?

Because there is no arraw type panel for gnomestipe yet :(

> Why did you add the orient=vertical to the panel?
>
> Does the max-width rule for the description actually work?

Yes, with orient=vertical. Without orient=vertical, it doesn't work.

Comment 16

[Benjamin Smedberg]

What is the effect of this on extension compatibility?

Comment 17

[Mounir Lamouri (:mounir)]

(In reply to comment #16)
> What is the effect of this on extension compatibility?

The orient change shouldn't have any effect given that AFAIK, panels have orient=vertical but not arrowpanels. So I've added that to keep the same behavior.

The only possible issue is if an extension is changing the style of the panel based on the current style (like adding red borders). With this change, the result might be ugly.
But I don't think that should happen given that this has been added in b7 (so, quite recently) and it's obvious that the current style should be changed (and I said so in my blog post for this feature) and why an extension would do that?

Comment 18

[Johnny Stenback (:jst)]

Blocking to get this feature finalized for Firefox 4.

Comment 19

[Mounir Lamouri (:mounir)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/2f448cda82bd

bug 615979 will be used to update the gnomestrip (GTK) theme as soon as arrow panel will be enabled for GTK.