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

RESOLVED FIXED in Thunderbird 61.0

Status

enhancement
RESOLVED FIXED
Last year
Last year

People

(Reporter: jorgk, Assigned: Paenglab)

Tracking

unspecified
Thunderbird 61.0
Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird60 fixed, thunderbird61 fixed)

Details

Attachments

(2 attachments, 4 obsolete attachments)

+++ 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
https://hg.mozilla.org/mozilla-central/rev/81891dee83da

Richard, do you know how to "restore the old binding"?
Posted 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
Status: NEW → ASSIGNED
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, https://hg.mozilla.org/comm-central/rev/4d405d2392d2 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+
Posted 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.
Posted 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]
numberbox-RM.patch

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)
Remove the spinbuttons on port fields.
Attachment #8967875 - Flags: review?(jorgk)
Comment on attachment 8967872 [details] [diff] [review]
1453322-part-2-numberbox.patch

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

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 mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/6dc1e432a9cf
Part 1: revert bug 1453233. r=jorgk
https://hg.mozilla.org/comm-central/rev/af3e3f9e3db1
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.
Status: ASSIGNED → RESOLVED
Closed: Last year
Keywords: leave-open
Resolution: --- → FIXED
Attachment #8967872 - Flags: approval-comm-beta?
Comment on attachment 8967872 [details] [diff] [review]
1453322-part-2-numberbox.patch

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.