We have collected quite some boxes in the past that are not needed. We should reduce them. I have a WiP patch locally that causes a notable performance improvement. This bug should also fix the issue of the alarm box wrapping to the next line and long-word titles pushing other elements outside of the event box.

Comment on attachment 385967 [details] [diff] [review] WiP Patch - v1 Some comments on the patch, as I'm testing it at the moment... >diff --git a/calendar/base/content/calendar-bindings.css b/calendar/base/content/calendar-bindings.css >--- a/calendar/base/content/calendar-bindings.css >+++ b/calendar/base/content/calendar-bindings.css >@@ -40,18 +40,22 @@ > > calendar-day-view { > -moz-binding: url(chrome://calendar/content/calendar-views.xml#calendar-day-view); >+ -moz-user-focus: normal; > } How does this relate to the bug description? I see no relation here. Is that a leftover from another patch? Same questions goes for the other occurrences of this in this file. >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 >@@ -258,6 +241,9 @@ > } else { > this.setAttribute("value", aDate.day); > } >+ >+ // Set up the accessible label >+ this.setAttribute("aria-label", aDate.day); > ]]></body> > </method> How does an accessibility-related change relate to the bug description? I see no relation here. Is that a leftover from another patch? >diff --git a/calendar/base/content/calendar-multiday-view.xml b/calendar/base/content/calendar-multiday-view.xml >--- a/calendar/base/content/calendar-multiday-view.xml >+++ b/calendar/base/content/calendar-multiday-view.xml >@@ -1898,57 +1893,56 @@ >+ <xul:stack flex="1"> >+ <xul:hbox anonid="event-container" >+ class="calendar-item calendar-color-box calendar-event-selection" >+ xbl:inherits="readonly,flashing,alarm,allday,priority,progress,status,calendar,categories,calendar-uri,calendar-id" >+ flex="1"> >+ <xul:gradient-box anonid="gradient-stack" >+ gradientclass="calendar-event-box-gradient" >+ xbl:inherits="parentorient=orient,readonly,flashing,alarm,allday,priority,progress,status,calendar,categories" >+ flex="1"> >+ <xul:calendar-editable-label anonid="event-name" >+ xbl:inherits="selected,readonly" >+ flex="1"/> >+ </xul:gradient-box> >+ <xul:stack> >+ <xul:hbox flex="1"> >+ <xul:gradient-box flex="1" gradientclass="calendar-event-box-gradient"> >+ <xul:spacer flex="1"/> >+ </xul:gradient-box> >+ <xul:gradient-box gradientclass="category-gradient" >+ xbl:inherits="categories" >+ class="category-color-box"> >+ <xul:spacer flex="1"/> >+ </xul:gradient-box> >+ </xul:hbox> >+ <xul:vbox pack="start" flex="1"> >+ <xul:hbox anonid="alarm-icons-box" >+ class="alarm-icons-box" >+ style="padding-top: 3px" Why are we adding an inline style here? Shouldn't that better move to a css file? >@@ -1976,8 +1970,8 @@ > if (needsrelayout) { > var otherorient = "vertical"; > if (val != "horizontal") otherorient = "horizontal"; >- var eventbox = document.getAnonymousElementByAttribute(this, "anonid", "eventbox"); >- eventbox.setAttribute("orient", val); >+ // var eventbox = document.getAnonymousElementByAttribute(this, "anonid", "eventbox"); >+ // eventbox.setAttribute("orient", val); Why comment this out? Can't we just delete it? Same questions goes for the other occurrences of this in this file. >diff --git a/calendar/base/content/calendar-views.xul b/calendar/base/content/calendar-views.xul >--- a/calendar/base/content/calendar-views.xul >+++ b/calendar/base/content/calendar-views.xul >@@ -132,15 +132,19 @@ > displayed time period as described in base/public/calICalendarView.idl --> > <calendar-day-view id="day-view" flex="1" > context="calendar-view-context-menu" >+ aria-label="&calendar.day.button.label;" How does an accessibility-related change relate to the bug description? I see no relation here. Is that a leftover from another patch? Same questions goes for the other occurrences of this in this file. >diff --git a/calendar/base/themes/winstripe/calendar-views.css b/calendar/base/themes/winstripe/calendar-views.css >--- a/calendar/base/themes/winstripe/calendar-views.css >+++ b/calendar/base/themes/winstripe/calendar-views.css This patch misses the corresponding Pinstripe changes. But I guess that you are already aware of that, right? >@@ -43,10 +43,24 @@ > * ***** END LICENSE BLOCK ***** */ > > /* Core */ >-calendar-category-box:not([categories]) { >+.category-color-box:not([categories]) { > display: none; > } > >+.category-color-box { >+ /* This rule should be adapted if the alarm image size is changed */ It would probably make sense to add a comment to calendar-alarms.css as well, so that this isn't forgotten.

(In reply to comment #2) > How does this relate to the bug description? I see no relation here. Is that a > leftover from another patch? Same questions goes for the other occurrences of > this in this file. I moved all a11y related issues to a different patch. > >+ <xul:vbox pack="start" flex="1"> > >+ <xul:hbox anonid="alarm-icons-box" > >+ class="alarm-icons-box" > >+ style="padding-top: 3px" > > Why are we adding an inline style here? Shouldn't that better move to a css > file? I guess so, although I just copy&pasted this from the original file. > >+ // var eventbox = document.getAnonymousElementByAttribute(this, "anonid", "eventbox"); > >+ // eventbox.setAttribute("orient", val); > > Why comment this out? Can't we just delete it? Same questions goes for the > other occurrences of this in this file. When the patch is no longer WiP, I'll remove all commented out things. I probably need to re-implement some parts in places where I commented things out. > >diff --git a/calendar/base/themes/winstripe/calendar-views.css b/calendar > This patch misses the corresponding Pinstripe changes. But I guess that you are > already aware of that, right? Yes, I will have to test pinstripe separately, or maybe have someone with a mac take care. > >+.category-color-box { > >+ /* This rule should be adapted if the alarm image size is changed */ > > It would probably make sense to add a comment to calendar-alarms.css as well, > so that this isn't forgotten. Done. Thanks for the comments.

Argh I'm not being lucky with this bug. Although I believe everything worked when I last looked at the patch, looking at it again and processing Simons comments I must have broken the wrapping. This will probably take some time to fix so if we have all other bugs fixed for the beta, then we should simply back out the bug that caused the regression.

(In reply to comment #5) Wrapping/cropping is already regressed without your patch, see bug 509332.

(In reply to comment #6) > (In reply to comment #5) > > Wrapping/cropping is already regressed without your patch, see bug 509332. No, the whole event box extends over the column boundaries, not only the label in it.

I have backed out and reopened bug 273279, therefore this is no longer blocking the beta. It would be nice to have this after the beta though.

> I have backed out and reopened bug 273279, therefore this is no longer blocking > the beta. It would be nice to have this after the beta though. Is it fixing issue with icons ? as i reported in Bug 510647 https://bug510647.bugzilla.mozilla.org/attachment.cgi?id=394623

(In reply to comment #10) > > I have backed out and reopened bug 273279, therefore this is no longer blocking > > the beta. It would be nice to have this after the beta though. > > Is it fixing issue with icons ? as i reported in Bug 510647 > > https://bug510647.bugzilla.mozilla.org/attachment.cgi?id=394623 Just follow up. Have nightly installed. Issue is not resolved. Really ugly looking and inconsistent icons.

If bug 510647 is not fixed by this one (which is about removing boxes and improving view performance), the action would be to undup and reopen bug 510647.

To us, this bug is limiting the normal use of Calender very severely. Having to wait 5 seconds for a redraw of the screen is terrible. I hope that a solution will be found soon. I'm very interested in the progress. Please point me out how I can help.

We should take the low-risk parts of this bug for the next beta. This includes removing obvious boxes (i.e <content><xul:box>...</xul:box></content> and maybe some other minor things.

I think performance is sufficiently boosted by bug 678343, I'm not going to take the risk so shortly before the release.

Philipp, perhaps you can post some performance statistics comparing 0.9, 1.0b1, 1.0b2 and the latest RC just to show people how much things have improved because of your various changes. Bonus points if you could post those stats on the blog. :-)