Closed Bug 925425 Opened 7 years ago Closed 7 years ago

Avoid jarring reflows when populating start grids


(Firefox for Metro Graveyard :: Firefox Start, defect)

Windows 8
Not set


(Not tracked)

Firefox 27


(Reporter: sfoster, Assigned: sfoster)




(2 files)

The content of Firefox Start jumps around a lot as each of the grids is populated. We could avoid a lot of that by having each grid manage a collection of fixed-size slots, which are populated as data becomes available. Toggling between an empty and populated state might also become transition-able then as the DOM node is retained, no destroyed and recreated each time.
Attached patch Misc nitsSplinter Review
I've pulled out these nits from the main patch as they arent really richgrid slots related but provide a more level surface to build on
Assignee: nobody → sfoster
Attachment #817457 - Flags: review?(mbrubeck)
To avoid progressive reflows and the jarring build-in we had as the start grids got data, this patch adds the notion of slots to the richgrid binding, which are assigned and populated by items. A grid can have empty slots if the number of items is less than the value of minSlots (3 by default). 

I've spent a while on new tests and further rationalizing the arrangeItems method so the expected thing happens. 

I'll file a new bug for implementing transitions of the grids. This patch just adds basic styles for empty slots, to match
Attachment #817461 - Flags: review?(mbrubeck)
Attachment #817457 - Flags: review?(mbrubeck) → review+
Comment on attachment 817461 [details] [diff] [review]
Richgrid slots and empty items implementation

Review of attachment 817461 [details] [diff] [review]:

This does a great job reducing layout changes as the start page loads.  I have some nit-picky code comments, and some UX questions that we can deal with in followups:

::: browser/metro/base/content/bindings/grid.xml
@@ +230,5 @@
> +            let childIndex = 0;
> +            let child = this.firstChild;
> +            while (child) {
> +              // remove excess elements and non-element nodes
> +              if (child.nodeType !== 1 || childIndex+1 > slotCount) {

Why remove non-element nodes?

Nit: Please use "Components.interfaces.nsIDOMNode.ELEMENT_NODE" instead of "1" -- but you can add a const alias for it on a separate line for brevity.

@@ +240,5 @@
> +              if (child.hasAttribute("value")) {
> +                this._releaseSlot(child);
> +                child = child.nextSibling;
> +              }
> +              childIndex++;

You can reach the bottom of this loop without changing "child" and end up testing the same node repeatedly.  Should the "child = child.nextSibling;" be outside of the "if" statement above?

After some more thought: I guess we have an invariant that the empty slots always come after the non-empty items, so once we get to an empty item we can safely stop iterating?  Even if this is the case, it still seems worthwhile to make the change above though since it makes the code easier to reason about (for me, at least).

@@ +245,3 @@
>              }
> +            // create our quota of item slots
> +            for(let count = this.childElementCount; count < slotCount; count++) {

Nit: space after "for"

@@ +274,5 @@
> +            if (!this.itemCount) {
> +              return this._slotAt(0);
> +            }
> +            let lastItem = this.items[this.itemCount-1];
> +            let nextIndex = 1+Array.indexOf(this.children, lastItem);

Optional style nit: I personally like spaces around "+" for readability at a glance.

@@ +279,5 @@
> +            return this._slotAt(nextIndex);
> +          ]]>
> +        </body>
> +      </method>
> +      <method name="_releaseSlot">

Nit: blank line between methods, please.  (It helps readability for me.)

@@ +287,5 @@
> +            // Flush out data and state attributes so we can recycle this slot/element
> +            let exclude = { value: 1, tiletype: 1 };
> +            let attrNames = [ for (attr of anItem.attributes)];
> +            for (let attrName of attrNames) {
> +              if(!(attrName in exclude))

Nit: space after "if"

@@ +301,1 @@
>        <method name="insertItemAt">

Blank line again.

@@ +311,5 @@
>              if (existing) {
> +              // use an empty slot if we have one, otherwise insert it
> +              let childIndex = Array.indexOf(this.children, existing);
> +              if (childIndex > 0 && !this.children[childIndex-1].hasAttribute("value")) {
> +                insertedItem = this.children[childIndex-1];

Does this ever happen that an empty slot comes before an item?  That would violate the invariant that I conjectured above.

@@ +541,5 @@
>              }
>              let itemDims = this._itemSize;
>              let containerDims = this._containerSize;
> +            let slotsOrItemsCount = Math.max(this.itemCount, this.childElementCount);

Isn't this always equal to this.childElementCount (because items are a subset of child elements)?

@@ +611,5 @@
>        <field name="_xslideHandler"/>
>        <constructor>
>          <![CDATA[
> +          // create our quota of item slots
> +          for(let count = this.childElementCount, slotCount = this.minSlots;

Nit: Space after 'for'

::: browser/metro/base/content/startui/Start.xul
@@ +47,5 @@
>          <label class="meta-section-title wide-title" value="&topSitesHeader.label;"/>
>          <html:div class="meta-section-title narrow-title" onclick="StartUI.onNarrowTitleClick('start-topsites')">
>            &narrowTopSitesHeader.label;
>          </html:div>
> +        <richgrid id="start-topsites-grid" observes="bcast_windowState" set-name="topSites" rows="3" columns="3" tiletype="thumbnail" seltype="multiple" minSlots="9" fade="true" flex="1">

Shouldn't this have minSlots="8"?

@@ +68,5 @@
>          </html:div>
> +        <richgrid id="start-bookmarks-grid" observes="bcast_windowState" set-name="bookmarks" seltype="multiple" fade="true" flex="1">
> +          <richgriditem/>
> +          <richgriditem/>
> +          <richgriditem/>

Similarly, we are down to 2 default bookmarks and so it might be less confusing to start with two empties (to avoid showing a "ghost" tile by default).

::: browser/metro/theme/tiles.css
@@ +281,5 @@
> +
> +richgriditem:not([value]) > .tile-content {
> +  padding: 10px 14px;
> +  box-shadow: 0px 0px 0px 1px rgba(0, 0, 0, 0.05);
> +  background-image: url("chrome://browser/skin/images/firefox-watermark.png");

For non-thumbnail tiles (i.e. outside of the Top Sites grid), I wonder if it would be less confusing to make the empty items invisible placeholders.  That is, they would still take up space in the layout to avoid reflows, but would be completely transparent/invisible.

And if a grid had only empty tiles, we could hide the entire grid and heading.  Then Recent History would be hidden on first run (just like Remote Tabs already is).

We don't need to decide this now, but maybe worth a followup with shorlander's input.
Attachment #817461 - Flags: review?(mbrubeck) → review+
On fx-team:

I got all the nits - and a few others nearby - taken care of. Also fixed a test where I check the slots aren't bound by richgrid-item. To answer your other questionss: 

> Why remove non-element nodes?
Partly force of habit, we don't want to end up accumulating whitespace nodes somehow as that collection is manipulated. 

> You can reach the bottom of this loop without changing "child"
Good catch. Fixed. You're correct that items should always be contiguous and slots being the remaining elements after the items. Provided the binding's methods are used to manipulate the collection (vs. DOM-level manipulation) this will always be true. There's some coverage of this as it applies to normal operation in the tests. 

> ...make the empty items invisible placeholders
I went ahead and did this, making richgriditem:not([value]) be visible:hidden; I'm happy to revisit this if we think its necessary. In any case, I think empty grid handling should probably be done by the View, not in CSS or this binding.
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 27
You need to log in before you can comment on or make changes to this bug.