793515 - Search results page displays a single line of tiles

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: Theme, defect)

Description

[Jim Mathies [:jimm]]

STR:

1) type a letter in the address bar

You get "Your Results" as a single line across the top of the results ui. Also this view can't be scrolled.

Comment 1

[Jim Mathies [:jimm]]

Attachment: fix

Comment 2

[Jim Mathies [:jimm]]

https://hg.mozilla.org/projects/elm/rev/9403a21f98af

Comment 3

[Jim Mathies [:jimm]]

rolling into bug 792901

Comment 4

[Jim Mathies [:jimm]]

Reopening. There were two bugs that could cause this, one will be fixed in bug 792901. 

The second bug involves initial display of autocomplete - there seems to be a layout bug with the smart grid such that on initial display of the autocomplete popup dimensional data for the top grid item is bogus. This is used in arrangeItems to calculate the layout of the grid.

STR:

1) type a letter in the address bar
result: you'll get a busted layout
2) type another letter while the autocomplete popup is still displayed
result: arrangeItems successfully sets the correct layout

Comment 5

[Sam Foster [:sfoster] (he/him)]

There is a simmilar issue going on in (true) fullscreen/metro where the bookmarks are all strung up on a single row. I assume this is related if not a manifestation of the same bug

Comment 6

[Sam Foster [:sfoster] (he/him)]

Attachment: Change to richgrid's arrangeItems to delay arrangment until the first item has height

When an item has not been previously rendered it shows up in arrangeItems with height of 10px. This change schedules arrangeItems to be called again later. 

If for some reason the item never gets height > 10 as it stands we would loop forever. I'm open to ideas on whether to either bail after n tries or n seconds, or other ways to improve how this is handled. 

Also the threshold of 10 is brittle - I suspect changes to the item css might break this. Is there a better way to know if the item's box is rendered and ready for measurement

A nice side effect of this change is that the repeated calls to arrangeItems are aborted until the first item has height. Its not true batching but helps in some cases.

Comment 7

[Matt Brubeck (:mbrubeck)]

Comment on attachment 682445 [details] [diff] [review]
Change to richgrid's arrangeItems to delay arrangment until the first item has height

Review of attachment 682445 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/metro/base/content/bindings/grid.xml
@@ +232,5 @@
> +            // delay as necessary until the item has a proper height
> +            if(gridItemRect.height <= itemHeightThreshold) { 
> +              // TODO: track # calls to ensure we don't loop endlessly?
> +              if(!this._scheduledArrangeItemsTimerId)
> +                this._scheduledArrangeItemsTimerId = setTimeout(this.arrangeItems.bind(this), 1);

If setTimeout(...,0) works, I'd prefer that to setTimeout(...,1).

Nit: Add a space after each "if" keyword, and remove trailing spaces.

I'm a bit worried about this solution in general.  It would be better if we could listen for some event or callback to tell us when the item heights were available, so we could retry exactly once.  Or maybe we can do something that forces a reflow like accessing item.offsetHeight?

r- for now until we've looked for some alternatives.

Comment 8

[Sam Foster [:sfoster] (he/him)]

Agreed, the setTimeout is always a pretty cruddy solution. I resorted to it only because I saw the same used elsewhere. Does the event you describe exist though? I don't think anything needs to block on arrangeItems, so scheduling to retry later should work - vs. needing to force the reflow by accessing offsetHeight. 

I can rework this patch to at least tuck away the nastiness so we are speaking in terms of events/callbacks? As to what fires that event/callback I'm all ears and will fall back o polling for > 10 height in the interim

Comment 9

[Matt Brubeck (:mbrubeck)]

I'm open to using setTimeout if there's not a better pre-existing event (and no, I can't think of one off the top of my head).  If we do stick with this approach, no need to hide the timeout behind an event.  My main worry is that we'll end up with an infinite timeout loop if something goes wrong, so maybe the retry limit mentioned in you comment is a good idea...

Anyway, feel free to re-request review on the current patch if you think it's the right solution for now (or upload a new version with nits addressed).  In the long term I think we might end up rewriting a lot of this code anyway, as we find ways to simplify it.  Perhaps I am over-optimistic. :)

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 682445 [details] [diff] [review]
Change to richgrid's arrangeItems to delay arrangment until the first item has height

Matt's got this one

Comment 11

[Sam Foster [:sfoster] (he/him)]

Attachment: Revised patch with max-tries to avoid infinite re-tries of arrangeItems

Comment 12

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 683064 [details] [diff] [review]
Revised patch with max-tries to avoid infinite re-tries of arrangeItems

I've reworked the arrangeItems patch a little to avoid endlessly re-scheduling if for some reason the item height never meets the condition.  Also fixed the style issues and moved a couple of the temporary/state variables into widget fields (which I /think/ is the right thing to do in this case?)

Comment 13

[Matt Brubeck (:mbrubeck)]

Comment on attachment 683064 [details] [diff] [review]
Revised patch with max-tries to avoid infinite re-tries of arrangeItems

Review of attachment 683064 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/metro/base/content/bindings/grid.xml
@@ +231,5 @@
>                return;
>              let gridItemRect = item.getBoundingClientRect();
> +
> +            // cap the number of times we reschedule calling arrangeItems
> +            let maxRetries = 5;

Nit: Let's make this "const maxRetries."  (I can fix this before checking in the patch.)