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

Status: RESOLVED FIXED

(Thunderbird :: Theme, enhancement)

Description

[Jorg K (CEST = GMT+2)]

+++ 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.

Comment 1

[Jorg K (CEST = GMT+2)]

In the inspector I set hidespinbuttos to false in FF's port settings and there they don't work any more either.

Comment 2

[Jorg K (CEST = GMT+2)]

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"?

Comment 3

[Richard Marti (:Paenglab)]

Attachment: numberbox.patch

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.

Comment 4

[Jorg K (CEST = GMT+2)]

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.

Comment 5

[Richard Marti (:Paenglab)]

Then you can use the keyboard up/down arrows.

Comment 6

[Jorg K (CEST = GMT+2)]

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 :-(

Comment 7

[Jorg K (CEST = GMT+2)]

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?

Comment 8

[Richard Marti (:Paenglab)]

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.

Comment 9

[Jorg K (CEST = GMT+2)]

Attachment: 1453322-part-1-reversal.patch

I think it is better to split this in two parts. Part 1: Backout of bug 1453233.

Comment 10

[Jorg K (CEST = GMT+2)]

Attachment: numberbox-bmo.patch

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.

Comment 11

[Jorg K (CEST = GMT+2)]

Attachment: numberbox-RM.patch

Wrong patch, sorry, here the one rebased on top of the backout.

Comment 12

[Richard Marti (:Paenglab)]

(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.

Comment 13

[Jorg K (CEST = GMT+2)]

That will also make the beta uplift easier, since I haven't uplifted bug 1453233.

Comment 14

[Jorg K (CEST = GMT+2)]

In fact, we should move the removal of the spinbuttons to part 3.

Comment 15

[Jorg K (CEST = GMT+2)]

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.

Comment 16

[Jorg K (CEST = GMT+2)]

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.

Comment 17

[Richard Marti (:Paenglab)]

Attachment: 1453322-part-2-numberbox.patch

Port field in SMTP Edit fixed.

Comment 18

[Richard Marti (:Paenglab)]

Attachment: 1453322-part-3-hidebuttons.patch

Remove the spinbuttons on port fields.

Comment 19

[Jorg K (CEST = GMT+2)]

Comment on attachment 8967872 [details] [diff] [review]
1453322-part-2-numberbox.patch

Yes, that works.

Comment 20

[Jorg K (CEST = GMT+2)]

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.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
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

Comment 22

[Richard Marti (:Paenglab)]

We are enough out of sync to FX to not need to remove the spinbuttons for port fields.

Comment 23

[Jorg K (CEST = GMT+2)]

Comment on attachment 8967872 [details] [diff] [review]
1453322-part-2-numberbox.patch

I hope it applies.

Comment 24

[Jorg K (CEST = GMT+2)]

Beta (TB 60 beta 4):
https://hg.mozilla.org/releases/comm-beta/rev/f51daa333cb3713bfdccb6e7be5c020b05208f15