Closed Bug 455045 Opened 12 years ago Closed 10 years ago

current day highlight should trump currently selected day

Categories

(Calendar :: Calendar Views, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: davida, Assigned: bv1578)

References

Details

(Whiteboard: [needed beta][no l10n impact])

Attachments

(7 files, 2 obsolete files)

Upon loading Sunbird 0.9rc1, I thought there was a bug that "today" was being highlighted on every friday in the week view.  After talking to mschroeder, i found out that the orange highlight corresponds to the selected day, not to today.  If I click on another day, then the current day highlight was visible.

AFAICT, the only point of selection highlight is to indicate where events that have been cut/copied can be pasted.  It seems to me that such a highlight is a lot less important than being able to quickly find 'today'.

I suggest shamelessly copying iCal:

1) make the "today" highlight override the "selected day" highlight when the two are on the same day
2) make identifying today easier than identifying the selected day, by making bold apply to today, not to the selected day, and making the selected day highlight a less bold color than the current day.

cc'ing UX folks for their take on it. 

(note that for the copy/paste use case, the user is likely to click on the day they're going to paste to as part of the interaction, so even a subtle highlight of the target day will be noticed)
New Event, the most prominent toolbar button, creates an event on the selected day.  (New Task does not yet, that is bug 411849.)
I was about to fill the same bug and yes it is really annoying when you use a multiweek or month view - the selected day and Today should not use the same highlighting mechanism (as it is now both state change cell background color), because one day can be both 'selected' and 'Today'.

Actually there are many way a day can be highlighted and it should not be too difficult to find the best combination, keeping in mind that probably Today highlighting is probably more important and should stand out more evidently (highlighting after selection is just a feedback that the click has been taken into consideration thus probably don't need to be too obvious - highlighting Today is important because it quickly shows where you are in the moonth, how many week after or before Today...)

Just for fun, highlighting method I can suggest:
- changing the day background color
- making the font for the day number bold
- changing the color of the day number
- changing the color of the day number background
- increasing the width of the line around the day (making it bold)
- changing the color of the line around the day
and of course a combination of these methods.

I think that Today should be prominent thus having a different background color and having a bold day number would be nice, as suggested by the reporter :-)
For selection following Excel could be nice: getting the cell darker (whatever it original background color) and getting the line around the day bolder. 

Notice that for higlighting selection it may be logical to follow the highlighting we get when we click on a calendar name - blue background with white font on Windows, but I guess it changes with OS - because after all it is just a selection... currently the pale orange color is really weird...
Attached image screenshot: proposal
What about something like that in the screenshot? 
I have a almost-complete patch if this is what you are looking for.
Attachment #385338 - Flags: review?(clarkbw)
Attachment #385338 - Flags: review?(clarkbw) → ui-review?(clarkbw)
Comment on attachment 385338 [details]
screenshot: proposal

nice, this looks a lot clearer
Attachment #385338 - Flags: ui-review?(clarkbw) → ui-review+
(In reply to comment #3)
> Created an attachment (id=385338) [details]
> screenshot: proposal
> 
> What about something like that in the screenshot? 
> I have a almost-complete patch if this is what you are looking for.

This got ui-review+. Are you going to implement this? :)
Assignee: nobody → bv1578
Status: NEW → ASSIGNED
A couple changes are needed:

1. Selected day may be full, hiding background

The currently implemented scheme is:

is           is       : box     box    date      date
selected?    today?   : border  bgnd   font      bgnd
------------ ---------: ------  -----  --------  ----
non-selected non-today: gray    white  non-bold  gray
selected     non-today: gray    tan    bold      tan  
selected     today    : gray    tan    bold      tan 
non-selected today    : gray    blue   non-bold  blue

From attachment 385338 [details] it looks like the proposal is:

is           is       : box     box    date      date
selected?    today?   : border  bgnd   font      bgnd
------------ ---------: ------  -----  --------  ----
non-selected non-today: gray    white  non-bold  gray
selected     non-today: gray    tan   *NON-BOLD  tan  
selected     today    :*BLUE    tan    bold     *BLUE
non-selected today    :*BLUE    blue  *BOLD      blue

i.e.
- add blue border for today (nice);
- associate date font with isToday, not isSelected.
- associate date bgnd with isToday, not isSelected.

PROBLEM: the only marker left for selection is a tan box background,
but box background may be hidden if boxes are full of events!  I
suggest the following:

is           is       : box     box    date      date
selected?    today?   : border  bgnd   font      bgnd
------------ ---------: ------  -----  --------  ----
non-selected non-today: gray    white  non-bold  gray
selected     non-today: gray    tan   *NON-BOLD  tan  
selected     today    :*BLUE    tan    bold     !tan
non-selected today    :*BLUE    blue  *BOLD      blue

i.e, rely on the blue border and bold (blue?) font to mark today,
and keep the tan background on the date header so it is visible
even when the day is filled with events.

2. day of week column head for multiple weeks may be misleading

In multiweek/month view, making the day of week header blue seems
confusing and should be unchanged, not blue.  (It might
mislead the user to think friday the 19th [in the first row] is today,
especially if it is filled with events.)
Attached patch patch (obsolete) — Splinter Review
I guess observation 2 in comment #6 is valid for day view too, isn't it?
In this case differences introduced by this patch are only these:

-Multiweek/Month view:
  border for today box;
  bold for today date number;
  bold for weekday label in column header when today is in the view;
  non-bold for selected day date number;

-Multiday view:
  border for today column;
  bold for weekday label corresponding to today;
  non-bold for weekday label corresponding to selected day;

-Day-view:
  bold for today label;


I modified the yellow color to a clearer one to distinguish selected events (just an idea), moreover, I modified today and selected-day in minimonth to match changes on calendar views.

I changed border in calendar-day-box class:
 .calendar-day-box {
-    border-right: 1px solid transparent;
+    border-right: 1px solid #D2D2D2; 
 }
it seems to me that look of week view is better with a border on the right side but I don't know whether there is a reason for a transparent border.

I added a bottom border in rotate view to get the same look as normal view:
box [anonid="daybox"] {
    border-bottom: 1px solid #D2D2D2;
}
but this border appears and disappears when window vertically resize. It seems this border improve misalignment of horizontal lines in rotate view (normally present in rotate view), it doesn't totally delete it though.

The patch has two problems and I need help to resolve them:
1. in week view, when today is the last day to the right, the border around the day leaks of the column header part (that one where allday events lie) on the right side. I've tried in different ways but I'm not able to make it appear changing only css properties;
2. in rotate view (week view), when today is the last day (bottom), the border  near the scroll bar appears or disappears when window vertically resizes, like the border mentioned above does. 

Patch needs testing on Linux and Mac.
Attachment #386123 - Flags: review?(mschroeder)
Looks as proposed on Linux. My only suggestion would be to take a slightly softer blue for the border color.
Attached image Different blue borders
What kind of blue (or other color) is better?
Otherwise you may give me the right rgb color because my monitor isn't exactly the best to notice differences with such thin lines.
I'd suggest #67ACD8. For some reason in the app the blue of the patch looks much more blue than it does in your screenshot.
OK, I'm going to add this color after the review.

There is another question I want ask for, it could be matter for another bug but it could be solved adding a code line to this patch.
In week view, all day events have no padding around header boxes, instead, normal events, have 1px right and left sides, IMO it would be better add this padding for header-container too:

 calendar-header-container {
     background-color: #FFFFFF;
     border-left: 1px solid #D2D2D2;
+    padding: 1px;
 }

effect is showed in the next screenshots.
Change described in comment #11.
Comment on attachment 386123 [details] [diff] [review]
patch

Here is a first review comment. But most important is the solution of the outstanding problems which were mentioned in comment#7 before we can proceed. This is possibly related to some margins defined in CSS. Decathlon, what have you already tried to get this working? I don't have the time to test this myself at the moment, so it has to wait until beginning of August.

>diff --git a/calendar/base/content/calendar-month-view.xml b/calendar/base/content/calendar-month-view.xml
>--- a/calendar/base/content/calendar-month-view.xml
>+++ b/calendar/base/content/calendar-month-view.xml
>@@ -900,12 +900,26 @@
> 
>           // Highlight today, if it's in the range of the view
>           if (today.compare(dateList[0]) != -1 &&
>               today.compare(dateList[dateList.length-1]) != 1) {
>               this.findDayBoxForDate(today).setAttribute("today", "true");
>           }
>+
>+          // Highlight today in column header if it's in the range of the view
>+          let labeldays = this.labeldaybox.childNodes;
>+          for (let i=0; i < labeldays.length; i++) {
>+              if (today.compare(dateList[0]) != -1 &&
>+                  today.compare(dateList[dateList.length-1]) != 1 &&
>+                  labeldays[i].weekDay == today.weekday) {
>+                  
>+                  labeldays[i].setAttribute("relation", "todayInView");
>+              } else {
>+                  labeldays[i].removeAttribute("relation");
>+              }
>+          }
>+

Can these two code blocks be merged in some way?
Attached patch patch_v2 (obsolete) — Splinter Review
(In reply to comment #13)
> (From update of attachment 386123 [details] [diff] [review])
> ...
> Decathlon, what have
> you already tried to get this working? I don't have the time to test this
> myself at the moment, so it has to wait until beginning of August.
> 

I tried a lot of combinations of borders, margins, overlapped boxes but I'm only able to make appear a misplaced border. At last I've used the workaround present in this new patch. It works fine but most probably isn't the best solution.
Since the only border I have completely control on is the right border of 'calendar-header-day-box' box (the box that contains all seven allday-event boxes), I've added to it an attribute "todaylastinview" which is true when today is the last day to the right, then I've used this attribute to change the border color to match that one of today.
If you find a solution that doesn't need that attribute, the relative code could be deleted.

> >diff --git a/calendar/base/content/calendar-month-view.xml b/calendar/base/content/calendar-month-view.xml
> >--- a/calendar/base/content/calendar-month-view.xml
> >+++ b/calendar/base/content/calendar-month-view.xml
> >@@ -900,12 +900,26 @@
> > 
> >           // Highlight today, if it's in the range of the view
> >           if (today.compare(dateList[0]) != -1 &&
> >               today.compare(dateList[dateList.length-1]) != 1) {
> >               this.findDayBoxForDate(today).setAttribute("today", "true");
> >           }
> >+
> >+          // Highlight today in column header if it's in the range of the view
> >+          let labeldays = this.labeldaybox.childNodes;
> >+          for (let i=0; i < labeldays.length; i++) {
> >+              if (today.compare(dateList[0]) != -1 &&
> >+                  today.compare(dateList[dateList.length-1]) != 1 &&
> >+                  labeldays[i].weekDay == today.weekday) {
> >+                  
> >+                  labeldays[i].setAttribute("relation", "todayInView");
> >+              } else {
> >+                  labeldays[i].removeAttribute("relation");
> >+              }
> >+          }
> >+
> 
> Can these two code blocks be merged in some way?

Oh yes, they can:

+    // Highlight box and column header corresponding to today if it's 
+    // in the range of the view
+    for (let i=0; i < this.labeldaybox.childNodes.length; i++) {
+        this.labeldaybox.childNodes[i].removeAttribute("relation");
+    }
     if (today.compare(dateList[0]) != -1 &&
         today.compare(dateList[dateList.length-1]) != 1) {
         this.findDayBoxForDate(today).setAttribute("today", "true");
+        this.labeldaybox.childNodes[(7+today.weekday-this.weekStartOffset)%7]
+                            .setAttribute("relation", "todayInView");
         }


I've also add the blue color proposed by Philipp in comment #10 but IMHO it's a bit clear, it gives a few contrast in particular when there are a lot of events in today box and in the next box.
Attachment #386123 - Attachment is obsolete: true
Attachment #389648 - Flags: review?(mschroeder)
Attachment #386123 - Flags: review?(mschroeder)
Duplicate of this bug: 510289
This is another idea, from the Palm Pre. Simply circling the date that is today is very obvious compared to yellow vs. blue vs. gray. 

Not sure how easy it is, or if there's some IP problems inherent in this, but it might make a good solution.
Flags: blocking-calendar1.0+
Whiteboard: [not needed beta][no l10n impact]
I just want to mention that this is the number one bug I notice whenever I use Lightning. Suggest to increase "importance" and to set a "target milestone".
Whiteboard: [not needed beta][no l10n impact] → [needed beta][no l10n impact]
Whiteboard: [needed beta][no l10n impact] → [needed beta][no l10n impact][needs review]
Comment on attachment 389648 [details] [diff] [review]
patch_v2

Peter, we rather use the target milestone to easier figure out which bugs we've fixed for the release, i.e we only set it when the bug is fixed. This bug is marked [needed beta], so its actual target will be the next beta release.


Decathlon, could you summarize what the remaining issues are in this patch? I'm happy to take over review (if Martin doesn't mind) so we can get this bug fixed.



Code level review follows:



>+calendar-day-label[relation="todayInView"] {
>+    background-color: white;
>+}
Could you check what happens here on a high-contrast theme? Maybe we should use a system color here?



>+calendar-header-container[orient="vertical"][relation="today"] {
>+    border-left: 1px solid #67ACD8 !important;
>+    border-right: 1px solid #67ACD8 !important;
>+    -moz-margin-end: -1px !important;
>+    position: relative;
>+}
Interesting, I've never seen use of the css position property in XUL. Just out of curiosity, could you tell me why its needed here?

> .minimonth-day[today="true"] {
>   background-color: #dfeaf4;
>-  border-top: 1px solid #BAC4CC;
>-  border-bottom: 1px solid #E8F5FF;
>-  border-left: 1px solid #BAC4CC;
>-  border-right: 1px solid #E8F5FF;
>+  border: 1px solid #67ACD8;
> }
Does changing the minimonth appearance belong to this patch? I think the idea for the different-colored borders here was to give it a 3d-effect.
Attachment #389648 - Flags: review?(mschroeder) → review?(philipp)
(In reply to comment #18)

> >+calendar-day-label[relation="todayInView"] {
> >+    background-color: white;
> >+}
> Could you check what happens here on a high-contrast theme? Maybe we should use
> a system color here?

Actually that rule is a mistake from the first patch I wrote, where the color in the week header was different, see attachment 385338 [details]. Since in comment #6 (point 2.) was told not to use a color for today in the week header in multiweek/month views, I badly changed that color to a white one (the default color of calendar-day-label) instead of deleting the rule.
However, white is the default background color of the element calendar-day-label  http://mxr.mozilla.org/comm-central/source/calendar/base/themes/winstripe/calendar-views.css#321, so if there is a problem with high-contrast themes, it concerns all the week header (by the way, could you suggest me an updated high-contrast theme for TB 3.1 to try with?).
About this element I wish to propose another solution: giving the border color to the box with the day name in the week header when today is in the view, for all the views (see screenshot, first row). It's a redundant information because today already has a bold font, but it's nice in particular for day/week views. What's your opinion?
 
 
> >+calendar-header-container[orient="vertical"][relation="today"] {
> >+    border-left: 1px solid #67ACD8 !important;
> >+    border-right: 1px solid #67ACD8 !important;
> >+    -moz-margin-end: -1px !important;
> >+    position: relative;
> >+}
> Interesting, I've never seen use of the css position property in XUL. Just out
> of curiosity, could you tell me why its needed here?

I'm not a css expert, probably it could be achieved in another way (maybe assigning a class to the calendar-header-container?), but the goal is to make the right border of today visible as showed in the screenshot (second row). With the positioning, the right border of today, with margin -1px, is stacked over the left border of tomorrow so it's visible. Is it wrong? Should I do it in a different way?

As I had already written in comment #14 and comment #7, I can't make this rule working with the last day on the extreme right (screenshot), i.e. I can't put the right border of the "calendar-header-container" _over_ the right border of "calendar-header-day-box" (that is a container for all the former boxes). For this reason I've used the "todaylastinview" attribute that allows to change the border color to "calendar-header-day-box" when today is the last day on the right.
The final result is the same and the patch works fine, but I see, it isn't the best of the elegance, if you have a better solution...


> Does changing the minimonth appearance belong to this patch? I think the idea
> for the different-colored borders here was to give it a 3d-effect.

I'm aware of that, but the minimonth has the same problem of the calendar views i.e. if the selected day is today, you can't see if today is in the month because it becomes like any other selected day ("today doesn't trump the selected day" not even in the mininmonth).
My idea was to apply to the minimonth the same look of the views, so the blue border always shows today even if today is selected (see again the screenshot). If it needs a different solution, a different look or no solution at all, just let me know.


> Decathlon, could you summarize what the remaining issues are in this patch? 

I have to fix the calendar-day-label rule, moreover I want to change the code that assigns the relations "today", "past", "future", if you could tell me about the questions mentioned above, I'm going to attach an updated patch.
> However, white is the default background color of the element
> calendar-day-label 
> http://mxr.mozilla.org/comm-central/source/calendar/base/themes/winstripe/calendar-views.css#321,
> so if there is a problem with high-contrast themes, it concerns all the week
> header (by the way, could you suggest me an updated high-contrast theme for TB
> 3.1 to try with?).
Thats fine. If we have issues with contrast then we can fix them in a different bug, we just shouldn't add more rules to work against it. Thanks for removing.

With high contrast, I rather mean the windows/linux default themes that provide high contrast, compare http://www.tunexp.com/tips/customize_your_computer/turn_on_high_contrast/

> About this element I wish to propose another solution: giving the border color
> to the box with the day name in the week header when today is in the view, for
> all the views (see screenshot, first row). It's a redundant information because
> today already has a bold font, but it's nice in particular for day/week views.
> What's your opinion?
I'll leave the final UI decisions to andreasn/clarkbw, but I think it looks great.

> over the left border of tomorrow so it's visible. Is it wrong? Should I do it
> in a different way?
Ah ok, now I understand. I wasn't saying what you did is wrong, rather I didn't see the reasoning behind it. Sorry for not reading the right comments! Sounds fine, lets do it that way :)

> As I had already written in comment #14 and comment #7, I can't make this rule
> working with the last day on the extreme right (screenshot), i.e. I can't put
> the right border of the "calendar-header-container" _over_ the right border of
> "calendar-header-day-box" (that is a container for all the former boxes). For
> this reason I've used the "todaylastinview" attribute that allows to change 
You could try using the :last-child pseudo-class? If not, then lets stick with the attribute.

> I'm aware of that, but the minimonth has the same problem of the calendar views
> i.e. if the selected day is today, you can't see if today is in the month
Ok, looking at your screenshot it makes perfect sense :)
Comment on attachment 389648 [details] [diff] [review]
patch_v2

