1569562 - remove grid usage from comm/calendar/lightning/content/lightning-item-iframe.xul

Status: RESOLVED FIXED

(Calendar :: Dialogs, task)

Comment 1

[Khushil Mistry [:khushil324]]

Attachment: Bug-1569562_remove-grid-lightning-item-iframe.patch

Comment 2

[Magnus Melin [:mkmelin]]

Comment on attachment 9081214 [details] [diff] [review]
Bug-1569562_remove-grid-lightning-item-iframe.patch

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

This data is not semantically tabular - it's just about presentation, so you shouldn't use table for this.

It also looks pretty bad, and not at all like before: spacings are all off, widths of textboxes, and the size of the whole event dialog are wrong. Will attach a screenshot

::: calendar/lightning/content/lightning-item-iframe.xul
@@ +134,5 @@
>      <!-- notificationbox will be added here lazily. -->
>    </hbox>
>  
> +  <html:table id="event-grid"
> +              style="padding-top:8px; padding-bottom:10px; padding-inline-start:8px; padding-inline-end:10px;">

can we move this and any added inline styling into a css file

@@ +213,2 @@
>  
> +    <separator class="groove" id="event-grid-basic-separator"/>

move id first while you're touching this

Comment 3

[Magnus Melin [:mkmelin]]

Attachment: eventdialog.png (showing problems)

Comment 4

[Khushil Mistry [:khushil324]]

Attachment: Bug-1569562_remove-grid_lightning-item-iframe-xul.patch

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=a6e0f806fea08af1f1d12e9d15be778a29cca9ff

Comment 5

[Paul Morris [:pmorris]]

Comment on attachment 9088701 [details] [diff] [review]
Bug-1569562_remove-grid_lightning-item-iframe-xul.patch

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

Code changes look good.  One small question there.  There's a problem with the relative sizing of the columns of the table.  The column on the left expands as the window grows horizontally, but the column on the right should expand instead.  This is really noticeable with the "event in a tab" preference turned on.  I'll upload some screenshots.

::: calendar/lightning/content/lightning-item-iframe.xul
@@ +188,5 @@
> +                         onpopuphiding="return categoryPopupHiding(event);">
> +                <textbox id="item-categories-textbox"
> +                         placeholder="&event.categories.textbox.label;"
> +                         onblur="this.parentNode.removeAttribute(&quot;ignorekeys&quot;);"
> +                         onfocus="this.parentNode.setAttribute(&quot;ignorekeys&quot;, &quot;true&quot;);"

