Closed Bug 1590996 Opened 5 months ago Closed 5 months ago

remove remaining use of <xul:spring> from Thunderbird code (<spring> removed in bug 1590897)

Categories

(Thunderbird :: General, task)

task
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 72.0

People

(Reporter: mkmelin, Assigned: mkmelin)

References

Details

Attachments

(1 file, 1 obsolete file)

Bug 1590897 is getting rid of <xul:spring>.

Attached patch bug1590897_rm_spring.patch (obsolete) — Splinter Review

Bug 1590897 is now on inbound.

Attachment #9103875 - Flags: review?(khushil324)
Comment on attachment 9103875 [details] [diff] [review]
bug1590897_rm_spring.patch

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

::: mail/components/addrbook/content/abContactsPanel.xul
@@ -156,5 @@
>  
>      <separator class="thin"/>
>  
> -    <hbox>
> -      <spring flex="1"/>

If you want to reduce the scope of the change, I _think_ you can use either:
- `spacer` instead of `spring`
-  <hbox pack="center"> on the parent

(I'd recommend the second solution over the first one if it works)

There seems to be another usage in editor/ui/dialogs/content/EdTableProps.xul

Thanks Tim. pack="center" on the hbox does work too.
I'm not too worried about the scope (which is still very small). Less work in future conversions would be my main objective.

It seems pack="center" is relatively little used in mozilla-central (57 hits only). With all this in mind, which approach would you recommend?

Flags: needinfo?(ntim.bugs)

Oh, and re editor/, Thunderbird is not using that anymore. We copied the things we actually use and need into mail/. I guess the springs went away in de-grid of the moved file.

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

Thanks Tim. pack="center" on the hbox does work too.
I'm not too worried about the scope (which is still very small). Less work in future conversions would be my main objective.

It seems pack="center" is relatively little used in mozilla-central (57 hits only). With all this in mind, which approach would you recommend?

You could set -moz-box-pack: center as an inline style or inside a stylesheet.

Flags: needinfo?(ntim.bugs)

That's the same as setting pack="center". But it's still an internal css property that we'd have to migrate away from at some point.
The strength of the current patch is that e.g. moving from hbox to html:div would work the same also over time.

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

That's the same as setting pack="center". But it's still an internal css property that we'd have to migrate away from at some point.
The strength of the current patch is that e.g. moving from hbox to html:div would work the same also over time.

Up to you, really :) To me, it seems a bit overkill to use CSS grid here, CSS flex would work here, but XUL flex would make more sense given the children are XUL boxes as well. It's more likely that we emulate XUL flex to be CSS flex or mass-migrate it, so 1 usage won't really make a difference.

Comment on attachment 9103875 [details] [diff] [review]
bug1590897_rm_spring.patch

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

Looks good to me. r=khushil.
Attachment #9103875 - Flags: review?(khushil324) → review+

Maybe we'll still go with just pack="center" then.

Attachment #9103875 - Attachment is obsolete: true
Attachment #9103916 - Flags: review?(khushil324)
Comment on attachment 9103916 [details] [diff] [review]
bug1590897_rm_spring_alt2.patch

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

Looks good to me. r=khushil
Comment on attachment 9103916 [details] [diff] [review]
bug1590897_rm_spring_alt2.patch

You forgot to set the r+
Attachment #9103916 - Flags: review?(khushil324) → review+

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/00dd91bba444
remove remaining use of <xul:spring> from Thunderbird code (<spring> removed in bug 1590897). r=khushil

Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 72.0
You need to log in before you can comment on or make changes to this bug.