626834 - Port |Bug 624151 - Better positioning for the invalid form popup| to SeaMonkey

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, enhancement)

Comment 1

[Edmund Wong (:ewong)]

Attachment: Position the invalid form popup better. (v1)

Comment 2

[[e-mail]]

Comment on attachment 606508 [details] [diff] [review]
Position the invalid form popup better. (v1)

>+    // We want to show the popup at the middle of checkbox and radio buttons
We have a different style of popup so it doesn't make sense to do this. But if it did, your patch would still have been wrong, not only because you didn't realise that the attached patch wasn't the one that was checked in, but because of the following nits:

>+    let offset = 0;
Nit: file style is to use var

>+    let position = "";
You always overwrite position, so no point setting it here.
Or, set it to one of the values here instead of inside the if.

>+    if (element.tagName == 'INPUT' &&
Don't need to check the tag name, only input can be radio or checkbox type.

Comment 3

[[e-mail]]

Comment on attachment 606508 [details] [diff] [review]
Position the invalid form popup better. (v1)

>+    let style = element.ownerDocument.defaultView.getComputedStyle(element, null);
Nit: we actually get the ownerDocument.defaultView twice, which we could in theory avoid, but not a biggie.

>+        (element.type == 'radio' || element.type == 'checkbox'))
This doesn't work with our popup.

>diff --git a/suite/common/utilityOverlay.js b/suite/common/utilityOverlay.js
Don't need to change this file at all, it is an unrelated popup.

Comment 4

[Edmund Wong (:ewong)]

Attachment: Better position the invalid form popup. (v2)

Comment 5

[[e-mail]]

Comment on attachment 607172 [details] [diff] [review]
Better position the invalid form popup. (v2)

>+    var position = "after_pointer";
Where does this come from?

>+      offset = parseInt(style.paddingLeft) + parseInt(style.borderLeftWidth);
Still using the code that was in the patch on the bug, rather than the code that eventually got checked in...

Comment 6

[Edmund Wong (:ewong)]

(In reply to [e-mail] from comment #5)
> Comment on attachment 607172 [details] [diff] [review]
> Better position the invalid form popup. (v2)
> 
> >+    var position = "after_pointer";
> Where does this come from?

I got this from http://mxr.mozilla.org/comm-central/source/mozilla/layout/xul/base/public/nsIPopupBoxObject.idl#114.  
The code that actually came with the CnP, I didn't see it on the list of 
available options.

> 
> >+      offset = parseInt(style.paddingLeft) + parseInt(style.borderLeftWidth);
> Still using the code that was in the patch on the bug, rather than the code
> that eventually got checked in...

Sorry..  will fix that.

Comment 7

[Edmund Wong (:ewong)]

Attachment: Position the invalid form popup better. (v3)

Comment 8

[[e-mail]]

(In reply to Edmund Wong from comment #6)
> The code that actually came with the CnP, I didn't see it on the list of 
> available options.
The list in that file is out of date... also after_pointer doesn't really work, it's the same as overlap but with a 21 pixel offset.

Comment 9

[[e-mail]]

Comment on attachment 607442 [details] [diff] [review]
Position the invalid form popup better. (v3)

>+    var position = "after_pointer";
>+    let style = element.ownerDocument.defaultView.getComputedStyle(element, null);
>+
>+    if (element.type != 'radio' && element.type != 'checkbox')
>+    {
>+      position = "after_start";
>+    }
>+
>+    this.panel.openPopup(element, position, offset, 0);  }
So, I don't see why we shouldn't stick with "after_start" for the position (in which case it no longer needs a variable). Also the offset for a radio or checkbox always turns out to be zero so we don't need to special-case it.

>+      if (style.direction == 'rtl')
>+      {
>+        offset = parseInt(style.paddingRight) + parseInt(style.borderRightWidth);
>+      }
>+      else
>+      {
>+        offset = parseInt(style.paddingLeft) + parseInt(style.borderLeftWidth);
>+      }
I'm not keen on this brace style, you could remove them, or put them on the same line, or possibly even switch to ?: instead.

Comment 10

[Edmund Wong (:ewong)]

Attachment: Position the invalid form popup better. (v4)

Comment 11

[[e-mail]]

Comment on attachment 609624 [details] [diff] [review]
Position the invalid form popup better. (v4)

>+    // We want to show the popup at the middle of checkbox and radio buttons
>+    // and where the content begin for the other elements.
Hmm, this comment doesn't make sense any more, might as well remove it, unless you can come up with a better one.

>+    var offset = 0;
Don't need this here, just use "var offset =" below instead.

>+    let style = element.ownerDocument.defaultView.getComputedStyle(element, null);
Nit: var please.

>+    offset = (style.direction == 'rtl') ? parseInt(style.paddingRight) + 
Nit: don't need the ()s around the direction check.

>-  }
>+    this.panel.openPopup(element, "after_start", offset, 0);  }
Oops, you accidentally joined the } on to the previous line!

r=me with the above fixed.

Comment 12

[Edmund Wong (:ewong)]

Attachment: Position the invalid form popup better. (v5)

Nits fixed.

Comment 13

[Edmund Wong (:ewong)]

Attachment: Position the invalid form popup better. (v6)

Fixed all nits.

Comment 14

[Edmund Wong (:ewong)]

Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/f7d0066f06b1