599745 - port ui parts of bug 561636 to support invalidformsubmit

Status: VERIFIED FIXED

(SeaMonkey :: General, defect)

Description

[Bruno 'Aqualon' Escherl]

Attachment: first WIP on port of bug 561636 UI parts

This WIP patch passes content/html/content/test/test_bug561636.html

I'm not sure about how it should look like for our themes, so asking for feedback first.

Comment 1

[[e-mail]]

Comment on attachment 478654 [details] [diff] [review]
first WIP on port of bug 561636 UI parts

>+Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
Nit: XPCOMUtils should already be available

>+  QueryInterface : XPCOMUtils.generateQI([Components.interfaces.nsIFormSubmitObserver]),
Nit: should include nsIObserver (since we pass it to addObserver)

>+    this.panel.appendChild(document.createTextNode(""));
Unnecessary, use textContent instead of nodeValue

>+  notifyInvalidSubmit : function (aFormElement, aInvalidElements)
Nit: no space before :

>+    if (gBrowser.contentDocument !=
>+        aFormElement.ownerDocument.defaultView.top.document) {
Just check that .top != content

>+    let element = aInvalidElements.queryElementAt(0, Components.interfaces.nsISupports);
[Bah, at least nsISupportsArray has an nsISupports GetElementAt method.
 But using aInvalidElements.enumerator.getNext() looks silly.
 Maybe we should query to nsIDOMElement instead?]

>+    let eventHandler = function(e) {
function eventHandler() {

>+    // One event to bring them all and in the darkness bind them all.
Only one "all" in the quote ;-)

>+      aEvent.target.removeEventListener("popuphiding", arguments.callee, false);
Nit: name the function; apparently arguments.callee is frowned upon
[At least, that's the impression I get from reading Waldo's blog]

>+  Services.obs.addObserver(observer, "invalidformsubmit", false);
>+  observer.init();
Nit: init the observer first

>+    <panel id="invalid-form-popup" noautofocus="true" hidden="true" level="parent"></panel>
This needs a <description> inside it otherwise you will get console messages about text nodes inside XUL elements. (And the description will probably give you some reasonable default padding.)

>+              // Hide the form invalid popup.
>+              if (gFormSubmitObserver.panelIsOpen()) {
>+                gFormSubmitObserver.panel.hidePopup();
This belongs in nsBrowserStatusHandler.js also if this is the only caller to panelIsOpen then I don't see the point of it.

>+/* Invalid form popup */
>+#invalid-form-popup {
>+  -moz-appearance: none;
>+  background-color: #fffcd6;
>+  border: 1px solid #dad8b6;
>+  padding: 5px 5px 5px 5px;
>+  font-weight: bold;
I don't know what this looks like because I couldn't get it to appear :-(

Comment 2

[[e-mail]]

Comment on attachment 478654 [details] [diff] [review]
first WIP on port of bug 561636 UI parts

>+#invalid-form-popup {
>+  -moz-appearance: none;
>+  background-color: #fffcd6;
>+  border: 1px solid #dad8b6;
>+  padding: 5px 5px 5px 5px;
>+  font-weight: bold;
>+}
OK, so I ran the right build this time, and this just looks like a tooltip. So you might as well copy the tooltip styles. (Except for the margin of course.)

Comment 3

[[e-mail]]

(And keep the bold font weight, I guess.)

Comment 4

[Bruno 'Aqualon' Escherl]

Attachment: show an error message on invalidformsubmit

The patch also includes another test (browser_bug595507.js) compared to the wip. I also ported the test from bug 581947, but that needs some work on FillInHTMLTooltip, I'll do this in another bug.

(In reply to comment #1)
> Comment on attachment 478654 [details] [diff] [review]
> first WIP on port of bug 561636 UI parts
> 
> >+Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
> Nit: XPCOMUtils should already be available
It doesn't seem to be for navigator.js, I get an error about missing XPCOMUtils.

> >+    let element = aInvalidElements.queryElementAt(0, Components.interfaces.nsISupports);
> [Bah, at least nsISupportsArray has an nsISupports GetElementAt method.
>  But using aInvalidElements.enumerator.getNext() looks silly.
>  Maybe we should query to nsIDOMElement instead?]
Didn't know what to do there, so kept it like it was

> >+              // Hide the form invalid popup.
> >+              if (gFormSubmitObserver.panelIsOpen()) {
> >+                gFormSubmitObserver.panel.hidePopup();
> This belongs in nsBrowserStatusHandler.js also if this is the only caller to
> panelIsOpen then I don't see the point of it.
There was also a discussion about it in the initial bug (see bug 561636 comment 77). In my opinion this looks cleaner than copying the code from panelIsOpen into nsBrowserStatusHandler.js

> I don't know what this looks like because I couldn't get it to appear :-(
I used `TEST_PATH=content/html/content/test/test_bug561636.html make mochitest-plain` to test the patch. Clicking on the submit button with the red glow shows the tooltip as does clicking on the small button next to the checkbox.

Comment 5

[Bruno 'Aqualon' Escherl]

(In reply to comment #4)
> Created attachment 480067 [details] [diff] [review]
> show an error message on invalidformsubmit
> +    // this.panel.firstChild.nodeValue = element.validationMessage.substring(0, 256);

Oops, forgot to remove that. I'll remove it after review (already did it locally).

Comment 6

[[e-mail]]

(In reply to comment #4)
> (In reply to comment #1)
> > (From update of attachment 478654 [details] [diff] [review])
> > >+Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
> > Nit: XPCOMUtils should already be available
> It doesn't seem to be for navigator.js, I get an error about missing
> XPCOMUtils.
Strange; nsContextMenu.js imports it.

> > >+              // Hide the form invalid popup.
> > >+              if (gFormSubmitObserver.panelIsOpen()) {
> > >+                gFormSubmitObserver.panel.hidePopup();
> > This belongs in nsBrowserStatusHandler.js also if this is the only caller to
> > panelIsOpen then I don't see the point of it.
> There was also a discussion about it in the initial bug (see bug 561636 comment
> 77). In my opinion this looks cleaner than copying the code from panelIsOpen
> into nsBrowserStatusHandler.js
Actually I was wondering why panelIsOpen exists in the first place.

Comment 7

[Bruno 'Aqualon' Escherl]

(In reply to comment #6)
> Actually I was wondering why panelIsOpen exists in the first place.

From jdolske's review in bug 561636 comment 56:

> >+    // Hide the form invalid popup.
> >+    document.getElementById('invalid-form-popup').hidePopup();

> Hmm, I think you want to avoid doing all this work for every location change
> (especially since most won't need to actually do anything!).
I don't know, if that line is so much more expensive that the check is justified.

Comment 8

[[e-mail]]

(In reply to comment #7)
> (In reply to comment #6)
> > Actually I was wondering why panelIsOpen exists in the first place.
> From jdolske's review in bug 561636 comment 56:
> > >+    // Hide the form invalid popup.
> > >+    document.getElementById('invalid-form-popup').hidePopup();
> > Hmm, I think you want to avoid doing all this work for every location change
> > (especially since most won't need to actually do anything!).
> I don't know, if that line is so much more expensive that the check is
> justified.
Well, we already unconditionally hide the context menu and tooltip each time the content location changes...

Comment 9

[[e-mail]]

Comment on attachment 480067 [details] [diff] [review]
show an error message on invalidformsubmit

>+Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
Oh, I see now, this gets included before nsContextMenu, and you need XPCOMUtils at top-level.

>+  QueryInterface : XPCOMUtils.generateQI([Components.interfaces.nsIFormSubmitObserver]),
Previous nit still not fixed. Also I forgot to point out the space before :

>+    // this.panel.firstChild.nodeValue = element.validationMessage.substring(0, 256);
Left over?

>+    // If the user type something or blur the element, we want to remove the popup.
Nit: "types something or blurs the element"

>+    // We could check for clicks but a click is already removing the popup.
"a click already removes the popup" (this is actually a common mistake amongst mediocre English speakers; it's unusual for English grammar to be harder!)

>+      <description />
Nit: this is XML, not HTML masquerading as XHTML, so no space before / ;-)

>+   // Hide the form invalid popup.
Nit: doesn't belong between the context menu and tooltip cases.

>+   if (gFormSubmitObserver.panelIsOpen()) {
>+     gFormSubmitObserver.panel.hidePopup();
>+   }
>+
>    if (document.tooltipNode) {
Ah, I see what you mean now, we check that there's a tooltip before hiding it. But if you wanted to do that then you might as well save the element so you can optimise hiding the panel in the case where you're not loading its window.

>+/* Invalid form popup */
>+#invalid-form-popup {
>+  -moz-appearance: tooltip;
>+  padding-top: 3px;
>+  font-weight: bold;
>+}
...
>+/* Invalid form popup */
>+#invalid-form-popup {
>+  padding-top: 3px;
>+  border: 1px solid #000000;
>+  background-color: #FFFFE7;
>+  font-weight: bold;
>+}
IMHO you should copy all of the rules, not just pick some that seem to work.

Comment 10

[Bruno 'Aqualon' Escherl]

(In reply to comment #9)
> >+  QueryInterface : XPCOMUtils.generateQI([Components.interfaces.nsIFormSubmitObserver]),
> Previous nit still not fixed. Also I forgot to point out the space before :
Ah, forgot to ask there. So this should be [Ci.nsIFormSubmitObserver, Ci.nsIObserver]?

> >+    // this.panel.firstChild.nodeValue = element.validationMessage.substring(0, 256);
> Left over?
Yes.

> >+   // Hide the form invalid popup.
> Nit: doesn't belong between the context menu and tooltip cases. 
> >+   if (gFormSubmitObserver.panelIsOpen()) {
> >+     gFormSubmitObserver.panel.hidePopup();
> >+   }
> >+
> >    if (document.tooltipNode) {
> Ah, I see what you mean now, we check that there's a tooltip before hiding it.
> But if you wanted to do that then you might as well save the element so you can
> optimise hiding the panel in the case where you're not loading its window.
Hm, the more I think about it, the less clear it gets, why we need this code at all. The only difference I've seen so far is that at the end of test_bug561636.html the popup is still shown without that code. But could there be a way, where the user triggers onLocationChange with the popup still shown?

> >+/* Invalid form popup */
> >+#invalid-form-popup {
> >+  -moz-appearance: tooltip;
> >+  padding-top: 3px;
> >+  font-weight: bold;
> >+}
> ...
> >+/* Invalid form popup */
> >+#invalid-form-popup {
> >+  padding-top: 3px;
> >+  border: 1px solid #000000;
> >+  background-color: #FFFFE7;
> >+  font-weight: bold;
> >+}
> IMHO you should copy all of the rules, not just pick some that seem to work.
Ok, I can do that for modern, even so I would prefer doing padding: 2px 3px 0px 3px there. But where do we have the CSS definition for classic? Or should I just copy the modern style, skip the border and the colors and use -moz-appearance: tooltip there?

Comment 11

[[e-mail]]

(In reply to comment #10)
> (In reply to comment #9)
> > >+  QueryInterface : XPCOMUtils.generateQI([Components.interfaces.nsIFormSubmitObserver]),
> > Previous nit still not fixed. Also I forgot to point out the space before :
> Ah, forgot to ask there. So this should be [Ci.nsIFormSubmitObserver,
> Ci.nsIObserver]?
Yes please.

> could there
> be a way, where the user triggers onLocationChange with the popup still shown?
Some sort of automatic refresh perhaps?

> But where do we have the CSS definition for classic?
Well I was considering copying from toolkit/themes/winstripe/global/popup.css although I checked and the other themes are very similar.

Comment 12

[Bruno 'Aqualon' Escherl]

Attachment: show an error message on invalidformsubmit v2

So, I hope I included all previous comments this time ;)

Comment 13

[[e-mail]]

Comment on attachment 480124 [details] [diff] [review]
show an error message on invalidformsubmit v2

>+    // We could check for clicks but a click already removs the popup.
Typo: removes

>+    if (gFormSubmitObserver.panelIsOpen()) {
[Still not convinced by this test. Never mind...]

Comment 14

[Ian Neal]

Comment on attachment 480124 [details] [diff] [review]
show an error message on invalidformsubmit v2

>+    this.panel = document.getElementById('invalid-form-popup');
Nit: elsewhere in this file, " is used rather than '

>+    // One event to bring them all and in the darkness bind them.
to bring them all together perhaps?

>diff --git a/suite/themes/classic/navigator/navigator.css b/suite/themes/classic/navigator/navigator.css
What about the mac navigator.css?
You'll needs stefanh to ui-review that change probably.
r=me with those points addressed.

Comment 15

[Justin Wood (:Callek)]

(In reply to comment #14)
> Comment on attachment 480124 [details] [diff] [review]
> show an error message on invalidformsubmit v2
> >+    // One event to bring them all and in the darkness bind them.
> to bring them all together perhaps?

Its a ref to "Lord of the Rings" a major Novel series by J.R.R. Tolkien and a three-part-motion-picture filmed for US Release. That said, I'm not sure if the ref is needed; but we know how much "Ghost-busters" played a role in early dev, so I'm not opposed

Comment 16

[Justin Wood (:Callek)]

Comment on attachment 480124 [details] [diff] [review]
show an error message on invalidformsubmit v2

Stefanh, how does this look on mac; does it need a formal mac-ui review or should we proceed with landing and do a followup for your mac nits?

Comment 17

[Stefan [:stefanh]]

(In reply to comment #16)
> Comment on attachment 480124 [details] [diff] [review]
> show an error message on invalidformsubmit v2
> Stefanh, how does this look on mac; does it need a formal mac-ui review or
> should we proceed with landing and do a followup for your mac nits?

Yes, I would say that I'll need to look at this. When there are changes to the mac navigator.css file, request r or ui-r from me.

Comment 18

[Bruno 'Aqualon' Escherl]

Comment on attachment 480124 [details] [diff] [review]
show an error message on invalidformsubmit v2

I'll address Ian's review comments and add CSS for the mac (forgot that we had a different file there). Requesting r? from Stefan for that patch then.

Comment 19

[Bruno 'Aqualon' Escherl]

Attachment: show an error message on invalidformsubmit

This includes the tooltip css from pinstripe without the margin and with added bold fonts. I also skipped the border that's there in classic and modern, since pinstripe didn't have this.

Comment 20

[Bruno 'Aqualon' Escherl]

(In reply to comment #15)
> (In reply to comment #14)
> > Comment on attachment 480124 [details] [diff] [review] [details]
> > show an error message on invalidformsubmit v2
> > >+    // One event to bring them all and in the darkness bind them.
> > to bring them all together perhaps?
> 
> Its a ref to "Lord of the Rings" a major Novel series by J.R.R. Tolkien and a
> three-part-motion-picture filmed for US Release. That said, I'm not sure if the
> ref is needed; but we know how much "Ghost-busters" played a role in early dev,
> so I'm not opposed
The full "quote" should probably adapt to something like "One Event to rule them all, One Event to find them, One Event to bring them all and in the darkness bind them." ;-)

Comment 21

[Stefan [:stefanh]]

Comment on attachment 480909 [details] [diff] [review]
show an error message on invalidformsubmit

I'm a bit unsure of where you found those rules, since they certainly don't match the pinstripe/browser.css rules. Anyway, it looks good ;-)

Just a comment on the rules:
+  font: message-box;

+  cursor: default;


You can remove those, since they don't change anything.

Comment 22

[Bruno 'Aqualon' Escherl]

Attachment: show an error message on invalidformsubmit v4

v4 includes the changes from Stefan's ui-review. Taking over the r/sr/ui-r flags. I wrote the initial reviewers in the patch comment.

(In reply to comment #21)
> I'm a bit unsure of where you found those rules, since they certainly don't
> match the pinstripe/browser.css rules. Anyway, it looks good ;-)
They are from the tooltip rule in global/popup.css, like I did for win/linux classic.

Comment 23

[Bruno 'Aqualon' Escherl]

Comment on attachment 480954 [details] [diff] [review]
show an error message on invalidformsubmit v4

I'm not sure if we should land this, before the 2.1b1 code freeze, so setting approval-seamonkey2.1b1?

Comment 24

[Robert Kaiser]

Comment on attachment 480954 [details] [diff] [review]
show an error message on invalidformsubmit v4

Yes, you can get this landed, no string changes, so looks good.

Comment 25

[Karsten Düsterloh]

Pushed http://hg.mozilla.org/comm-central/rev/3c80a5c9aec3 onto trunk.

Comment 26

[Bruno 'Aqualon' Escherl]

The tests in test_bug561636.html pass too on all tinderboxes with that patch.

Comment 27

[[e-mail]]

We need to port bug 606817...

Comment 28

[[e-mail]]

Comment on attachment 480954 [details] [diff] [review]
show an error message on invalidformsubmit v4

>+    function eventHandler() {
>+      gFormSubmitObserver.panel.hidePopup();
>+    };
Bah, I totally overlooked the superfluous semicolon :-(