1527003 - Remove the XUL grid in emailWizard.xul

Status: RESOLVED FIXED

(Thunderbird :: General, task)

Description

[Richard Marti (:Paenglab)]

In emailWizard.xul we have XUL grids that can be removed.

Comment 1

[Richard Marti (:Paenglab)]

Attachment: 1527003-no-grid-emailWizard.patch

I have already a patch that removes the grids except https://searchfox.org/comm-central/source/mail/components/accountcreation/content/emailWizard.xul#270-278.

Ben, how should I exchange the <grid> so it still works best for you to fill it with your extension? Do you want a <html:table> or something like:

<hbox>
<vbox id="result_addon_install_column_icon" pack="start" align="center" />
<vbox id="result_addon_install_column_link" pack="start" align="center" />
<vbox id="result_addon_install_column_button" pack="start" align="center" />
</hbox>

Or something different? As a reference I added my WIP patch which needs bug 1524457 applied first.

Comment 2

[Ben Bucksch (:BenB)]

Hey Richard,

1. Is there a particular reason why this <grid> needs to be replaced? Or is this just a general attempt to remove XUL? Because this dialog heavily depends on XUL, including specific XUL widgets and also how precisely the events of these widgets work. Transforming this to HTML is going to be a major piece of work.

2. If you're removing <grid>, could you please replace it with CSS display: grid and not an HTML <table>?

how should I exchange the <grid> so it still works best for you to fill it with your extension?

The code to fill this is in emailWizard.js displayConfigResult(). Search for "result_addon_install_rows".
Essentially, the XUL contains the <grid> with the columns, and the mentioned function fills in the rows.

The data is based on a JSON file we get from the server. The code is written to handle several addons, and an description with arbitrary length, as a link. The description line-wraps. This flexibility should be retained in any rewrite.

Comment 3

[Richard Marti (:Paenglab)]

(In reply to Ben Bucksch (:BenB) from comment #2)

Hey Richard,

1. Is there a particular reason why this <grid> needs to be replaced? Or is this just a general attempt to remove XUL? Because this dialog heavily depends on XUL, including specific XUL widgets and also how precisely the events of these widgets work. Transforming this to HTML is going to be a major piece of work.

Ask in bug 1520625 why they want to remove it.

2. If you're removing <grid>, could you please replace it with CSS display: grid and not an HTML <table>?

This is also asked in bug 1520625 comment 1 but no answer and they implemented it different. Maybe the CSS display: grid doesn't work well with XUL?

how should I exchange the <grid> so it still works best for you to fill it with your extension?

The code to fill this is in emailWizard.js displayConfigResult(). Search for "result_addon_install_rows".
Essentially, the XUL contains the <grid> with the columns, and the mentioned function fills in the rows.

I think, then the HTML<table> would fit better.

The data is based on a JSON file we get from the server. The code is written to handle several addons, and an description with arbitrary length, as a link. The description line-wraps. This flexibility should be retained in any rewrite.

I have no exchange account to test, and my JS knowledge is limited. When I wrote my solution, could you test and improve it?

Comment 4

[Ben Bucksch (:BenB)]

bug 1520625

Thanks, that was the part I didn't know.

They implemented it different.

Maybe they had good reasons, maybe not. Maybe the reasons apply only to them. It makes sense to me to try CSS grid first.
If it doesn't work, then we can still fall back to HTML tables.

Note that HTML and XUL don't work terribly well together, esp. when it comes to line wrap, which we do need here.

I have no exchange account to test

See bug 1500105 comment 7.

When I wrote my solution, could you test and improve it?

At the moment, I'm completely swamped with work due to the Owl release. I'm already at 180% work load.

Comment 5

[Richard Marti (:Paenglab)]

Attachment: 1527003-no-grid-emailWizard.patch

I converted the grids except the result_addon_install grid which is for OWL only. Until the grids are removed, I think we can leave it and Ben, or someone else that knows the extension, can change it when it's really needed.

Comment 6

[Magnus Melin [:mkmelin]]

Comment on attachment 9053124 [details] [diff] [review]
1527003-no-grid-emailWizard.patch

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

There's a misalignment (centered?) for the manual config collumns of SMTP and Username

::: mail/components/accountcreation/content/emailWizard.xul
@@ +128,5 @@
>  
>    <spacer id="fullwidth"/>
>  
>    <vbox id="mastervbox" class="mastervbox" flex="1">
> +    <html:table id="initialSettings">

Since this first section is not table data, I'm not sure we want a table for it. That's quite discouraged.

Is it possible to set a width for the label and just use
<html:label>: <textbox> <description......>

Of course, there's all kind of ugly here, so, it's hard to say if it's worth pursuing in this patch...

::: mail/themes/linux/mail/accountCreation.css
@@ +158,5 @@
>  .technical_details[expanded] {
>    background-image: url("chrome://messenger/skin/section_expanded.png");
>  }
>  
> +.alignEnd {

I think it would be prefereable not to use camelCasing for class names. e.g. align-end would be nicer

::: mail/themes/osx/mail/accountCreation.css
@@ +151,1 @@
>  }

move these (and some more of the new rule) into the shared accountCreation.css?

Comment 7

[Richard Marti (:Paenglab)]

Attachment: 1527003-no-grid-emailWizard.patch v3

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

Comment on attachment 9053124 [details] [diff] [review]
1527003-no-grid-emailWizard.patch

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

There's a misalignment (centered?) for the manual config collumns of SMTP
and Username

On Windows it looked good because the IMAP/POP menulist isn't so wide like under Linux. Reverted to left align.

::: mail/components/accountcreation/content/emailWizard.xul
@@ +128,5 @@

<spacer id="fullwidth"/>

<vbox id="mastervbox" class="mastervbox" flex="1">

• <html:table id="initialSettings">

Since this first section is not table data, I'm not sure we want a table for
it. That's quite discouraged.

Is it possible to set a width for the label and just use
<html:label>: <textbox> <description......>

Of course, there's all kind of ugly here, so, it's hard to say if it's worth
pursuing in this patch...

Now without table. As long as the labels in the first column aren't wider than 9em, it works.

::: mail/themes/linux/mail/accountCreation.css
@@ +158,5 @@

.technical_details[expanded] {
background-image: url("chrome://messenger/skin/section_expanded.png");
}

+.alignEnd {

I think it would be prefereable not to use camelCasing for class names. e.g.
align-end would be nicer

changed.

::: mail/themes/osx/mail/accountCreation.css
@@ +151,1 @@

}

move these (and some more of the new rule) into the shared
accountCreation.css?

Done. When this lands as it is now, we can close bug 1503719.

Comment 8

[Magnus Melin [:mkmelin]]

Comment on attachment 9053146 [details] [diff] [review]
1527003-no-grid-emailWizard.patch v3

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

Looks good, thx! r=mkmelin

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/e8c17fa950fa
Remove grid usage from emailWizard.xul. r=mkmelin