Can we use single quotes (') instead of &quot; here?  If that works, that would be more readable.

Comment 6

[Paul Morris [:pmorris]]

Attachment: calendar-event-dialog-tb60-before.png

Previous status quo on TB 60.

Comment 7

[Paul Morris [:pmorris]]

Attachment: calendar-event-dialog-with-patch-after.png

After applying patch, column sizing behavior isn't the same.

Comment 8

[Khushil Mistry [:khushil324]]

Attachment: Bug-1569562_remove-grid_lightning-item-iframe-xul.patch

Comment 9

[Paul Morris [:pmorris]]

Comment on attachment 9088774 [details] [diff] [review]
Bug-1569562_remove-grid_lightning-item-iframe-xul.patch

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

The column sizing looks good now, and using single quotes instead of " works fine.  I see a couple other spacing issues.  

1. The 'title' and 'location' text boxes extend too far to the right, out into the right margin of the dialog.  They should be aligned with the right edge of the 'calendar' drop down menu below them.  

2. There's now more space between the horizontal separator lines and the UI elements that appear above them, so things look a little less compact and balanced.

Comment 10

[Magnus Melin [:mkmelin]]

Khushil and I discussed the use of <table> here yesterday. I think we shouldn't use that unless it's semantically tabular data.
Looking more closely, it's fairly close, but the "row" with category and calendar in one mess it up... We might want to put "calendar" above the other fields. In a way, having it next to category is conceptually wrong too - I guess it's there to save space, but we do have plenty.

Comment 11

[Khushil Mistry [:khushil324]]

For "Calendar", do we want menulist to cover 100% width of the dialog or the default width?

Comment 12

[Magnus Melin [:mkmelin]]

I'd think just the default width. Would have to see it in action.

Comment 13

[Khushil Mistry [:khushil324]]

Attachment: Bug-1569562_remove-grid_lightning-item-iframe-xul.patch

Comment 14

[Khushil Mistry [:khushil324]]

Attachment: Bug-1569562_remove-grid_lightning-item-iframe-xul.patch

Comment 15

[Magnus Melin [:mkmelin]]

Comment on attachment 9088999 [details] [diff] [review]
Bug-1569562_remove-grid_lightning-item-iframe-xul.patch

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

There are a few pixels of misalignment with title and category/calendar. Looking at it, maybe the calendar dropdown would look better with full width after all.

::: calendar/lightning/content/lightning-item-iframe.js
@@ +921,5 @@
> +    if (maxCount === 0) {
> +        document.getElementById("event-grid-category-color-row").setAttribute("hidden", "hidden");
> +    } else {
> +        document.getElementById("event-grid-category-color-row").removeAttribute("hidden");
> +    }

document.getElementById("event-grid-category-color-row").toggleAttribute("hidden", maxCount === 0);

@@ +3695,5 @@
> +    let eventGridLinkRow = document.getElementById("event-grid-link-row");
> +    if (aShow) {
> +        eventGridLinkRow.removeAttribute("hidden");
> +    } else {
> +        eventGridLinkRow.setAttribute("hidden", "hidden");

use toggleattribute

::: calendar/lightning/content/lightning-item-iframe.xul
@@ +475,5 @@
> +      <html:tr>
> +        <html:td colspan="2">
> +          <separator id="event-grid-alarm-separator" class="groove full-width"/>
> +        </html:td>
> +      </html:tr>

This is just a visual separator. Instead of a row with a separator, just add a suitable css class to style the tr border.

@@ +594,5 @@
> +      <html:tr>
> +        <html:td colspan="2">
> +          <separator id="event-grid-tabbox-separator" class="groove full-width"/>
> +        </html:td>
> +      </html:tr>

same with this (and any other of these)

@@ +599,3 @@
>  
>        <!-- Multi purpose tab box -->
> +      <html:tr id="event-grid-tab-box-row">

This is where the table should end.

::: calendar/providers/gdata/content/gdata-lightning-item-iframe.js
@@ +58,5 @@
> +            if (node) {
> +                if (isGoogleTask) {
> +                    node.setAttribute("hidden", "hidden");
> +                } else {
> +                    node.removeAttribute("hidden");

toggleAttribute

@@ +117,1 @@
>          }

toggleattribute

Comment 16

[Khushil Mistry [:khushil324]]

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

This is where the table should end.

There is a event-grid-link-row below this tab box, I guess we should shift it above the tab box and put tabbox outside the table. What do you say?

Comment 17

[Magnus Melin [:mkmelin]]

Yes the tabbox should move out of the table.

Comment 18

[Khushil Mistry [:khushil324]]

Attachment: Bug-1569562_remove-grid_lightning-item-iframe-xul.patch

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=e89e0dce2baa1ec0c525429c343921ff24b4f92e

Comment 19

[Magnus Melin [:mkmelin]]

Comment on attachment 9089794 [details] [diff] [review]
Bug-1569562_remove-grid_lightning-item-iframe-xul.patch

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

The dropdowns for calendar and category are not as high as the other fields. 
The title and location fields still have slightly wrong margin/padding, so the are a bit further left than the other controls.

::: calendar/base/themes/common/dialogs/calendar-event-dialog.css
@@ +159,5 @@
> +    padding-top: 8px;
> +    padding-inline-start: 8px;
> +    padding-inline-end: 10px;
> +    border-spacing: 0;
> +    height: 100%;

we really should fix indention of css too to be 2 space...

::: calendar/lightning/content/lightning-item-iframe.xul
@@ +211,3 @@
>  
> +    <html:tr>
> +      <html:td id="event-grid-basic-separator" colspan="2" class="separator-line">

Why not just add a suitable top border for event-grid-allday-row (using a class of course)

@@ +397,1 @@
>  

same.  though I'm not convinced there should be a separator here

@@ +471,5 @@
>  
> +    <html:tr>
> +      <html:td id="event-grid-alarm-separator" colspan="2" class="separator-line">
> +      </html:td>
> +    </html:tr>

and here

::: calendar/providers/gdata/content/gdata-lightning-item-iframe.js
@@ +53,5 @@
> +    ];
> +
> +    for (let id of hideRowForTaskIds) {
> +      let node = document.getElementById(id);
> +      if (node) {

these are all "known to exist" so you can just do 

 document.getElementById(id).toggleAttribute("hidden", isGoogleTask);

Comment 20

[Magnus Melin [:mkmelin]]

Comment on attachment 9089794 [details] [diff] [review]
Bug-1569562_remove-grid_lightning-item-iframe-xul.patch

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

The dropdowns for calendar and category are not as high as the other fields. 
The title and location fields still have slightly wrong margin/padding, so the are a bit further left than the other controls.

::: calendar/base/themes/common/dialogs/calendar-event-dialog.css
@@ +159,5 @@
> +    padding-top: 8px;
> +    padding-inline-start: 8px;
> +    padding-inline-end: 10px;
> +    border-spacing: 0;
> +    height: 100%;

we really should fix indention of css too to be 2 space...

::: calendar/lightning/content/lightning-item-iframe.xul
@@ +211,3 @@
>  
> +    <html:tr>
> +      <html:td id="event-grid-basic-separator" colspan="2" class="separator-line">

Why not just add a suitable top border for event-grid-allday-row (using a class of course)

@@ +397,1 @@
>  

same.  though I'm not convinced there should be a separator here

@@ +471,5 @@
>  
> +    <html:tr>
> +      <html:td id="event-grid-alarm-separator" colspan="2" class="separator-line">
> +      </html:td>
> +    </html:tr>

and here

::: calendar/providers/gdata/content/gdata-lightning-item-iframe.js
@@ +53,5 @@
> +    ];
> +
> +    for (let id of hideRowForTaskIds) {
> +      let node = document.getElementById(id);
> +      if (node) {

these are all "known to exist" so you can just do 

 document.getElementById(id).toggleAttribute("hidden", isGoogleTask);

Comment 21

[Khushil Mistry [:khushil324]]

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

The dropdowns for calendar and category are not as high as the other fields.

I am not getting this. Can you share some screenshot?

Comment 22

[Magnus Melin [:mkmelin]]

Attachment: newevent.png

Comment 23

[Khushil Mistry [:khushil324]]

Are you talking about margin/padding bottom is not enough for these two dropdowns?

Comment 24

[Magnus Melin [:mkmelin]]

I didn't check what exactly is wrong, but they are "thinner" than e.g the reminder dropdown.
Also, you can see Title and Location are not completely aligned with the other widgets.

Comment 25

[Khushil Mistry [:khushil324]]

Attachment: Bug-1569562_remove-grid_lightning-item-iframe-xul.patch

Comment 26

[Khushil Mistry [:khushil324]]

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

The dropdowns for calendar and category are not as high as the other fields.

I checked it but layout shows same height for all the dropdown items.

Why not just add a suitable top border for event-grid-allday-row (using a
class of course)

We need certain padding/margin but we can not apply that to table row so we need to use table data.

Comment 27

[Magnus Melin [:mkmelin]]

Comment on attachment 9090660 [details] [diff] [review]
Bug-1569562_remove-grid_lightning-item-iframe-xul.patch

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

Seems to look ok now

::: calendar/lightning/content/lightning-item-iframe.xul
@@ +211,4 @@
>  
> +    <html:tr>
> +      <html:td id="event-grid-basic-separator" colspan="2" class="separator-line">
> +      </html:td>

my earlier comments about this still not addressed. Why not just style the top border of the next tr?

Comment 28

[Khushil Mistry [:khushil324]]

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

my earlier comments about this still not addressed. Why not just style the
top border of the next tr?

I am using html:td here because this reason: We need certain padding/margin but we can not apply that to table row so we need to use table data.
And as I am using html:td for giving certain padding, I thought we should set the border to html:td itself only.

Comment 29

[Paul Morris [:pmorris]]

Comment on attachment 9090660 [details] [diff] [review]
Bug-1569562_remove-grid_lightning-item-iframe-xul.patch

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

One substantive comment on the code changes.  And a few visual layout things:

- If the "calendar" drop-down is going to move to its own line I think it should not be the first one.  I'd put it before the "category" line.  But let's bring in aleca on this UI question.

- The widths of "calendar" and "category" drop-downs should be the same.  I think making them full width is better.  Another question for aleca.

- I'm not sure about putting all the "start", "end", "repeat", and "reminder" rows together, removing the dividing lines between them.  I could see having "start", "end", and "repeat" together since they all involve timing.  And then a line above and below "reminder" to put it in its own section.  Another question for aleca.

- The dividing line below "category" is thinner than the one after "reminder".  They both should be like the one under "reminder".

- The right margin is different for the description text area than for the title, location, etc. items at the top. These should line up.

I'll attach a screenshot and needinfo aleca.

::: calendar/base/themes/common/dialogs/calendar-event-dialog.css
@@ +101,4 @@
>  }
>  
>  #declineButton {
> +  list-style-image: url(chrome://calendar-common/skin/icons/decline.svg);

Just for future reference, it's best to put formatting-only (whitespace) changes into a separate patch file.  That makes it easier to review and makes the history more meaningful and easier to understand.  

(Ideally bugzilla would do a better job of pointing out which characters actually changed, which would help with this, but alas.)

::: calendar/providers/gdata/content/gdata-lightning-item-iframe.js
@@ +52,5 @@
> +
> +    for (let id of hideRowForTaskIds) {
> +      document.getElementById(id).toggleAttribute("hidden", isGoogleTask);
> +    }
> +

Looks like these could just stay in |hideForTaskIds|, right?  I don't see a need for a separate list.  You could use |node.toggleAttribute("hidden", isGoogleTask)| for the whole list.

Comment 30

[Paul Morris [:pmorris]]

Attachment: calendar-event-dialog-with-patch-after-1.png

Here's a screenshot. aleca, there are some UI questions for you in my review comment above, thanks.

Comment 31

[Paul Morris [:pmorris]]

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

Looks like these could just stay in |hideForTaskIds|, right? I don't see a
need for a separate list. You could use |node.toggleAttribute("hidden",
isGoogleTask)| for the whole list.

Oh, wait, maybe this is a "combining XUL and HTML" thing. In that case lets name the new array "htmlHideForTaskIds" or similar to make it clearer what's going on.

Comment 32

[Magnus Melin [:mkmelin]]

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

• If the "calendar" drop-down is going to move to its own line I think it
  should not be the first one. I'd put it before the "category" line. But
  let's bring in aleca on this UI question.

The reason I suggested to move it there was because if you look at this dialog from a data pov, calendar is obviously the most important thing. Where it was before was semantically wrong: it had nothing to do with category.

Comment 33

[Alessandro Castellani [:aleca]]

Hey, thanks for the needinfo on this.

I agree that the Calendar and Category dropdowns should be both full-width.
Having the Calendar as first option definitely makes sense from a data POV, like Magnus pointed out.
It might feel weird, but it's definitely the most important attribute the user should select before creating the event.

Regarding the timing section, I agree that we should keep a spacer line above the "Reminder" section.
Also, I'd like to suggest moving the "All day Event" checkbox beside the "Start:" row, to the right of the time dropdown, as it makes more sense semantically to select/confirm the day of the event and then specify if it goes one all day or it needs and end time.

Comment 34

[Khushil Mistry [:khushil324]]

Attachment: Bug-1569562_remove-grid_lightning-item-iframe-xul-1.patch

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=a258ab628a6f3612b9cd9102897a37fd5772ccd1

Comment 35

[Paul Morris [:pmorris]]

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

The reason I suggested to move it there was because if you look at this dialog from a data pov, calendar is obviously the most important thing. Where it was before was semantically wrong: it had nothing to do with category.

But is a data pov the right one to take here? I disagree with putting the calendar first for broader UX reasons. I don't think the calendar I'm creating an event in is the most important or first thing in my mind as a user. The first thing on my mind when creating an event is the event, its title, location, time, etc... Basically, I want to first say what the event is, and then say what calendar it should be added to (if that even needs to change).

Also, the calendar is already selected, based on the currently active calendar. In many cases, I won't need to change it because it will already be correct. If I'm a user with only one calendar I'll never have to change it.

Also, when opening an existing event to view details about it, I care about the details specific to that event, what is it, where is it, when is it, etc. which calendar it happens to be in is of secondary importance.

I know aleca has already commented, but I wanted to make the case for the current arrangement of having title, location, time, etc. at the top before calendar. FWIW, other calendar applications (google, apple, etc.) put the title first, then location, etc. things that are specific to the event.

Comment 36

[Paul Morris [:pmorris]]

Comment on attachment 9091433 [details] [diff] [review]
Bug-1569562_remove-grid_lightning-item-iframe-xul-1.patch

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

Code changes look good, and I like the new location of the "all day" checkbox, the placement of the divider lines, and the matching width of the drop downs for calendar and category.  There a few small issues remaining.  I'll attach a screenshot.

1. The spacing between a divider line and the next input element looks too close to me.  The input elements should appear centered vertically between the divider lines, so we need a little more space between input elements and the divider line above them.  

2. The divider lines don't all have the same thickness.  Namely, the last one looks thicker than the others.  I like the thinner ones, but either is fine just as long as they are all the same.

::: calendar/providers/gdata/content/gdata-lightning-item-iframe.js
@@ +43,4 @@
>        }
>      }
>  
> +    let hideRowForTaskIds = [

It would be good to make it clearer why there are two lists of ids here, one for XUL elements and one for HTML elements. 

One option would be to rename "hideForTaskIds" -> "xulHideForTaskIds" and then rename "hideRowForTaskIds" -> "hideForTaskIds".  Then as we move away from XUL the XUL one will eventually go away.

Another option would be to just add a comment somewhere explaining why there are two arrays.

Comment 37

[Paul Morris [:pmorris]]

Attachment: calendar-event-dialog-with-patch-after-2.png

Comment 38

[Magnus Melin [:mkmelin]]

I do find that looking at the data first often you get the right UX too, because usually well designed data is logical, and many times logical UI makes good UX.

I don't think you should move the "All day Event". That's wrong, as it's not only about the start day, it's also about the end day. See, from a data pov, it is wrong: the row is "Start". The section is When (but we don't say that anywhere explicitly). So it makes sense to have All day Event on it's own row like it was.

Comment 39

[Alessandro Castellani [:aleca]]

Sounds good, put back the "All day Event" where it was.

Comment 40

[Khushil Mistry [:khushil324]]

Attachment: Bug-1569562_remove-grid_lightning-item-iframe-xul-2.patch

Comment 41

[Paul Morris [:pmorris]]

Comment on attachment 9091787 [details] [diff] [review]
Bug-1569562_remove-grid_lightning-item-iframe-xul-2.patch

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

Getting close, but not quite there yet.

Now that calendar is the first item it should be focused on opening the dialog, rather than the title field.

The last separator line (the one that is now outside the table and uses <separator>) still looks thicker here (see screenshots).  To avoid this kind of problem, we need to use the same implementation for all the separators.  Also, as Magnus says we shouldn't need additional table rows for the separator lines.  It's tricky because margin won't work on table cells to add spacing on the other side of the line, so all we have to work with is padding.  So we need to adjust the padding both above and below separator lines.  And luckily we can just add a separator line below the last row of the table to get rid of the <separator>.  And then adjust the padding of the thing that comes after the table, as needed.  Below is the basic idea (also https://jsfiddle.net/1ghsbov6/).  

<table>
      <tr>
        <th>header1</th>
        <td>data1</td>
      </tr>
      <tr>
        <th class="above-separator">header2</th>
        <td class="above-separator">data2</td>
      </tr>
      <tr>
        <th class="below-separator above-separator">header3</th>
        <td class="below-separator above-separator">data3</td>
      </tr>
      <tr>
        <th class="below-separator">header4</th>
        <td class="below-separator">data4</td>
      </tr>
      <tr>
        <th>header4</th>
        <td>data4</td>
      </tr>
      <tr>
        <th>header5</th>
        <td>data5</td>
      </tr>
      <tr>
        <th class="above-separator">header6</th>
        <td class="above-separator">data6</td>
      </tr>
    </table>

/* CSS */

table {
  border-collapse: collapse;
}

td,
th {
  padding: 5px;
}

.above-separator {
  /* twice the usual cell padding */
  padding-bottom: 10px;
  border-bottom: 1px solid black;

  /* margin-bottom: 100px; has no effect */
}

.below-separator {
  /* twice the usual cell padding */
  padding-top: 10px;
}

Since the padding values should stay in sync this is a good case for using CSS var and calc:
https://developer.mozilla.org/en-US/docs/Web/CSS/var
https://developer.mozilla.org/en-US/docs/Web/CSS/calc

Comment 42

[Khushil Mistry [:khushil324]]

Attachment: Bug-1569562_remove-grid_lightning-item-iframe-xul-3.patch

Menulist, Checkbox and Tabbox contain certain margin and there is a difference of 4px between top margin and bottom margin. So I have used calc and var in CSS to maintain the gap of 4px.

Comment 43

[Paul Morris [:pmorris]]

Comment on attachment 9092288 [details] [diff] [review]
Bug-1569562_remove-grid_lightning-item-iframe-xul-3.patch

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

Hm, tricky with those elements having margins.   I think the spacing is not quite right yet.  I'll attach a screenshot.  The main thing I notice is the space above and below a separator line should be the same size, but it's not, like with the line between "repeat" and "reminder".  And that size should be roughly the same as the size of the space between items that don't have a separator between them, like the status and repeat drop-down menus.

There are various ways you might get there.  My first thought would be to set the top and bottom margins on those elements to 0 and then you can just use the table cell padding to provide uniform spacing.  Then if you set the default cell padding to N (using CSS var), then you can set the above and below spacer padding to N*2 (using CSS calc).

A new issue I noticed: in a task dialog there's no "all day" checkbox, so a different row should have the "below-separator" class.  I'm not sure the best way to fix that...

A side note (not for this bug): in task dialog the percent complete input box is too tall.  (I remember this coming up somewhere else not that long ago...)

::: calendar/lightning/content/lightning-item-iframe.js
@@ +448,1 @@
>      document.getElementById("item-title").select();

It looks like the .select() undoes the .focus().  The title text is selected when you open a new dialog.  So likely both of these need to be changed to fix that.

Comment 44

[Paul Morris [:pmorris]]

Attachment: new-task.png

Forgot to say that it's much better with the spacers all the same and not having to add additional table rows.

Comment 45

[Khushil Mistry [:khushil324]]

Attachment: Bug-1569562_remove-grid_lightning-item-iframe-xul-4.patch

Comment 46

[Paul Morris [:pmorris]]

Comment on attachment 9093140 [details] [diff] [review]
Bug-1569562_remove-grid_lightning-item-iframe-xul-4.patch

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

Looks good!  The spacing with the spacers is just right, focus is on the calendar element, and the special handling for the task dialog works well.

::: calendar/base/themes/common/dialogs/calendar-event-dialog.css
@@ +230,5 @@
> +}
> +
> +#percent-complete-textbox {
> +  border: 1px solid rgba(0, 0, 0, 0.3);
> +  background-color: -moz-field;

This fixes the height for the percent complete input box, but here on Linux it leads to it having square corners instead of rounded ones.  I'd say let's leave this out for now and address it in a follow-up bug.

Comment 47

[Paul Morris [:pmorris]]

Attachment: new-task-dialog-percent-complete-box.png

Here's what I'm seeing on the percent complete box. It's not too bad, but I'd say let's leave that part out and handle it in a follow-up bug. (I'm remembering that getting this to look right is trickier than it looks.)

Comment 48

[Khushil Mistry [:khushil324]]

Attachment: Bug-1569562_remove-grid_lightning-item-iframe-xul-5.patch [landed]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=2de5f5a8a28be7f0903c87978e53a6d7e201fc4f

Comment 49

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/1dea742ee4cc
remove grid usage from lightning-item-iframe.xul. r=pmorris

Comment 50

[Alessandro Castellani [:aleca]]

Attachment: Screenshot from 2019-09-19 11-29-05.png

I'm sorry to bring back this bug again, but I'm seeing some visual inconsistencies on the Calendar Even Dialog.
The Title and Location fields are shorter, not as tall as they should be, and are missing the initial inline padding.

I'd recommend taking a look at the input-fields.css file I implemented to keep all the input fields consistent and overcome the necessity of forcing the width: 100% style on them.

Cheers

Comment 51

[Khushil Mistry [:khushil324]]

Is this on the windows machine?

Comment 52

[Khushil Mistry [:khushil324]]

I am on Mac. Yes, there is a problem with inline padding. But I am not seeing a problem with the width of the html:input so just want to confirm.

Comment 53

[Alessandro Castellani [:aleca]]

Is Linux, elementary OS, based on Ubuntu 18.04

Comment 54

[Khushil Mistry [:khushil324]]

Thanks, I am looking into it.

Comment 55

[Khushil Mistry [:khushil324]]

Attachment: Bug-1569562_follow-up_input-width-1.patch

Comment 56

[Paul Morris [:pmorris]]

Comment on attachment 9094149 [details] [diff] [review]
Bug-1569562_follow-up_input-width-1.patch

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

The css changes look good to me.  And the inline padding and width of the inputs look good here with this patch.  

But I defer to Alex, since he knows the css better, and since the input widths were not off on my ubuntu before this follow-up.

Comment 57

[Alessandro Castellani [:aleca]]

Attachment: calendar-dialog.png

Comment 58

[Alessandro Castellani [:aleca]]

Comment on attachment 9094149 [details] [diff] [review]
Bug-1569562_follow-up_input-width-1.patch

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

A bit better, but not complete.

The input fields are not as tall as the others.
Looking at the default textbox, we always have a padding top of 2px and a padding bottom of 3px.
As you can see the title and location fields are smaller in height than the other fields in the dialog.

Also, the spacing between the rows is inconsistent and the fields grow past the width of other elements (see screenshot).

Comment 59

[Stefan Sitter [:ssitter]]

Could this change be the cause for the regression reported in Bug 1582929?

Comment 60

[Khushil Mistry [:khushil324]]

Attachment: Bug-1569562_follow-up_input-width-2.patch

Comment 61

[Alessandro Castellani [:aleca]]

Comment on attachment 9094524 [details] [diff] [review]
Bug-1569562_follow-up_input-width-2.patch

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

Looks better, thanks.
Add the changes I suggested and it should be good to land.
Do you have the chance to testing it on Windows or macOS?

::: calendar/base/themes/common/dialogs/calendar-event-dialog.css
@@ +185,5 @@
>  
>  .event-input-td > input {
> +  flex: 1;
> +  margin-inline-end: 3px;
> +  padding-inline-start: 4px;

Add padding top and bottom of 3px, no need to add the !important attribute.

@@ +199,2 @@
>    padding-top: 3px !important;
>    padding-bottom: 3px !important;

Remove padding top and bottom from the `td`, these should be on the input.

@@ +207,3 @@
>  #event-grid > tr > th,
>  #event-grid > tr > td {
>    padding: 0;

Add margin: 2px 0; so we can handle the spacing of the various rows with a single consistent attribute.

Comment 62

[Jorg K (CEST = GMT+2)]

Do you have the chance to testing it on Windows or macOS?

Khushil is on Mac, Alex, you're on Linux (and Mac? (once repaired?)) and we can get Richard (or myself) to check it on Windows.

Comment 63

[Khushil Mistry [:khushil324]]

Attachment: Bug-1569562_follow-up_input-width-3.patch

Jorg and Richard, can one of you check for the windows machine?

Comment 64

[Paul Morris [:pmorris]]

Comment on attachment 9094888 [details] [diff] [review]
Bug-1569562_follow-up_input-width-3.patch

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

One question about one change.  The other changes looks okay to me.  (I haven't applied the patch to test it yet.)

::: calendar/base/themes/common/dialogs/calendar-event-dialog.css
@@ +187,5 @@
> +  flex: 1;
> +  margin-inline-end: 3px;
> +  padding-inline-start: 4px;
> +  margin-top: 3px;
> +  margin-bottom: 3px;

Hm, aleca requested adding 'padding-' top and bottom of 3px here.  But I see you've added 'margin-' top and bottom instead.  What's the reason for the difference?

Comment 65

[Khushil Mistry [:khushil324]]

Padding will increase the inner size of the input. If we are removing padding of the td, it will be margin for the input. I guess, Aleca just wants styling related to margin/padding on the element itself, not on the td or th. Am I missing here something?

Comment 66

[Paul Morris [:pmorris]]

OK, that makes sense, thanks for the explanation. I'll try the patch.

Comment 67

[Paul Morris [:pmorris]]

Attachment: calendar-event-dialog-sept-24.png

Here's a screenshot with the current patch. The spacing between the first four rows still seems slightly inconsistent to me, but pretty close though. (This is Linux/Ubuntu.) Let's see what it looks like on windows. I wonder if this might just be a glitch that's specific to Ubuntu and not present other Linux flavors like Aleca's?

Comment 68

[Richard Marti (:Paenglab)]

Attachment: EventWindows-sept-24.png

This is how it looks on Windows:

• The spacing between the four lines on top isn't equal.
• The "Calendar" and "Category" menulists aren't aligned on the right with the textboxes and the separator lines.
• The "Start", "End" and "Repeat" lines are too close together

Comment 69

[Khushil Mistry [:khushil324]]

Attachment: Bug-1569562_follow-up_input-width-4.patch

Comment 70

[Khushil Mistry [:khushil324]]

Attachment: event-macOS-25-Sept.png

Comment 71

[Richard Marti (:Paenglab)]

Comment on attachment 9095075 [details] [diff] [review]
Bug-1569562_follow-up_input-width-4.patch

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

On Windows for .event-input-td > input {} a margin-inline-start: 1px; and a margin-inline-end: 2px; is needed to align with the menulists.

I think the "Start", "End" and "Repeat" rows are to close when you compare with the rows above. 2px more space would look better.

::: calendar/base/themes/windows/dialogs/calendar-event-dialog.css
@@ +19,5 @@
>  }
> +
> +#event-grid-category-color-td,
> +#event-grid-item-calendar-td {
> +  padding-right: 3px !important;

Please don't use padding-right but padding-inline-end instead to support RTL languages too. And please change every such padding/margin-left/right to the -inline- variants. Only when left and right have the same value it's not needed.

Comment 72

[Paul Morris [:pmorris]]

Comment on attachment 9095075 [details] [diff] [review]
Bug-1569562_follow-up_input-width-4.patch

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

I'll hold off on reviewing until feedback from Paenglab and aleca is addressed.

Comment 73

[Alessandro Castellani [:aleca]]

Comment on attachment 9095075 [details] [diff] [review]
Bug-1569562_follow-up_input-width-4.patch

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

Please apply all the changes suggested by Richard.

Overall, I don't think this approach is the correct one. As you noticed we've been going back and forth tweaking pixels left and right because it doesn't always look consistent.
This is mostly due to the fact that we're using IDs to style sections, which it shouldn't really be the case for repeated areas, like in this case the repeated table rows.

I'd suggest to implement a dedicated class for the rows and always use that one in order to always have a consistent padding and margin, and avoid creating uniquely styled exceptions based on IDs.
A simpler and more modular CSS should be the way to go.

Comment 74

[Khushil Mistry [:khushil324]]

Attachment: Bug-1569562_follow-up_input-width-5.patch

Richard, can you please check this patch on Windows machine. I have checked on Mac and Linux and it looks okay there.

Comment 75

[Khushil Mistry [:khushil324]]

I am mainly confuse with the margin-inline-start on input. I think 4px should work. Right now, I have kept it as 1px as you have mentioned in the previous feedback. I have removed the padding, just using margins now.

Comment 76

[Richard Marti (:Paenglab)]

Comment on attachment 9100246 [details] [diff] [review]
Bug-1569562_follow-up_input-width-5.patch

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

F+ for Windows with the margin-inline-start changed to 4px. And the #keepduration-button needs a margin-bottom of -19px to be centred between the two rows. Not checked how it looks on the other platforms.

I see an issue in the lightning-item-iframe.xul: can you move the line 12 `<?xml-stylesheet type="text/css" href="chrome://calendar/skin/calendar-event-dialog.css"?>` after the line 17? Then the platform special rules will have precedence about the common rules.

::: calendar/base/themes/windows/dialogs/calendar-event-dialog.css
@@ +24,4 @@
>  }
> +
> +.event-input-td > input {
> +  margin-inline-start: 1px;

Sorry about my previous comment but this should be 4px. Or have you changed the menulist margins since the last version?

Comment 77

[Khushil Mistry [:khushil324]]

Attachment: Bug-1569562_follow-up_input-width-6.patch

Sorry, Richard for bothering you again. But now what I have done is that I have removed margins and paddings from all the elements and added padding only to tds and ths so that it looks consistent on all the operating systems. I have checked on Linux and MacOS, it looks okay there. Please give feedback.

Comment 78

[Richard Marti (:Paenglab)]

Attachment: Windows.11.10.png

This is how it looks on Windows.
As you can see, the text in the textboxes touches the border. The daypicker menulists aren't no more aligned and the textboxes in them have a strange border (there is a rule with negative margins in menulist.css that is now overridden.

Below is how they should look.

Comment 79

[Alessandro Castellani [:aleca]]

Attachment: Screenshot from 2019-10-11 09-39-28.png

I'm sorry to keep pushing back but this doesn't look good on Linux either.
The standard padding of the input fields is gone, and the datetime pickers have that initial margin with break the alignment.

Comment 80

[Khushil Mistry [:khushil324]]

Attachment: Bug-1569562_follow-up_input-width-7.patch

Comment 81

[Richard Marti (:Paenglab)]

Comment on attachment 9100740 [details] [diff] [review]
Bug-1569562_follow-up_input-width-7.patch

Thanks, now it looks good.

Comment 82

[Alessandro Castellani [:aleca]]

Comment on attachment 9100740 [details] [diff] [review]
Bug-1569562_follow-up_input-width-7.patch

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

Looks good now, thanks

Comment 83

[Paul Morris [:pmorris]]

Comment on attachment 9100740 [details] [diff] [review]
Bug-1569562_follow-up_input-width-7.patch

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

r+  I took a look and noticed a couple of minor things, but let's tackle those in a followup.  I'll post a screenshot.

Comment 84

[Paul Morris [:pmorris]]

Attachment: edit-task-glitches-oct-2019.png

Task edit dialog with the latest patch. Not sure if these things are related to the changes in the patch.

• Start and due date date picker drop-down arrows appear and disappear as I check the checkboxes, which pushes things to the right. They used to be visible even when checkboxes were unchecked.
• Height of the % complete input box is too short.

Khushil, can you take a closer look in a follow-up bug?

Comment 85

[Jorg K (CEST = GMT+2)]

Comment on attachment 9100740 [details] [diff] [review]
Bug-1569562_follow-up_input-width-7.patch

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

::: calendar/base/themes/osx/dialogs/calendar-event-dialog.css
@@ +23,3 @@
>    margin-top: -1.2em;
>    margin-bottom: 1.2em;
>  }

Please check your patches not to leave
\ No newline at end of file

I said that before.

Comment 86

[Jorg K (CEST = GMT+2)]

My apologies, the patch adds the missing newline, I got confused.

Comment 87

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/2738f47944c5
Follow-up: fix width of input fields for title and location. r=pmorris