[de-xbl] get rid of the forked common bindings related to textbox.xml: textbox, numberbox, spinbuttons
Categories
(Thunderbird :: General, task)
Tracking
(Not tracked)
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?
Comment 1•6 years ago
|
||
(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.
Reporter | ||
Comment 2•6 years ago
|
||
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
Assignee | ||
Comment 3•6 years ago
|
||
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.
Assignee | ||
Comment 4•6 years ago
|
||
First of three patches. Separate patches to make review easier, can combine them before landing if needed. This one removes the spinbuttons.
Assignee | ||
Comment 5•6 years ago
|
||
Second of three patches. Separate patches to make review easier, can combine them before landing if needed. This one removes the numberbox.
Assignee | ||
Comment 6•6 years ago
|
||
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.
Reporter | ||
Comment 7•6 years ago
|
||
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.
Assignee | ||
Comment 8•6 years ago
|
||
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
Reporter | ||
Comment 9•6 years ago
|
||
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)
Assignee | ||
Comment 10•6 years ago
|
||
Hmm, sorry about that. I've now merged them into one patch and rebased on the most recent commit.
Reporter | ||
Comment 11•6 years ago
|
||
Comment 12•6 years ago
|
||
(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.
Reporter | ||
Comment 13•6 years ago
|
||
Do you have any pointers?
Assignee | ||
Comment 14•6 years ago
|
||
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.
Assignee | ||
Comment 15•6 years ago
|
||
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.)
Assignee | ||
Comment 16•6 years ago
|
||
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;
Assignee | ||
Comment 17•6 years ago
|
||
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•6 years ago
|
||
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•6 years ago
|
||
Comment 20•6 years ago
|
||
Updated to tip.
Assignee | ||
Comment 21•6 years ago
|
||
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•6 years ago
|
||
(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).
Assignee | ||
Comment 23•6 years ago
|
||
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.)
Reporter | ||
Comment 24•6 years ago
|
||
Reporter | ||
Comment 25•6 years ago
|
||
(Usually when the bug has a patch -> ASSIGNED)
Reporter | ||
Updated•6 years ago
|
Comment 26•6 years ago
|
||
(In reply to Magnus Melin [:mkmelin] from comment #24)
Comment on attachment 9051535 [details] [diff] [review]
textbox-de-xbl-textbox-5.patchReview 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•6 years ago
|
||
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/87875f8dae87
[de-xbl] Remove the spinbuttons, numberbox and textbox bindings. r=mkmelin
Updated•6 years ago
|
Comment 28•6 years ago
|
||
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 :-(
Assignee | ||
Comment 29•6 years ago
|
||
Ah, duly noted. Clearly I need to get more up to speed on how to do try server runs.
Comment 30•6 years ago
|
||
We discussed that on IRC at length: https://mozilla.logbot.info/maildev/20190314#c16090000
Assignee | ||
Comment 31•6 years ago
|
||
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•6 years ago
|
||
Does this mean all the bindings (and worse, preprocessing) added in Bug 1490431 and elsewhere should be removed?
Reporter | ||
Updated•6 years ago
|
Updated•5 years ago
|
Description
•