1568504 - remove grid usage from comm/calendar/base/content/dialogs/calendar-print-dialog.xul

Status: RESOLVED FIXED

(Thunderbird :: General, task)

Comment 1

[Khushil Mistry [:khushil324]]

Attachment: Bug-1568504_remove-grid-calendar-print-dialog.patch

Comment 2

[Magnus Melin [:mkmelin]]

Comment on attachment 9080319 [details] [diff] [review]
Bug-1568504_remove-grid-calendar-print-dialog.patch

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

Please update to use grid-layout.css

Comment 3

[Khushil Mistry [:khushil324]]

Attachment: Bug-1568504_remove-grid-calendar-print-dialog-xul.patch

Comment 4

[Magnus Melin [:mkmelin]]

Comment on attachment 9083773 [details] [diff] [review]
Bug-1568504_remove-grid-calendar-print-dialog-xul.patch

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

LGTM

Comment 5

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 9083773 [details] [diff] [review]
Bug-1568504_remove-grid-calendar-print-dialog-xul.patch

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

r+ with these changes:

::: calendar/base/content/dialogs/calendar-print-dialog.xul
@@ +47,5 @@
> +          <html:div>
> +            <textbox id="title-field"
> +                     class="padded"
> +                     flex="1"
> +                     onchange="refreshHtml();"/>

If we are moving this to html, any reason not to use <html:input> here?

@@ +51,5 @@
> +                     onchange="refreshHtml();"/>
> +          </html:div>
> +          <html:div class="flex-items-center">
> +            <label control="layout-field"
> +                   value="&calendar.print.layout.label;"/>

Same here, can we use a html element instead? Same for similar elements.

@@ +54,5 @@
> +            <label control="layout-field"
> +                   value="&calendar.print.layout.label;"/>
> +          </html:div>
> +          <html:div>
> +            <menulist id="layout-field">

This one might be a bit more tricky, but I think you can use <html:select> here with the right attributes.

Comment 6

[Magnus Melin [:mkmelin]]

The textbox -> html part is being done in bug 1563000.

Comment 7

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Great, thanks!

Comment 8

[Khushil Mistry [:khushil324]]

(In reply to Philipp Kewisch [:Fallen] [:📆] from comment #5)

Same here, can we use a html element instead? Same for similar elements.

For label, we have control attribute assigned to it. HTML equivalent of the label is html:label and its control attribute equivalent is for. "for" only works on html:input and we have datepicker which is not html:input.

Comment 9

[Khushil Mistry [:khushil324]]

Attachment: Bug-1568504_remove-grid_calendar-print-dialog-xul.patch

Apply patch from bug 1563000 before testing this patch.

Comment 10

[Paul Morris [:pmorris]]

Comment on attachment 9086340 [details] [diff] [review]
Bug-1568504_remove-grid_calendar-print-dialog-xul.patch

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

The code changes look good to me.  Visually some alignment in the layout seems to be worse though.  I'll attach some screenshots.

Since Magnus and Philipp have r+'d earlier versions of this, and the point is to get rid of the XUL grids, I'm leaning toward landing this and then fixing the layout issues in a follow-up bug.  Will needinfo Philipp for confirmation of that before finalizing my review.

::: calendar/base/content/dialogs/calendar-print-dialog.js
@@ +50,5 @@
>          // Use the contractid as value
> +        let option = document.createElementNS("http://www.w3.org/1999/xhtml", "option");
> +        option.textContent = formatter.name;
> +        option.setAttribute("value", contractid);
> +        layoutList.appendChild(option);

Huh, interesting that you can append an XHTML <option> inside a XUL <menulist>.  (Things I've learned.)

Comment 11

[Khushil Mistry [:khushil324]]

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

Huh, interesting that you can append an XHTML <option> inside a XUL
<menulist>. (Things I've learned.)

No, I have changed menulist to html:select.

Comment 12

[Paul Morris [:pmorris]]

Attachment: calendar-print-dialog-without-patch-0.png

Print dialog before this patch. Note that "Print Settings" and other headings are left-aligned the same as the input labels like "Title".

Comment 13

[Paul Morris [:pmorris]]

Attachment: calendar-print-dialog-with-patch-0.png

Print dialog with this patch. Note that input labels like "Title" stick out to the left further than headings like "Print Settings". The left-alignment is off.

Philipp, should we fix this alignment in a follow-up bug or this one? Curious not just for this one but also for similar cases in the future.

Comment 14

[Paul Morris [:pmorris]]

(In reply to Khushil Mistry [:khushil324] from comment #11)

No, I have changed menulist to html:select.

Ah, of course, makes sense, thanks.

Comment 15

[Khushil Mistry [:khushil324]]

Attachment: Bug-1568504_remove-grid_calendar-print-dialog-xul.patch

I have updated the patch with solving alignment issue.
Can you check on Linux machine how it is coming up?

Comment 16

[Paul Morris [:pmorris]]

Comment on attachment 9086572 [details] [diff] [review]
Bug-1568504_remove-grid_calendar-print-dialog-xul.patch

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

Alignment looks good here on linux, thanks for fixing it.  r+

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/21657f746451
remove grid usage from calendar-print-dialog.xul. r=pmorris

Comment 18

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Sorry I didn't respond here, I think my input isn't required any more? Feel free to ping me if you need anything.