Eventboxes with and without icons have different height in multiweek/month view

VERIFIED FIXED in 1.0b1

Status

defect
--
minor
VERIFIED FIXED
11 years ago
8 years ago

People

(Reporter: bv1578, Assigned: bv1578)

Tracking

unspecified
1.0b1

Details

Attachments

(5 attachments, 2 obsolete attachments)

(Assignee)

Description

11 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; it-IT; rv:1.9.0.1) Gecko/2008070208 Firefox/3.0.1
Build Identifier: Lightning 0.9pre build 2008083019

When add a reminder to an event in month view and multiweek view (alarm icon is showed), its boxes become 2 pixels higher.

Reproducible: Always

Steps to Reproduce:
1.take two empty days with side by side position in month view
2.create a new event per day so they are at the same vertical position
3.add a reminder to an event
Actual Results:  
eventbox with reminder is 2 pixels higher than the other.

Expected Results:  
eventboxes should have the same height.

works with Lightning 0.9pre 2008082218
fails with Lightning 0.9pre 2008082615
I can't find Windows version of Lghtning NB between 2008/08/22 and 2008/08/26 on http://ftp.mozilla.org/pub/mozilla.org/calendar (my fault?)
Confirmed using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.18pre) Gecko/2008083118 Calendar/0.9pre.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: [Calendar-view] Eventboxes with alarm icon have a different height → Eventboxes with and without alarm icon have different height in multiweek/month view
Flags: blocking-calendar0.9?
I personally don't think this is a blocker. While the 2px difference is certainly not what we want, its very minor. If we find an easy fix we should surely take it though.
I agree with Philipp, not a blocker IMO.
Probably caused by the checkin for Bug 437412
Flags: blocking-calendar0.9?

Comment 5

11 years ago
I at first could not reproduce this bug, but Andreas managed to showed it to me on his system where the standard fontsize is smaller thant the alarm icon. I am not yet sure how to resolve this best. Currently I calculate the minimum height of the eventbox by the size of the standard font. Maybe it's sufficiiont to also calculate the height of the alarm icon and set the maximum of both values.
As I tested I could also reproduce the bug by modifying the invitation-status, where a dotted 2px border is set. Here it's probably sufficient to set the outline instead of the border. Or if this does not help setting the -moz-box-sizing to -moz-border will probably do, too.
Severity: normal → minor
(Assignee)

Comment 6

11 years ago
Posted patch patch based on icon margin (obsolete) — Splinter Review
This patch solves the problem on my system. Alarm icon is in the middle of the boxes in month and multiweek view, but it would need another pixel on top side in day and week view.
Probably considerations in comment #5 need a more sophisticated patch.
Hope could help.
Attachment #336324 - Flags: review?(Berend.Cornelius)
Assignee: nobody → bv1578
Status: NEW → ASSIGNED
OS: Windows XP → All
Hardware: PC → All

Comment 7

11 years ago
Comment on attachment 336324 [details] [diff] [review]
patch based on icon margin

I must say I am not really happy with the patch as you replace a margin that I have  added not without reason. In a "normal" 1 hour event, that is not shrinked because of the adjacent eventboxes I can now see that the top of the  alarm icon is not propery aligned with the text label. I think Christian should decide whether we want to have this or not. Codewise it would be Ok then, although the patch is not a 100 percent solution. In certain themes with even smaller fonts the problem might still occur.

Updated

11 years ago
Attachment #336324 - Flags: ui-review?(christian.jansen)
(Assignee)

Comment 8

11 years ago
No problem :-). As I said in comment #6, alarm icon in eventboxes in day-view and week-view doesn't look perfectly, moreover, considerations in comment #5, and now your considerations, say that another way should be found.
Seemed to me that image margin on bottom side would cause the problem, maybe a 

margin-top: 2px;
-moz-margin-end: 2px;

would be better for event boxes in day-view and week-view and tolerable for multiweek-view and month-view. Sure, it remains a workaround an not a final solution.
Feel free to discard my patch, give it a r- without problems :-). 

