501302 - Improve View performance by removing unneeded boxes

Status: NEW

(Calendar :: Calendar Frontend, defect, P3)

Description

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

Attachment: WiP Patch - v1

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 2

[Simon Paquet [:sipaq]]

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.

Comment 4

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

(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.

Comment 5

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

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.

Comment 6

[Stefan Sitter [:ssitter]]

(In reply to comment #5)

Wrapping/cropping is already regressed without your patch, see bug 509332.

Comment 8

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

(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.

Comment 9

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

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.

Comment 10

[jigar shah]

> 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

Comment 11

[jigar shah]

(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.

Comment 15

[Alfred Kayser]

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.

Comment 18

[Rene Balvers]

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.

Comment 19

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

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.

Comment 20

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

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

Comment 21

[Simon Paquet [:sipaq]]

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. :-)

Comment 22

[Magnus Melin [:mkmelin]]

Philipp, I'm assuming you're not currently working on this. Something we should look into though.