Closed Bug 1534930 Opened 3 years ago Closed 3 years ago

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

Categories

(Thunderbird :: General, task)

task
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 68.0

People

(Reporter: mkmelin, Assigned: pmorris)

References

Details

Attachments

(3 files, 8 obsolete files)

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?

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

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

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.

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

Attachment #9050769 - Flags: review?(mkmelin+mozilla)
Attached patch textbox-de-xbl-numberbox-1.patch (obsolete) — Splinter Review

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

Attachment #9050771 - Flags: review?(mkmelin+mozilla)
Attached patch textbox-de-xbl-textbox-2.patch (obsolete) — Splinter Review

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.

Attachment #9050777 - Flags: review?(mkmelin+mozilla)

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.

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

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)

Attached patch textbox-de-xbl-3.patch (obsolete) — Splinter Review

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

Attachment #9050769 - Attachment is obsolete: true
Attachment #9050771 - Attachment is obsolete: true
Attachment #9050777 - Attachment is obsolete: true
Attachment #9050769 - Flags: review?(mkmelin+mozilla)
Attachment #9050771 - Flags: review?(mkmelin+mozilla)
Attachment #9050777 - Flags: review?(mkmelin+mozilla)
Attachment #9051156 - Flags: review?(mkmelin+mozilla)
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
Attachment #9051156 - Flags: review?(mkmelin+mozilla) → feedback+

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

Do you have any pointers?

Attached patch textbox-de-xbl-textbox-3.patch (obsolete) — Splinter Review

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.

Attachment #9051156 - Attachment is obsolete: true
Attachment #9051383 - Flags: ui-review?(richard.marti)
Attachment #9051383 - Flags: review?(mkmelin+mozilla)

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

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;

Attached patch textbox-de-xbl-4.patch (obsolete) — Splinter Review

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

Attachment #9051383 - Attachment is obsolete: true
Attachment #9051383 - Flags: ui-review?(richard.marti)
Attachment #9051383 - Flags: review?(mkmelin+mozilla)
Attachment #9051502 - Flags: ui-review?(richard.marti)
Attachment #9051502 - Flags: review?(mkmelin+mozilla)
Attached patch textbox-de-xbl-textbox-5.patch (obsolete) — Splinter Review

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?

Attachment #9051510 - Flags: review?(mkmelin+mozilla)
Attachment #9051510 - Flags: feedback?(paul)
Comment on attachment 9051502 [details] [diff] [review]
textbox-de-xbl-4.patch

Removing the reviews until we see if -5 is what we want.
Attachment #9051502 - Flags: ui-review?(richard.marti)
Attachment #9051502 - Flags: review?(mkmelin+mozilla)
Attached patch textbox-de-xbl-textbox-5.patch (obsolete) — Splinter Review

Updated to tip.

Attachment #9051510 - Attachment is obsolete: true
Attachment #9051510 - Flags: review?(mkmelin+mozilla)
Attachment #9051510 - Flags: feedback?(paul)
Attachment #9051535 - Flags: review?(mkmelin+mozilla)
Attachment #9051535 - Flags: feedback?(paul)

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.

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

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 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
Attachment #9051535 - Flags: review?(mkmelin+mozilla)
Attachment #9051535 - Flags: review+
Attachment #9051535 - Flags: feedback?(paul)

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

Status: NEW → ASSIGNED
Keywords: checkin-needed
Attachment #9051502 - Attachment is obsolete: true

(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

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

I don't suppose this should be needed

Yes, removed.

Attachment #9051535 - Attachment is obsolete: true
Attachment #9051931 - Flags: review+

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

Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 68.0
Depends on: 1533702

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

Depends on: 1536745

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

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.

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

Flags: needinfo?(paul)
Flags: needinfo?(mkmelin+mozilla)

Filed bug 1543330.

Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(paul)
Type: enhancement → task
Regressions: 1583098
You need to log in before you can comment on or make changes to this bug.