Generally talking, about the alarm icon I think it should be shrinked and moved to the extreme right (see bug 413594 comment #6) because  month and week views need a little more space for text in eventboxes with today pane on. A smaller icon would help with smaller fonts as well.

By the way I've seen that this bug has been assigned to me, and this have to be changed, I haven't experience and background knowledge for that.
(Assignee)

Comment 9

11 years ago
Same problem occurs with taskboxes (when showed in calendar view) and generally when there is an icon inside the box.

Comment 10

11 years ago
Comment on attachment 336324 [details] [diff] [review]
patch based on icon margin

Is this bug still valide? I can't reproduce this bug. 

See attachment. The screenshot was taken on win xp.
(Assignee)

Comment 12

11 years ago
The bug is still there if you try with tasks on calendar-view and if your font is small enough (from your screenshot I'd say you are going to see it for sure) because as reported in comment #9, issue occurs with any icon in the box.
I've changed font on my WinXP from MS Sans Serif to Segoue UI, and now I can't reproduce it any more neither for alarm icon nor for task icon.
I tested with font MS Sans Serif and the bug is still there only for task icons (open tasks and completed tasks) but not for alarm icon because Berend Cornelius fixed it with patch 338815 as reported in bug 455462 comment #4:

/* Alarm image */
 .alarm-image {
     list-style-image: url(chrome://calendar/skin/alarm.png);
-    margin: 2px;
+    margin: 2px 2px 0px;
}

Maybe a similar change would also fix the issue for task icon though with a smaller font it could appear again (in truth a font smaller than 8pt is a very small font), moreover wouldn't cause problem reported in comment #7 because task icon is not present in day-view  and week-view (though I don't understand whether this is by design or is a bug).
The attachment shows boxes with task and alarm icons with system font Segoe UI and Ms Sans Serif both 8 pt size.

Comment 13

10 years ago
Comment on attachment 336324 [details] [diff] [review]
patch based on icon margin

ui+
Attachment #336324 - Flags: ui-review?(christian.jansen) → ui-review+
If there is any other way to do it, I'd like to avoid setting margins on the alarm image, since bug 353492 changes the way the alarm image is displayed. Anyway we should revise this as soon as bug 353492 has landed the UI part.
How does this bug interact with bug 413594 ? Is this issue maybe solved with the patch for that bug? If not, then it would probably make sense to just fold the fix for this one into bug 413594.
Depends on: 413594
(Assignee)

Comment 16

10 years ago
In my opinion this bug is not related with bug 413594 because depends on relation between font size (and type) and event box size. As described in comment #11, Boxes height decrease with smaller fonts while icons and their margins establish a minimum height for boxes with icons but not for boxes without icons.
Without icons the minimum height depends only on font size and could be smaller than icons height (see screen shot with font 6pt size). This causes the issue when font is small or is a particular type, e.g. 8pt for MS Sans Serif font causes the issue, but 8pt Segoe UI doesn't (on Win XP).

At the moment alarm icon doesn't have margins because, as reported in comment #14, bug 353492 changes the way the alarm image is displayed, so the minimum height for boxes with alarm icon is 12 pixels (10 for the icon and 2 for the box borders) and, on my system, the bug is visible, with alarm icons, only with font Microsoft Sans Serif 6pt, i.e. a very small font (not usable).
Instead, task icons are displayed in the "old way" and have 2px margins top and bottom side: http://mxr.mozilla.org/comm-central/source/calendar/base/themes/winstripe/calendar-views.css#429
and this causes the bug with font MS Sans Serif 8pt or smaller (see screen shot).

IMHO if you want take care of the issue even with fonts smaller than 8pt, a solution might be to decide a minimum height for event boxes not related to font size and equals to the sum of:
icons height = 10 pixels
icons margins = <- ???
boxes borders = 2 pixels
-> boxes' minimum height = 12 + ??? pixels 

but should be decided a common value for margins with alarm icons and task icons and/or a common way to display them.
(Assignee)

Updated

10 years ago
Summary: Eventboxes with and without alarm icon have different height in multiweek/month view → Eventboxes with and without icons have different height in multiweek/month view
(Assignee)

Comment 17

10 years ago
This patch sets a minimum height of 13 pixels for event boxes, 11 for icons (alarm and todo) plus 2 for boxes borders, and puts todo icon inside a box similar to alarm icon box.
In this way event boxes, with or without icon, are always of the same height even with smaller fonts.
Boxes appear like in the following screenshot, to compare with screenshot attachment 374433 [details] (where I write wrong values for boxes height: actually all boxes are 1px higher than reported in that screenshot).

Might it be a good approach?

By the way, Thunderbird has a similar problem in message header where there is a button with an icon and the others with text (see bug 456832).
Attachment #336324 - Attachment is obsolete: true
Attachment #378720 - Flags: review?(philipp)
Attachment #336324 - Flags: review?(berend.cornelius09)
(Assignee)

Comment 19

10 years ago
Same patch as attachment 378720 [details] [diff] [review] with same motivations (see comment #17), but without an error (4px margin always present).
Made obsolete attachment 378720 [details] [diff] [review].
Screenshot in comment #18 still valid.
Attachment #378720 - Attachment is obsolete: true
Attachment #378838 - Flags: review?(philipp)
Attachment #378720 - Flags: review?(philipp)
Attachment #378838 - Flags: review?(philipp) → review+
Comment on attachment 378838 [details] [diff] [review]
proposal patch (fix todo icon and minimum box height). The right one.

instead of using the minheight attribute, I'd prefer css min-height to make sure that themes that use a different alarm icon can specify a different min-height.

I'll fix this before checkin.

r=philipp
Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/d4e0ef5c3687>

-> FIXED
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.0
verified with
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.9pre) Gecko/20100305 Calendar/1.0b2pre
Status: RESOLVED → VERIFIED
These bugs are likely targeted at Lightning 1.0b1, not Lightning 1.0. If this change was done in error, please adjust the target milestone to its correct value. To filter on this bugspam, you can use "lightning-10-target-move".
Target Milestone: 1.0 → 1.0b1
You need to log in before you can comment on or make changes to this bug.