1534930 - [de-xbl] get rid of the forked common bindings related to textbox.xml: textbox, numberbox, spinbuttons

Status: RESOLVED FIXED

(Thunderbird :: General, task)

Description

[Magnus Melin [:mkmelin]]

We need to get rid of a bunch of the forked bindings ASAP. I was looking at bug 1534913 which is going land after the soft freeze (so likely early next week).

Trying out the patch I get into problems and it looks like it's due to the forked textbox stuff...

https://searchfox.org/comm-central/source/common/bindings/textbox.xml
https://searchfox.org/comm-central/source/common/bindings/spinbuttons.xml
https://searchfox.org/comm-central/source/common/bindings/numberbox.xml

I don't think any of the forking is ultimately needed, and was forked just due to rush. We lose some spinbuttons on numbers and such, but that was never really a very useful functionality if you ask me.

There are also a bunch of other textbox de-xbl work coming from m-c (likely to html:input within a month) coming so we need to move now not to get hit in the next two weeks.

Paul, could you take this on as a priority?

Comment 1

[Richard Marti (:Paenglab)]

(In reply to Magnus Melin [:mkmelin] from comment #0)

We lose some spinbuttons on numbers and such, but that was never really a very useful functionality if you ask me.

I'm against it. Spinbuttons are useful for fields where you set a timeout (for example how long a dialog should be shown) Then you can simply de-/increase instead of erasing the entry and typing the new value.

Comment 2

[Magnus Melin [:mkmelin]]

The timeout value is perhaps the only such occurrence...

In general, I don't think it's a common use case to want to switch the value one-by-one. Almost always when you do that you want to change perhaps 5->10 or something, typing is way more efficient.

That said, there are no spinbuttons on xul textboxes, but once we move over to <html:input type="number"> in a few weeks, there are spinbuttons again in the default implementation - but maybe some internal way to hide them should we want to. https://www.w3schools.com/tags/tryit.asp?filename=tryhtml5_input_step

Comment 3

[Paul Morris [:pmorris]]

I'm on it. Would it make sense to go ahead and move to <html:input type="number"> while we're at it? I'll look into it as I work on this. Let me know if there's a reason not to.

Comment 4

[Paul Morris [:pmorris]]

Attachment: textbox-de-xbl-spinbuttons-0.patch

First of three patches. Separate patches to make review easier, can combine them before landing if needed. This one removes the spinbuttons.

Comment 5

[Paul Morris [:pmorris]]

Attachment: textbox-de-xbl-numberbox-1.patch

Second of three patches. Separate patches to make review easier, can combine them before landing if needed. This one removes the numberbox.

Comment 6

[Paul Morris [:pmorris]]

Attachment: textbox-de-xbl-textbox-2.patch

Third of three patches. Separate patches to make review easier, can combine them before landing if needed. This one removes our forked version of textbox.

With these three patches applied I was able to build and didn't see any errors in the console or notice anything wrong. Need to do a try server build and/or run mozmill tests locally.

There are 50 or so instances of <textbox type="number">. I tried converting the "percent complete" one in calendar task editing dialog to <html:input type="number">, and it appeared to work as expected. (It was a little taller than the input fields next to it.)

The HTML input supports 'max' and 'min' attributes:
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/number

I see some other attributes like "disable-if-readonly", which seem to be custom? I couldn't find anything about that one by searching for it.

So I've held off on converting to <html:input type="number"> for now, but since we're heading that way soon, it might be worth going ahead and doing it in a follow-up bug. That would restore the spin button functionality, maybe before anyone misses it.

Comment 7

[Magnus Melin [:mkmelin]]

True that on one hand moving to html:input straight away is one way to go, but maybe still preferable to do it one step at the time and wait for m-c to be ready. They are still sorting out the final things they want to fix before pulling through with the switchover - follow bug 1513325.

Comment 8

[Paul Morris [:pmorris]]

That makes sense, there's already enough changes in these patches. Here's a try server run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed4473e2c97aea06ebaa2a59601c71ae04fcfe72

Comment 9

[Magnus Melin [:mkmelin]]

Looks pretty ok, but the patches don't apply. I think you can just fold them into one. (hg qfold if you're using mq)

