Closed Bug 1453322 Opened 3 years ago Closed 3 years ago

Restore spinbuttons for number fields after their removal in bug 1453233 following them getting broken by bug 1437302.


(Thunderbird :: Theme, enhancement)

Not set


(thunderbird60 fixed, thunderbird61 fixed)

Thunderbird 61.0
Tracking Status
thunderbird60 --- fixed
thunderbird61 --- fixed


(Reporter: jorgk-bmo, Assigned: Paenglab)




(2 files, 4 obsolete files)

+++ This bug was initially created as a clone of Bug #1453233 +++

We removed the spinbuttons from number fields (proxy port numbers, cache size, various wait intervals) in bug 1453233 because since bug 1437302 they don't work any more.

History: Removed from M-C in bug 1429573 on 10th Feb and restored in TB in March in bug 1446329.

Tim any tips on how to get them to work again? The placement is odd, which could be fixed via CSS, and clicking onto the arrows doesn't work any more.
Flags: needinfo?(ntim.bugs)
In the inspector I set hidespinbuttos to false in FF's port settings and there they don't work any more either.
Depends on: 1453371
From IRC:
17:23:28 - ntim: jorgk: yeah, I switched[1] to using the builtin spin buttons used by input[type=number]
17:24:29 - ntim: jorgk: I must be guessing it's a bug with the interaction between XUL flexbox and the input type=number layout code
17:32:14 - ntim: jorgk: I think your best bet might be restoring the old binding in comm-central for now
17:32:57 - ntim: jorgk: since we no longer use the spinbuttons feature on the Firefox side

[1] So I think the "switching" refers to

Richard, do you know how to "restore the old binding"?
Attached patch numberbox.patch (obsolete) — Splinter Review
This patch restores the spinbuttons. I let them hidden on port fields.

I tried the patch on all platforms and it looks like before.

Jörg, please check if I catched every field.
Assignee: nobody → richard.marti
Attachment #8967843 - Flags: review?(jorgk)
Let's put them back on the ports as well for consistency.
You can argue that if you use a (private) proxy, you might just go to the adjacent port.
Flags: needinfo?(ntim.bugs)
Then you can use the keyboard up/down arrows.
FYI, added hidespinbuttons="true" 22 times, so I guess you'll have to take it out 22 times again :-(
But what's the point to have some numbers with the spinbuttons and others without? You can *always* use the arrows, so why are we fixing this bug?
There are 65000 ports and when you want go through all of them with the spinbuttons it lasts a bit long. For the minutes and second fields it's okay  as there are not so much different numbers.

Then we are looking also consistent with the FX proxy dialog.
I think it is better to split this in two parts. Part 1: Backout of bug 1453233.
Attachment #8967843 - Attachment is obsolete: true
Attachment #8967843 - Flags: review?(jorgk)
Attachment #8967860 - Flags: review+
Attached patch numberbox-bmo.patch (obsolete) — Splinter Review
Then, in a part 2, we do the "new" stuff.

If you do it this way, you will notice that you forgot to revert a hunk in imAccountOptionsHelper.js.

I didn't carry the hidespinbuttons="true" here, so it you want them, please add them here.
Attached patch numberbox-RM.patch (obsolete) — Splinter Review
Wrong patch, sorry, here the one rebased on top of the backout.
Attachment #8967861 - Attachment is obsolete: true
(In reply to Jorg K (GMT+1) from comment #10)
> If you do it this way, you will notice that you forgot to revert a hunk in
> imAccountOptionsHelper.js.

It wasn't forgotten. This is also a port field.
That will also make the beta uplift easier, since I haven't uplifted bug 1453233.
In fact, we should move the removal of the spinbuttons to part 3.
Comment on attachment 8967862 [details] [diff] [review]

I checked with both patches applied and the number fields look good, apart from the port number on the SMTP edit panel. Maybe I messed that up, although I folded the two patches together and got mostly your patch apart from the spinbutton removal from the ports.
Sorry, that's a port number, so without special tricks the spinbuttons won't work. And there are no special tricks, since you wanted to remove the spinbuttons there in the first place. So part 3 will take care of that.
Port field in SMTP Edit fixed.
Attachment #8967862 - Attachment is obsolete: true
Attachment #8967872 - Flags: review?(jorgk)
Attached patch 1453322-part-3-hidebuttons.patch (obsolete) — Splinter Review
Remove the spinbuttons on port fields.
Attachment #8967875 - Flags: review?(jorgk)
Comment on attachment 8967872 [details] [diff] [review]

Yes, that works.
Attachment #8967872 - Flags: review?(jorgk) → review+
Comment on attachment 8967875 [details] [diff] [review]

I'm not a friend of this, so I would not land this. If you really want to remove port numbers, you need to do it everywhere: There are NNTP ports, IMAP ports and POP ports.

If it's really important to be in sync with FF here, perhaps only remove the proxy ports, although they are most likely the more useful ones.
Attachment #8967875 - Flags: review?(jorgk)
Keywords: leave-open
Pushed by
Part 1: revert bug 1453233. r=jorgk
Part 2: Add numberbox and textbox bindings to restore spinbuttons for number fields. r=jorgk
Target Milestone: --- → Thunderbird 61.0
We are enough out of sync to FX to not need to remove the spinbuttons for port fields.
Closed: 3 years ago
Keywords: leave-open
Resolution: --- → FIXED
Attachment #8967872 - Flags: approval-comm-beta?
Comment on attachment 8967872 [details] [diff] [review]

I hope it applies.
Attachment #8967872 - Flags: approval-comm-beta? → approval-comm-beta+
Attachment #8967875 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.