Improve View performance by removing unneeded boxes (fixes regression: alarm icon cropped, misplaced)

ASSIGNED
Assigned to

Status

Calendar
Calendar Views
ASSIGNED
9 years ago
5 years ago

People

(Reporter: Fallen, Assigned: Fallen)

Tracking

(Blocks: 1 bug, {perf})

unspecified
Dependency tree / graph
Bug Flags:
blocking-calendar1.0 +

Details

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

Attachments

(1 attachment)

(Assignee)

Description

9 years ago
Created attachment 385967 [details] [diff] [review]
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.
(Assignee)

Updated

9 years ago
Duplicate of this bug: 501214
Flags: blocking-calendar1.0?
(Assignee)

Updated

9 years ago
Blocks: 474607
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.
(Assignee)

Updated

9 years ago
Duplicate of this bug: 505671
(Assignee)

Updated

9 years ago
Summary: Improve View performance by removing unneeded boxes → Improve View performance by removing unneeded boxes (fixes regression: alarm icon cropped, misplaced)
(Assignee)

Updated

9 years ago
Flags: blocking-calendar1.0? → blocking-calendar1.0+
(Assignee)

Updated

9 years ago
Whiteboard: [needed beta][no l10n impact]
(Assignee)

Comment 4

9 years ago
(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.
(Assignee)

Comment 5

9 years ago
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.
(Assignee)

Updated

9 years ago
Duplicate of this bug: 510647
(Assignee)

Comment 8

9 years ago
(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.
(Assignee)

Comment 9

9 years ago
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.
Whiteboard: [needed beta][no l10n impact] → [not needed beta][no l10n impact]

Comment 10

9 years ago
> 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

9 years ago
(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.
(Assignee)

Updated

8 years ago
Duplicate of this bug: 420615
(Assignee)

Updated

8 years ago
Duplicate of this bug: 457013
(Assignee)

Updated

8 years ago
Duplicate of this bug: 462283

Comment 15

8 years ago
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.
(Assignee)

Updated

8 years ago
Duplicate of this bug: 481033
(Assignee)

Updated

8 years ago
Duplicate of this bug: 471028

Comment 18

8 years ago
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.
(Assignee)

Updated

7 years ago
Blocks: 530423
(Assignee)

Comment 19

7 years ago
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.
Whiteboard: [not needed beta][no l10n impact] → [needed beta][no l10n impact]
(Assignee)

Comment 20

7 years ago
I think performance is sufficiently boosted by bug 678343, I'm not going to take the risk so shortly before the release.
Whiteboard: [needed beta][no l10n impact] → [not needed beta][no l10n impact]
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. :-)

Updated

5 years ago
Keywords: perf
Whiteboard: [not needed beta][no l10n impact] → [not needed beta][no l10n impact][patchlove]
You need to log in before you can comment on or make changes to this bug.