Comment 10

[Paul Morris [:pmorris]]

Attachment: textbox-de-xbl-3.patch

Hmm, sorry about that. I've now merged them into one patch and rebased on the most recent commit.

Comment 11

[Magnus Melin [:mkmelin]]

Comment on attachment 9051156 [details] [diff] [review]
textbox-de-xbl-3.patch

Review of attachment 9051156 [details] [diff] [review]:
-----------------------------------------------------------------

Ah so the spinbuttons are there in xul after all..
I think you got most of it. The numberbox.css is still there, but maybe not used? 

The fields look more beefy than before, so we should adjust the css for them

Comment 12

[Richard Marti (:Paenglab)]

(In reply to Magnus Melin [:mkmelin] from comment #11)

The fields look more beefy than before, so we should adjust the css for them

Which is not so simple as it's HTML flexbox inside of XUL.

Comment 13

[Magnus Melin [:mkmelin]]

Do you have any pointers?

Comment 14

[Paul Morris [:pmorris]]

Attachment: textbox-de-xbl-textbox-3.patch

Okay, I should have checked this more closely before. I've now:

• renamed 3 numberbox.css files to textbox.css, but kept them so we can adjust the styles for textbox[type="number"]
• removed styles that were causing problems (-moz-appearance: none;)

(Note, I'm on linux, so I can't really tell what things look like on osx or windows.)

I tried to adjust the CSS to reduce the height of the number textbox, but didn't have much luck. Setting the padding of textbox[type="number"] to 0 helps, but they are still about 4px too tall.

The problem is the height of the spinbuttons which "pushes out" and increases the height of their container(s). And they and the other contents of the html:input are shadow dom so can't be styled.

I tried changing the element to "html:input" and was able to fix the height then, so I'd suggest filing a follow-up bug to fix this once we've converted these to html:input.

Comment 15

[Paul Morris [:pmorris]]

Attachment: Screenshot from 2019-03-15 15-33-16.png

Screenshot of 4px too tall on linux. This is what patch "3" does. The best I could get it with reasonable effort. (This is from the new task dialog.)

Comment 16

[Paul Morris [:pmorris]]

Attachment: Screenshot from 2019-03-15 15-42-57.png

This is just a proof-of-concept that the height was fixable when the element was "html:input". There would be more to do of course. This was with the following styles on the textbox[type="number"]:

padding: 1px !important;
border: 1px solid #999;
border-radius: 4px;

Comment 17

[Paul Morris [:pmorris]]

Attachment: textbox-de-xbl-4.patch

Fixed indentation of textbox.css in the jar.mn files and added the provisional first pass <html:input> styles from comment 16 to the linux textbox.css file (commented out). (Thanks to Richard for the heads-up on these things.)

Comment 18

[Richard Marti (:Paenglab)]

Attachment: textbox-de-xbl-textbox-5.patch

I played a bit and this is what I think is like before on all platforms. Also the spinbuttons in the prefs look like before.

Paul what do you think?

Comment 19

[Richard Marti (:Paenglab)]

Comment on attachment 9051502 [details] [diff] [review]
textbox-de-xbl-4.patch

Removing the reviews until we see if -5 is what we want.

Comment 20

[Richard Marti (:Paenglab)]

Attachment: textbox-de-xbl-textbox-5.patch

Updated to tip.

Comment 21

[Paul Morris [:pmorris]]

I took a look and tried Richard's patch. Code changes look fine to me. Here on Ubuntu Linux, the spinbuttons in the prefs look good. In the task edit dialog the textbox is still slightly taller (about 4px). It looks the same as in screenshot I posted. I didn't see the new styles for the spinbuttons being applied in the inspector. I checked an event snooze popup and it looked slightly larger there as well (same 4px).

Since the height difference is pretty subtle, we could fix it in a follow-up bug, if we wanted to go ahead and land this ahead of moz-central changes.

Comment 22

[Richard Marti (:Paenglab)]

(In reply to Paul Morris [:pmorris] from comment #21)

I took a look and tried Richard's patch. Code changes look fine to me. Here on Ubuntu Linux, the spinbuttons in the prefs look good. In the task edit dialog the textbox is still slightly taller (about 4px).

Here it isn't taller, is the padding: 0; applied on your textbox? The spinbuttons aren't completely on the right because I still use the system -moz-appearance and this uses a border 6px wide. If we would use the -moz-appearance: none; it would be almost impossible to style the box like the system on all Linux themes (like gradients and different border-radius).

Comment 23

[Paul Morris [:pmorris]]

Yes, the following is applied to the textbox, via chrome://messenger/skin/textbox.css

textbox[type="number"] {
    padding: 0 !important;
}

Agreed that we don't want to change the default system -moz-appearance borders. The inspector says the height of the spinbuttons container div is 26px, which is causing the textbox to be bigger than the other ones. I just tried to reduce its height (via -moz-number-spin-box, -moz-number-spin-up, etc.), but I was not able to make that work. (My changes either had no effect or the height shrank further than I wanted and changes to the amount didn't change the size, probably due to something with flex box.)

Comment 24

[Magnus Melin [:mkmelin]]

Comment on attachment 9051535 [details] [diff] [review]
textbox-de-xbl-textbox-5.patch

Review of attachment 9051535 [details] [diff] [review]:
-----------------------------------------------------------------

The commit message could say "... bindings".

Anyway, I think this is good to go now.

::: mail/base/content/bindings.css
@@ +1,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  @import url("chrome://global/skin/textbox.css");

I don't suppose this should be needed

@@ +2,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  @import url("chrome://global/skin/textbox.css");
> +@import url("chrome://messenger/skin/textbox.css");

and this should be imported directly instead of through bindings.css

But, that's just cleanup and we can do it later

Comment 25

[Magnus Melin [:mkmelin]]

(Usually when the bug has a patch -> ASSIGNED)

Comment 26

[Richard Marti (:Paenglab)]

Attachment: textbox-de-xbl-textbox-5.patch

(In reply to Magnus Melin [:mkmelin] from comment #24)

Comment on attachment 9051535 [details] [diff] [review]
textbox-de-xbl-textbox-5.patch

Review of attachment 9051535 [details] [diff] [review]:

The commit message could say "... bindings".

Updated.

Anyway, I think this is good to go now.

::: mail/base/content/bindings.css
@@ +1,5 @@

/* This Source Code Form is subject to the terms of the Mozilla Public

• License, v. 2.0. If a copy of the MPL was not distributed with this
• file, You can obtain one at http://mozilla.org/MPL/2.0/. */

@import url("chrome://global/skin/textbox.css");

I don't suppose this should be needed

Yes, removed.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/87875f8dae87
[de-xbl] Remove the spinbuttons, numberbox and textbox bindings. r=mkmelin

Comment 28

[Jorg K (CEST = GMT+2)]

This caused two regressions, bug 1533702 and bug 1536745. No one checked the try run from comment #8:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed4473e2c97aea06ebaa2a59601c71ae04fcfe72

Hmm, next time please push to try-comm-central. Pushing patches to M-C's try won't do any good :-(

Comment 29

[Paul Morris [:pmorris]]

Ah, duly noted. Clearly I need to get more up to speed on how to do try server runs.

Comment 30

[Jorg K (CEST = GMT+2)]

We discussed that on IRC at length: https://mozilla.logbot.info/maildev/20190314#c16090000

Comment 31

[Paul Morris [:pmorris]]

Yes, thanks, I have the discussion in my notes. I had already pushed the try run in comment #8 before most of that discussion. I thought I had pushed it to try-comm-central since that's what is in my hgrc file. Next time I will make sure the right thing happens.

Comment 32

[alta88]

Does this mean all the bindings (and worse, preprocessing) added in Bug 1490431 and elsewhere should be removed?

Comment 33

[Magnus Melin [:mkmelin]]

Filed bug 1543330.