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

RESOLVED FIXED

Status

Firefox for Metro
Browser
P1
normal
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: kjozwiak, Assigned: sfoster)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
Created attachment 733725 [details]
results appearing horizontally instead of vertically

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)
(Reporter)

Comment 1

5 years ago
consideration for Iteration #6
Flags: needinfo?(mmucci)
(Assignee)

Comment 2

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

Updated

5 years ago
Blocks: 841997
No longer blocks: 855905

Updated

5 years ago
Blocks: 859003
No longer blocks: 841997
p=2
actually let's call it p=1

Updated

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

Updated

5 years ago
Blocks: 855905
No longer blocks: 859003
Point Estimate = 1.
Blocks: 859003
No longer blocks: 855905
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
(Assignee)

Comment 8

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

Comment 9

5 years ago
(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

Updated

5 years ago
Blocks: 862352
No longer blocks: 859003
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

Updated

5 years ago
Priority: -- → P1

Updated

5 years ago
Blocks: 859003
No longer blocks: 862352
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

Updated

5 years ago
Blocks: 831915
No longer depends on: 831915

Updated

5 years ago
No longer blocks: 859003, 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]
(Assignee)

Comment 10

5 years ago
Created attachment 758858 [details] [diff] [review]
Defer richgrid arrangeItems until the popup is open

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+
(Assignee)

Comment 12

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

Updated

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

Updated

5 years ago
Duplicate of this bug: 881868
(Assignee)

Comment 15

5 years ago
Created attachment 763737 [details] [diff] [review]
autocomplete's grids now deferlayout

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 16

5 years ago
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+
(Assignee)

Comment 17

5 years ago
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
Last Resolved: 5 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.