Closed
Bug 858388
Opened 11 years ago
Closed 11 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)
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)
704.49 KB,
image/png
|
Details | |
9.27 KB,
patch
|
jimm
:
review+
|
Details | Diff | Splinter Review |
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)
Assignee | ||
Comment 2•11 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.
Comment 3•11 years ago
|
||
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.
Comment 4•11 years ago
|
||
For consideration in Iteration #6.
Flags: needinfo?(mmucci)
Whiteboard: feature=defect c=firefox_app_bar_and_autocomplete u=metro_firefox_user p=0
Updated•11 years ago
|
Updated•11 years ago
|
Comment 5•11 years ago
|
||
p=2
Comment 6•11 years ago
|
||
actually let's call it p=1
Updated•11 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•11 years ago
|
Comment 7•11 years ago
|
||
Point Estimate = 1.
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•11 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•11 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.
Updated•11 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•11 years ago
|
Priority: -- → P1
Updated•11 years ago
|
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•11 years ago
|
Updated•11 years ago
|
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
Updated•11 years ago
|
Whiteboard: feature=work → feature=work [will be fixed by new grid]
Assignee | ||
Comment 10•11 years ago
|
||
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 11•11 years ago
|
||
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•11 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)
Assignee | ||
Updated•11 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 | ||
Comment 15•11 years ago
|
||
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•11 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•11 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
Comment 18•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c952e7bb36f0
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Updated•10 years ago
|
OS: Windows 8 Metro → Windows 8.1
You need to log in
before you can comment on or make changes to this bug.
Description
•