Last Comment Bug 626834 - Port |Bug 624151 - Better positioning for the invalid form popup| to SeaMonkey
: Port |Bug 624151 - Better positioning for the invalid form popup| to SeaMonkey
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: UI Design (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: seamonkey2.11
Assigned To: Edmund Wong (:ewong)
:
Mentors:
http://mxr.mozilla.org/comm-central/s...
Depends on: 624151
Blocks: 787189
  Show dependency treegraph
 
Reported: 2011-01-18 15:21 PST by Serge Gautherie (:sgautherie)
Modified: 2012-08-30 12:55 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Position the invalid form popup better. (v1) (2.15 KB, patch)
2012-03-16 02:54 PDT, Edmund Wong (:ewong)
neil: review-
Details | Diff | Review
Better position the invalid form popup. (v2) (1.53 KB, patch)
2012-03-19 08:18 PDT, Edmund Wong (:ewong)
no flags Details | Diff | Review
Position the invalid form popup better. (v3) (1.16 KB, patch)
2012-03-19 21:21 PDT, Edmund Wong (:ewong)
neil: review-
Details | Diff | Review
Position the invalid form popup better. (v4) (1.07 KB, patch)
2012-03-26 22:50 PDT, Edmund Wong (:ewong)
neil: review+
Details | Diff | Review
Position the invalid form popup better. (v5) (954 bytes, patch)
2012-04-02 00:54 PDT, Edmund Wong (:ewong)
ewong: review+
Details | Diff | Review
Position the invalid form popup better. (v6) (946 bytes, patch)
2012-04-02 00:55 PDT, Edmund Wong (:ewong)
ewong: review+
Details | Diff | Review

Description Serge Gautherie (:sgautherie) 2011-01-18 15:21:27 PST

    
Comment 1 Edmund Wong (:ewong) 2012-03-16 02:54:24 PDT
Created attachment 606508 [details] [diff] [review]
Position the invalid form popup better. (v1)
Comment 2 neil@parkwaycc.co.uk 2012-03-16 06:09:33 PDT
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 neil@parkwaycc.co.uk 2012-03-16 06:11:28 PDT
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) 2012-03-19 08:18:27 PDT
Created attachment 607172 [details] [diff] [review]
Better position the invalid form popup. (v2)
Comment 5 neil@parkwaycc.co.uk 2012-03-19 08:46:50 PDT
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) 2012-03-19 18:45:41 PDT
(In reply to neil@parkwaycc.co.uk 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) 2012-03-19 21:21:32 PDT
Created attachment 607442 [details] [diff] [review]
Position the invalid form popup better. (v3)
Comment 8 neil@parkwaycc.co.uk 2012-03-21 06:46:43 PDT
(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 neil@parkwaycc.co.uk 2012-03-21 06:52:46 PDT
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) 2012-03-26 22:50:26 PDT
Created attachment 609624 [details] [diff] [review]
Position the invalid form popup better. (v4)
Comment 11 neil@parkwaycc.co.uk 2012-03-27 05:44:25 PDT
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) 2012-04-02 00:54:35 PDT
Created attachment 611377 [details] [diff] [review]
Position the invalid form popup better. (v5)

Nits fixed.
Comment 13 Edmund Wong (:ewong) 2012-04-02 00:55:56 PDT
Created attachment 611379 [details] [diff] [review]
Position the invalid form popup better. (v6)

Fixed all nits.
Comment 14 Edmund Wong (:ewong) 2012-04-02 01:45:46 PDT
Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/f7d0066f06b1

Note You need to log in before you can comment on or make changes to this bug.