Invalid form popup should use an arrow panel

RESOLVED FIXED in Firefox 4.0b8

Status

()

defect
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: mounir, Assigned: mounir)

Tracking

Trunk
Firefox 4.0b8
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 betaN+, status2.0 ?)

Details

Attachments

(3 attachments, 1 obsolete attachment)

Unfortunately the invalid form popup is going to land without an arrow panel. We should change that as soon as bug 554937 is pushed.
Blocks: 602500
Posted patch WIP Patch (obsolete) — Splinter Review
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.
status2.0: --- → ?
Depends on: 606606
Depends on: 606663
Posted patch Patch v1Splinter Review
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/mlamouri@mozilla.com-50c95438d689

(screenshots will come)
Assignee: nobody → mounir.lamouri
Attachment #484644 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #485733 - Flags: ui-review?(limi)
Attachment #485733 - Flags: review?(gavin.sharp)
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.
(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.)
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 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... :(
Attachment #486022 - Attachment description: Style change for Windows (non Aero) → Style change for Windows
(In reply to comment #7)
> I should check if we have a bug filed for that.

Bug 606343?
(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 :)
Whiteboard: [needs review]
Depends on: 606343
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.
Attachment #485733 - Flags: ui-review?(limi) → ui-review+
(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.
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 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?
(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.
Attachment #485733 - Flags: approval2.0?
What is the effect of this on extension compatibility?
(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?
Blocking to get this feature finalized for Firefox 4.
blocking2.0: --- → betaN+
Attachment #485733 - Flags: review?(gavin.sharp) → review+
Whiteboard: [needs review] → [needs review][needs approval]
Whiteboard: [needs review][needs approval] → [needs approval]
Attachment #485733 - Flags: approval2.0?
Whiteboard: [needs approval]
Depends on: 615979
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.
Target Milestone: --- → Firefox 4.0b8
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Depends on: 616502
Depends on: 616607
Depends on: 619223
You need to log in before you can comment on or make changes to this bug.