Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED in seamonkey2.11

Status

SeaMonkey
UI Design
--
enhancement
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: sgautherie, Assigned: ewong)

Tracking

Trunk
seamonkey2.11
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 5 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

5 years ago
Created attachment 606508 [details] [diff] [review]
Position the invalid form popup better. (v1)
Assignee: nobody → ewong
Status: NEW → ASSIGNED
Attachment #606508 - Flags: review?(neil)

Comment 2

5 years ago
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

5 years ago
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.
Attachment #606508 - Flags: review?(neil) → review-
(Assignee)

Comment 4

5 years ago
Created attachment 607172 [details] [diff] [review]
Better position the invalid form popup. (v2)
Attachment #606508 - Attachment is obsolete: true
Attachment #607172 - Flags: review?(neil)

Comment 5

5 years ago
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...
(Assignee)

Comment 6

5 years ago
(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.
(Assignee)

Comment 7

5 years ago
Created attachment 607442 [details] [diff] [review]
Position the invalid form popup better. (v3)
Attachment #607172 - Attachment is obsolete: true
Attachment #607442 - Flags: review?(neil)
Attachment #607172 - Flags: review?(neil)

Comment 8

5 years ago
(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

5 years ago
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.
Attachment #607442 - Flags: review?(neil) → review-
(Assignee)

Comment 10

5 years ago
Created attachment 609624 [details] [diff] [review]
Position the invalid form popup better. (v4)
Attachment #607442 - Attachment is obsolete: true
Attachment #609624 - Flags: review?(neil)
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.
Attachment #609624 - Flags: review?(neil) → review+
(Assignee)

Comment 12

5 years ago
Created attachment 611377 [details] [diff] [review]
Position the invalid form popup better. (v5)

Nits fixed.
Attachment #609624 - Attachment is obsolete: true
Attachment #611377 - Flags: review+
(Assignee)

Comment 13

5 years ago
Created attachment 611379 [details] [diff] [review]
Position the invalid form popup better. (v6)

Fixed all nits.
Attachment #611377 - Attachment is obsolete: true
Attachment #611379 - Flags: review+
(Assignee)

Comment 14

5 years ago
Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/f7d0066f06b1
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Reporter)

Updated

5 years ago
No longer blocks: 467530
Target Milestone: --- → seamonkey2.11

Updated

5 years ago
Blocks: 787189
You need to log in before you can comment on or make changes to this bug.