Closed Bug 858388 Opened 7 years ago Closed 7 years ago

Work - "Internet Searches" results grid doesnt layout correctly (appears horizontally/vertically in regular/snapped view)

Categories

(Firefox for Metro Graveyard :: Browser, defect, P1)

x86_64
Windows 8.1
defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: kjozwiak, Assigned: sfoster)

References

Details

(Whiteboard: feature=work [will be fixed by new grid])

Attachments

(2 files, 1 obsolete file)

When pasting a URL into the URL App Bar that has never been used before, the "Internet Searches" results will be displayed horizontally instead of vertically. Attached a screenshot to illustrate the issue occurring. Reproduced the issue on two different machines.

Steps to reproduce the issue:

1) Cut a URL from any browser (make sure its a full URL and has never been used before in Firefox Metro)
2) Open Firefox Metro
3) Select the URL navigation app bar and paste the URL (CTRL + V)

Current Behavior:

- Internet Searches appearing horizontally when pasting a URL of a website that has not been visited before

Expected Behavior:

- Internet Searches should be displaying vertically (should be consistent throughout)
consideration for Iteration #6
Flags: needinfo?(mmucci)
We have certainly seen this behavior in the past, but I can't reproduce this with the steps above. I get the 7 internet searches tiles in a single vertical column. Maybe display/window size is a factor? This device (Vaio duo 11) is 1600x900.
I can reproduce every time, I'm on low-dpi and 1366x768 resolution.  I can just take this in it6 since I can reproduce if no one else grabs it.
For consideration in Iteration #6.
Flags: needinfo?(mmucci)
Whiteboard: feature=defect c=firefox_app_bar_and_autocomplete u=metro_firefox_user p=0
Blocks: metrov1planning
No longer blocks: metrov1it6
Blocks: metrov1defect&change
No longer blocks: metrov1planning
actually let's call it p=1
Whiteboard: feature=defect c=firefox_app_bar_and_autocomplete u=metro_firefox_user p=0 → feature=defect c=firefox_app_bar_and_autocomplete u=metro_firefox_user p=1
Blocks: metrov1it6
No longer blocks: metrov1defect&change
Point Estimate = 1.
Blocks: metrov1defect&change
No longer blocks: metrov1it6
Whiteboard: feature=defect c=firefox_app_bar_and_autocomplete u=metro_firefox_user p=1 → feature=defect c=firefox_app_bar_and_autocomplete u=metro_firefox_user p=0
This may turn out to be a trivial fix. Or it may not. Either way, if we can live with it for a while longer I would like to propose punting on this until our new tilegroup implemetation lands (#831915) which should fix it by design - in that we'll use CSS columns for a much simpler grid layout solution
(In reply to Sam Foster [:sfoster] from comment #8)
> This may turn out to be a trivial fix. Or it may not. Either way, if we can
> live with it for a while longer I would like to propose punting on this
> until our new tilegroup implemetation lands (#831915) which should fix it by
> design - in that we'll use CSS columns for a much simpler grid layout
> solution

Totally cool with me. Thanks for the larger context, Sam.
Depends on: 831915
Blocks: metrov1it7
No longer blocks: metrov1defect&change
Whiteboard: feature=defect c=firefox_app_bar_and_autocomplete u=metro_firefox_user p=0 → feature=defect c=firefox_app_bar_and_autocomplete u=metro_firefox_user p=1
Priority: -- → P1
Blocks: metrov1defect&change
No longer blocks: metrov1it7
Whiteboard: feature=defect c=firefox_app_bar_and_autocomplete u=metro_firefox_user p=1 → feature=defect c=firefox_app_bar_and_autocomplete u=metro_firefox_user p=0
Blocks: 831915
No longer depends on: 831915
No longer blocks: metrov1defect&change, 831903
Summary: Defect - "Internet Searches" being displayed horizontally instead of vertically → Work - "Internet Searches" results appearing horizontally instead of vertically
Whiteboard: feature=defect c=firefox_app_bar_and_autocomplete u=metro_firefox_user p=0 → feature=work
Whiteboard: feature=work → feature=work [will be fixed by new grid]
WIP, there's lots of calls to arrange the results and searches richgrids before that popup is open and layour is possible. As the grid (especially the new columns-based grid) is sensitive to its container's height its important we defer arrangeItems calls until that measurement gives the right value.
Assignee: nobody → sfoster
Attachment #758858 - Flags: feedback?(mbrubeck)
Comment on attachment 758858 [details] [diff] [review]
Defer richgrid arrangeItems until the popup is open

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

Overall I don't like that we have to do this, but we gotta do what we gotta do.  If we can come up with any coherent questions/suggestions about platform features that would eliminate the need for this JS/XBL code, let's start a thread on dev.platform.

::: browser/metro/base/content/bindings/autocomplete.xml
@@ +275,5 @@
> +      </field>
> +      <field name="_pendingGridReflows">
> +        <![CDATA[
> +          let reflows = {};
> +          reflows;

I think you can write this as <field name="_pendingGridReflows">{}</field> if you prefer.

I had an idea for possibly simplifying this, though I haven't thought it all the way through... Maybe instead of keeping a table here, we could set a boolean "dirty" flag on each grid that needs a reflow, and openAutoCompletePopup and updateResults could call a grid.arrangeItemsIfDirty() method (but with a better name, hopefully).

@@ +291,5 @@
> +            // TODO: debounce to avoid redundant reflows of the same grid
> +            if (this._computedStyle.visibility !== "collapse") {
> +              aGrid.arrangeItems();
> +              this._pendingGridReflows[gridId] = null;
> +              delete this._pendingGridReflows[gridId];

I'm curious - why null the property before deleting it?

@@ +293,5 @@
> +              aGrid.arrangeItems();
> +              this._pendingGridReflows[gridId] = null;
> +              delete this._pendingGridReflows[gridId];
> +            } else {
> +              // queup and schedule for later when we are showing

typo (queup)
Attachment #758858 - Flags: feedback?(mbrubeck) → feedback+
I believe this to be a dupe of bug 881868, which has a (considerably simpler) patch in for review and appears to fix both snapped/full view search grids. Can you confirm bbondy?
Flags: needinfo?(netzen)
Yup looks like they're the same, dupe away.
Flags: needinfo?(netzen)
Summary: Work - "Internet Searches" results appearing horizontally instead of vertically → Work - "Internet Searches" results grid doesnt layout correctly (appears horizontally/vertically in regular/snapped view)
Duplicate of this bug: 881868
This patch refactors richgrid's arrangeItems quite a bit to help isolate what's going on, when. It adds support for a deferlayout attribute which can be set when we know arrangeItems would be fruitless, and arrangeItemsNow method to clear it and re-attempt layout (arrangeItems). 

The autocomplete binding makes use of this deferlayout attribute to not attempt grid layout until the poupup is open and grid is populated. In my testing this seems to do the trick.
Attachment #758858 - Attachment is obsolete: true
Attachment #763737 - Flags: review?(jmathies)
Comment on attachment 763737 [details] [diff] [review]
autocomplete's grids now deferlayout

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

In a local test, this worked for me. Any chance this will break grid tests? You might want to push it to try.

::: browser/metro/base/content/bindings/grid.xml
@@ +400,5 @@
> +        <parameter name="aTime"/>
> +        <body>
> +          <![CDATA[
> +              // cap the number of times we reschedule calling arrangeItems
> +              let maxRetries = 5;

This should probably be saved in a const w/a comment someplace prominent, although I'm not sure where you put it in this case.

@@ +462,5 @@
> +              clearTimeout(this._scheduledArrangeItemsTimerId);
> +              delete this._scheduledArrangeItemsTimerId;
> +            }
> +            this._scheduledArrangeItemsTries = 0;
> +            // pass over any params 

nit - whitespace
Attachment #763737 - Flags: review?(jmathies) → review+
Tests came up clean and green, I made maxRetries a _maxArrangeItemsRetries field. Pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/c952e7bb36f0
https://hg.mozilla.org/mozilla-central/rev/c952e7bb36f0
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.