Removing review request, since you said you wanted to fix a few issues.
Attachment #389648 - Flags: review?(philipp)
Attached patch patch - v3Splinter Review
The patch that fixes bug 467908 has changed the attribute "orient" for the element calendar-header-container (now the orientation is vertical for both normal and rotated views), so I've added a new attribute "rotated" in order to handle some css rule for borders.

I've deleted some "!important" statements in the css files, but I've noticed that the behavior is different with Lanikai and Shredder so I've left some statement to make the patch working with both.

I've also added the bottom border for today in the week view that I had completely missed.

About the issue mentioned in the previous comments, I have to raise the white flag. I'm not able to make it working without the "todaylastinview" attribute. Maybe the solution is simple but I'm not able to find it. The ":last-child" pseudo-class allow me just to identify the correct box but then it needs a rule for the correct border positioning. The only certainty is that the more obvious solution of deleting the right border to the class calendar-header-day-box in order to substitute it with the right border of the last calendar-header-container box, can't be done because this causes a borders misalignment for all the boxes calendar-header-container.
I think the solution should be to stack the child box over its parent but my little mind and my poor css knowledge don't allow me to find a way.
Attachment #389648 - Attachment is obsolete: true
Attachment #472993 - Flags: review?(philipp)
Comment on attachment 472993 [details] [diff] [review]
patch - v3

(In reply to comment #22)
> About the issue mentioned in the previous comments, I have to raise the white
> flag. I'm not able to make it working without the "todaylastinview" attribute.
> Maybe the solution is simple but I'm not able to find it. The ":last-child"
I've tried to make it work purely with CSS, but I've also failed. I though maybe making the parent border transparent works, but also the closest I've come is a matching right border with the misalignment you talk about. Therefore lets just keep the attribute for now, maybe this will be fixed with the next toolkit version.

> I think the solution should be to stack the child box over its parent but my
> little mind and my poor css knowledge don't allow me to find a way.
I think the problem is that we use the XUL equalsize="always" on the parent box. This is fine, since we have a nice distribution of the day header boxes, but it seems to be causing problems with the margin showing. I think you've done a great job to work around the issues that XUL is having, so don't take the fault on you!




>+                      let relation_ = this.numVisibleDates == 1 ? "today1day" : "today";
>+                      dayHeaderBox.setAttribute("relation", relation_);
>+                      dayEventsBox.setAttribute("relation", relation_);
>+                      labelbox.setAttribute("relation", relation_);
>+                      if (dayHeaderBox == headerdaybox.childNodes[headerdaybox.childNodes.length-1] &&
>+                           this.numVisibleDates > 1) {
>+                          headerDayBox.setAttribute("todaylastinview", "true");
>+                      }
I'm going to add a comment here, why we need this attribute together with a reference to this bug. This way we'll know what the problem is later on.


Aside from that, r=philipp.
Attachment #472993 - Flags: review?(philipp) → review+
Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/bc64f8b8655b>

-> FIXED
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [needed beta][no l10n impact][needs review] → [needed beta][no l10n impact]
Target Milestone: --- → 1.0b3
Testing Lightning 1.0b3pre, I noticed a new problem in the display of the 'Day view' and 'Week view', that was probably introduced by fixing this bug.


1. Extra space on the right border
----------------------------------
An extra space of a few pixels appears between the event boxes and the right border of every day column in the 'Day view' and 'Week view'. All day events don't have that problem.

I will upload a screenshot to show that problem.
Should I report this as a new bug ?


Today and Selected day colors
-----------------------------
As stated by Decathlon in Comment 14, I also think the blue border is not dark enough to have a good contrast.

But of course, each one has its own idea of what's the best color to use ...
I heard many times people asking how to change the 'Today' and 'Selected day' colors. Right now, this can be achieved by using a Userchrome.css file or with
the 'Calendar Tweaks' addon that mimics the colors and styles found in MS Outlook ( https://addons.mozilla.org/en-US/thunderbird/addon/14384/ ).

That's why I think it would make sense to give the users an easy way to modify these two colors.

Making this choice possible in the Ligtning Options dialog is probably too much work, but making these two colors a pref. value that we can modify with the Config Editor should be easy ?
(In reply to comment #26)
> An extra space of a few pixels appears between the event boxes and the right
> border of every day column in the 'Day view' and 'Week view'.

Intentionally added by Bug 367131.
You need to log in before you can comment on or make changes to this bug.