Work - Use the Firefox Start Top Sites tile group

RESOLVED FIXED in Firefox 22

Status

defect
P2
normal
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: jimm, Assigned: ally)

Tracking

unspecified
Firefox 22
All
Windows 8.1
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [part-1-completed] [part-2-completed] feature=work , )

Attachments

(5 attachments, 8 obsolete attachments)

5.01 KB, patch
mbrubeck
: review+
Details | Diff | Splinter Review
5.50 KB, text/html
Details
508.87 KB, image/png
Details
15.87 KB, patch
mbrubeck
: review+
Details | Diff | Splinter Review
15.91 KB, patch
Details | Diff | Splinter Review
Reporter

Description

7 years ago
No description provided.
Assignee: nobody → mbrubeck
Hardware: x86_64 → All
Whiteboard: [metro-beta]
Assignee

Comment 1

7 years ago
yoink! 

mbrubeck, do you have mocks, guidelines, plans, or other intel I should know about before giving this a whirl? Does ux have any strong feelings on this topic?
Assignee: mbrubeck → ally
Bug 747789 has some mockups.  Yuan or shorlander may have more detailed specs, or can provide them as needed.
Keywords: uiwanted
Assignee

Updated

7 years ago
Flags: needinfo?(ywang)
Reporter

Updated

7 years ago
Component: General → Theme
Product: Firefox → Firefox for Metro
[Basic Use Cases] - P1 must have for beta
- View top 8 sites in thumbnail (3(row) by 3(column))
- Rearrange the top site thumbnail
- Tap/Click to launch the site
- Get the most updated top site thumbnails based on users' data.
- If there are fewer than 8 top sites, display blank thumbnails. 


[First run users] - P1 must have
- For first run users, probably will have Mozilla page, SUMO page, Metro public announcement page, etc as the top site thumbnails.

[Layout adjustment] - P2 need to have for beta
- When tab panel is shown, top site thumbnails in 3 by 3 should smoothly move to 4 by 2.
- When users are in portrait orientation, thumbnails should be in 2 by 4. Tab panel doesn't affect the layout of top sites.
- When users are in filled view(2/3 screen), the adjustment should be the same with full screen view. 
(But it's worthy looking into whether it's necessary to slightly reduce the size of thumbnails and tiles.)
- When users are in snapped view(1/3 screen),top site thumbnails should changes to small tiles.
Mockup here: http://cl.ly/image/3l2J2D0g3b1Y/o

[Advanced Use Cases] - P3 nice to have
- Slide to select single thumbnail (typical Windows8 gesture for selection) or right-click on single thumbnail should bring up contextual app bar. Repeat the same gesture or right-click again should exit the app bar.
   App bar for individual top site selection should contain 3 commands: "Open this link","Remove","Pin/Unpin on the current position".
- Slide to select multiple thumbnails, will bring up the app bar as well. Reversible interactions should be applied
   App bar for multiple tops site selection should contain commands: "Open these links", "Remove all", "Clear selection"
- If user removes top sites, the sites ranked behind the removed site should move forward, and load new top sites from the end.(the same interactions with new tab, Firefox desktop).

Hope this is clear. Happy to clarify any question you may have.
Flags: needinfo?(ywang)
[Basic Use Cases] - P1 must have for beta
- View top 8 sites in thumbnail (3(row) by 3(column))
- Tap/Click to launch the site
- Get the most updated top site thumbnails based on users' data.
- If there are fewer than 8 top sites, display blank thumbnails. 

I took out "- Rearrange the top site thumbnail". Thumbnails should be displayed based on frequency. Users shouldn't have the ability to rearrange them.
Whiteboard: [metro-beta] → [metro-mvp]
Assignee

Comment 5

7 years ago
So if I understand you correctly, translating the use cases into behavioral requirements yields:

start up:
population of grid
  top sites grid will always appear
  determine top 8 site by frecency
  populate screenshot/thumbnails into topsites grid
  if < 8, fill with blank
  users should not be able to rearrange this grid 
layout events: 
if portrait
  3x3 grid
  if tab panel is triggered
    4x2 grid
if landscape
  3x3 grid
  if tab panel is triggered
    2x4 triggered
if snapped view
  replace thumbnails with small tiles
  layout remains the same

events:
tap
  opens site in new tab
click
  opens site in new tab


Questions:
1) if the grid is 3x3 (a total of 9), why only 8 sites? why not 9?
2) what do you mean by tab panel? What event triggers it? Does a user touch something? You appear to be inconsistent about what yo want when the orientation is portrait (before & after said tab panel is shown)
3) "When users are in filled view(2/3 screen), the adjustment should be the same with full screen view." What exactly do you mean by adjustment here? Adjustment of what?
4) should snapped view change layout/behavior if landscape or portrait?
5) do we populate 'topsites' for people on first run? Your first comment suggests that you want it loaded with mozilla pages. I don't think that is consistent with either desktop or mobile?


6) I think a number of the advanced use cases/ nice to haves should be filed as separate enhancement bugs. This one should represent mvp only.

Comment 6

7 years ago
The requirements above look right to me. 

To answer your questions:
1) if the grid is 3x3 (a total of 9), why only 8 sites? why not 9?
I think that is a decision made by Shorlander for aesthetic reason. Since the layout needs to be adaptive, 8 thumbnails are easier to break down into 4x2 or 2x4 grid. If 9 sites, there will be one tile being left out at 4x2 or 2x4 grid. 
 
2) what do you mean by tab panel? What event triggers it? Does a user touch something? You appear to be inconsistent about what yo want when the orientation is portrait (before & after said tab panel is shown)
   Tab panel is area that contains tabs. 
   Single swipe from top or bottom edge or right-click by mouse will trigger URL bar and app bar. If you swipe one more time or right-click one more time, tab panel will open. The same time, app bar slides back. (Edge swipe and right-click are the standard Win8 interactions to bring up controls)

3) "When users are in filled view(2/3 screen), the adjustment should be the same with full screen view." What exactly do you mean by adjustment here? Adjustment of what?
That means in filled view,
 - When tab panel is shown, top site thumbnails in 3 by 3 should smoothly move to 4 by 2.
- When users are in portrait orientation, thumbnails should be in 2 by 4. Tab panel doesn't affect the layout of top sites.
These adjustments on layout is the same with adjustments on full screen view. 
 

4) should snapped view change layout/behavior if landscape or portrait?
Thanks for asking this. Snap view or filled view doesn't apply to portrait orientation. 
- If users rotate snap&filled view screen from landscape to portrait, the app that was in filled view(2/3 screen) will take the whole screen and display itself in full-screen portrait. Rotating the device back to landscape doesn't bring back the app that was in snap view.

5) do we populate 'topsites' for people on first run? Your first comment suggests that you want it loaded with mozilla pages. I don't think that is consistent with either desktop or mobile?
- I will check in with designers from other platforms to keep consistent. Will keep u posted. 

6) I think a number of the advanced use cases/ nice to haves should be filed as separate enhancement bugs. This one should represent mvp only. 
That sounds good to me.
Whiteboard: [metro-mvp] → [metro-mvp][LOE:2]
Assignee

Comment 7

7 years ago
as from today's ux-metro meeting, examine pinning for topsites. May require a separate bug.
Depends on: 808770
Assignee

Comment 8

7 years ago
hey yuan, did you ever have a chance to check in with the other designers about populating topsites on first run?
Assignee

Updated

7 years ago
Blocks: 801000
Assignee

Updated

7 years ago
Blocks: 808770
No longer depends on: 808770
Assignee

Comment 9

7 years ago
After discussing/demoing the current wip with yuan, I am re-organizing this a bit.

1) This bug is going to include: topsites not appearing on startup (there is no history and none of the other fx prepopulate.) Once the user has a populated profile, Topsites as thumbnails will appear in an 3x3 grid. 

2) Pinning will be a followup feature (bug 808770)

3) The filled view is not going to change from normal view

4) The snapped view will be a totally different awesomescreen(bug 801000) , and it may use favicons for topsites instead of thumnails to conserve space. Handling of topsites in the snapped view will be part of that bug
Assignee

Updated

7 years ago
Depends on: 811548
Assignee

Comment 10

7 years ago
exposes topsites (up to 9) on startui after the first run (hides based on "browser.firstrun.show.localepicker" pref, based on what I saw in b2g & android code. If I've got the wrong pref, let me know).

- Since the thumbnail code is dependent on a port into toolkit(which may be time consuming), this uses favicons. (So we can get a functioning, if not super polished, form of the feature out)

- Tim, is this worth writing a test for? Do we have a functioning framework to test that? I am not sure
Attachment #681685 - Flags: review?(mbrubeck)
Attachment #681685 - Flags: feedback?(tabraldes)
Assignee

Updated

7 years ago
Whiteboard: [metro-mvp][LOE:2] → [metro-mvp][LOE:3][metro-it1]
Assignee

Updated

7 years ago
Blocks: 812291
Assignee

Updated

7 years ago
Keywords: uiwanted
Comment on attachment 681685 [details] [diff] [review]
wip topsits patch, part 1/2

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

::: browser/metro/base/content/bindings/grid.xml
@@ +216,5 @@
>        <property name="columnCount" readonly="true" onget="return this._columnCount;"/>
>  
>        <method name="arrangeItems">
> +        <parameter name="numRows"/>
> +        <parameter name="numColumns"/>

Nit: Please use "aNumRows" and "aNumColumns".

@@ +245,5 @@
>   
> +	    if (numRows) {
> +	      this._rowCount = numRows;
> +            }
> +            else {

Our totally undocumented Fennec style guide says to put "} else {" on one line.  (We can change this if there is popular demand, but we will have to tie up mfinkle in the basement first.)

::: browser/metro/base/content/history.js
@@ +3,4 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +'use strict';

\o/
Attachment #681685 - Flags: review?(mbrubeck) → review+
Assignee

Updated

7 years ago
Whiteboard: [metro-mvp][LOE:3][metro-it1] → [metro-mvp][LOE:3][metro-it1][part-1-completed]
Comment on attachment 681685 [details] [diff] [review]
wip topsits patch, part 1/2

(In reply to :Ally Naaktgeboren from comment #10)
> [...]
>
> - Tim, is this worth writing a test for? Do we have a functioning framework
> to test that? I am not sure

This functionality seems like it will be manually tested every time someone opens the modern browser, so it's probably not a big deal to add a test for it.

On the other hand, it's always nice to have at least one test already written so that down the road when bugs crop up we can just edit a js file to add more tests.  If you're interested in adding a test, check out the files in browser/metro/base/tests - those are all the browser chrome tests we have for metro.  To run the tests, "pymake -C objdir mochitest-metro-chrome" then look in objdir for a log file with the results.  I've got a patch on the back burner (on hold while I work on some widget events) that might add some helper functions for use by our mochitests, but you probably don't need them.
Attachment #681685 - Flags: feedback?(tabraldes)
Depends on: 818671
Assignee

Updated

7 years ago
Whiteboard: [metro-mvp][LOE:3][metro-it1][part-1-completed] → [metro-mvp][LOE:3][metro-it1][part-1-completed][metro-it2]

Updated

7 years ago
Summary: Implement top sites thumbnail view for StartUI → Story – Use the Awesome screen Top Sites tile group
Whiteboard: [metro-mvp][LOE:3][metro-it1][part-1-completed][metro-it2] → [metro-mvp][LOE:3][metro-it1][part-1-completed][metro-it2] feature=story

Updated

7 years ago
Priority: -- → P2

Updated

7 years ago
No longer depends on: 818671
No longer blocks: 812291

Updated

6 years ago
Summary: Story – Use the Awesome screen Top Sites tile group → Story – Use the Firefox Start Top Sites tile group
Assignee

Updated

6 years ago
No longer blocks: 801000
Depends on: 801000
Keywords: feature
Whiteboard: [metro-mvp][LOE:3][metro-it1][part-1-completed][metro-it2] feature=story → [part-1-completed] feature=story
Assignee

Comment 15

6 years ago
-icon disappears, name appears in light txt, and the watermark appears as the background image (its faint, but there).

Updated

6 years ago
Summary: Story – Use the Firefox Start Top Sites tile group → Story - Use the Firefox Start Top Sites tile group
Assignee

Comment 16

6 years ago
So, this *sort* of works, but in a buggy way that suggests that I may be going about this the wrong way or missing a case. I'm hoping for some feedback from the other front-end folks.

functionally:
- Sometimes loaduri() does not fire when I load a page in the browser. Is this expected? (seems to cause some pages not to get a screenshot atm)
- For others, there is a screenshot in residence, but the link returned from PageThumbs is incorrect/DNE (this suggests that I may be consuming it incorrectly)

eyesore:
- captured pagethumbs are probably too large. need to be shrunk
- tiles are tiny now that the icon box is collapased

flatout not covered in user story, but should before we ship:
- when to skip taking screenshots (see shouldcapture() in browser-thumbnails)
Attachment #716287 - Attachment is obsolete: true
(In reply to :Ally Naaktgeboren from comment #16)
> - Sometimes loaduri() does not fire when I load a page in the browser. Is
> this expected? (seems to cause some pages not to get a screenshot atm)

There are a variety of ways to load a page in the browser, only some of which call Browser.loadURI.  For example, clicking a link or using back/foward navigation does not go through our frontend code.

To catch all of these, you can use messageManager.addMessageListener to listen for a "Content:LocationChange" message from the browser (which fires when a new URL is about to load) or a "pageshow" message (which fires when the page has actually loaded -- this is probably what you want).  

I haven't looked into the other issues from this comment yet.
No longer blocks: 808770
Assignee

Comment 18

6 years ago
- looking functionally correct now. (thanks mbrubeck)

- wrong size (tiles are too small & images are too large)
- missing shouldcapture logic (sometimes, we dont actually want to take a screenshot)
- does not update dynamically (more interesting given that we need a shut down to get images to show up. it seems. This is ok on desktop, this might not be ok in the metro world)
Attachment #718215 - Attachment is obsolete: true
Assignee

Comment 19

6 years ago
Posted patch wip core -functional (obsolete) — Splinter Review
- has hardcoded tile dimensions (don't like that)
- has hardcoded color for tile (dont like that in all cases either)
-- not covered in user story, but with skipped capture this can be ugly
- does not pick up new thumbs until restart (really dont like that)
Attachment #718558 - Attachment is obsolete: true
Assignee

Updated

6 years ago
Attachment #718712 - Flags: feedback?(fyan)
Component: Theme → Metro Operations
Product: Firefox for Metro → Tracking
Version: Trunk → ---
Assignee

Updated

6 years ago
Component: Metro Operations → General
Product: Tracking → Firefox for Metro
Summary: Story - Use the Firefox Start Top Sites tile group → Work - Use the Firefox Start Top Sites tile group
Version: --- → unspecified
Assignee

Updated

6 years ago
Whiteboard: [part-1-completed] feature=story → [part-1-completed] [part-2-inprogress]
No longer blocks: metrov1onhold
Assignee

Updated

6 years ago
Blocks: 845911
Reporter

Updated

6 years ago
Component: General → Firefox Start
No longer blocks: 747789
Blocks: 831923
No longer blocks: 845911
Assignee

Comment 20

6 years ago
ok, so it all works, but has a horrible hack that seems really bad for our performance. I feel I need less fugly css if possible, and updateTileHack() should only update the changed tile

I think we should also get this reviewed by someone who knows the desktop thumbnail. I cant say whether or not we need the expiration filter logic with certainty or whether that could wait for v2
Attachment #718712 - Attachment is obsolete: true
Attachment #718712 - Flags: feedback?(fyan)
Attachment #719232 - Flags: feedback?(mbrubeck)
Attachment #719232 - Flags: feedback?(fyan)
Comment on attachment 719232 [details] [diff] [review]
fully functional, with updates, but horrible, horrible hack (and probably a perf hit)

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

Looks good!  Various suggestions/requests below.

::: browser/metro/base/content/TopSites.js
@@ +170,5 @@
> +    for(let i=0; i<9;++i) {
> +      let item = this._set.getItemAtIndex(i);
> +      let bkgrd = item.backgroundImage;
> +      Cu.reportError("hackloop:"+i+" bkgrd = "+ bkgrd + " getcomputedsyle = " +window.getComputedStyle(item._box, "").backgroundImage);
> +      item._box.style.removeProperty("background-image");

Rather than do this for all 9 tiles every time, let's pass in the URI of the page we just captured, and update only when we find a tile for that specific page.

To avoid the implementation leakage mentioned in your comment, we could move the code from this loop into a "refreshBackground" method in the richgriditem binding.

::: browser/metro/base/content/bindings/grid.xml
@@ +536,5 @@
> +            if (this.backgroundImage != undefined) {
> +              this._box.parentNode.setAttribute("customImagePresent", "true");
> +              this._box.style.backgroundImage = this.backgroundImage;
> +            } else {
> +              this._box.parentNode.removeAttribute("customImagePresent");

Perhaps we should also unset this._box.style.backgroundImage.

::: browser/metro/base/content/browser-ui.js
@@ +787,5 @@
> +      case "pageshow":
> +        // anaaktge
> +        let currBrowser = Browser.selectedBrowser;
> +        if( this._shouldCapture(currBrowser)) {
> +          PageThumbs.captureAndStore(currBrowser);

I know I'm the one who suggested "pageshow", but now that I look at the desktop implementation, we might want to duplicate more of their logic.  Desktop's browser-thumbnails.js listens for a "network stop" notification and then captures the screenshot after a 1-second delay.

In the Metro world, you can do this by listening for the "Content:StateChange" message; see /browser/metro/base/content/WebProgress.js for sample code.

@@ +792,5 @@
> +          Cu.reportError("captured. should be image in directory");
> +        }
> +        // for the love of all that is good, don't let this ship as-is
> +        let topsitesobj = window["TopSitesStartView"];
> +        topsitesobj._view.updateTilesHack();

We can replace the direct _view call with a Server.obs notification to help make this less hacky.

@@ +801,5 @@
>    },
>  
> +  // Private Browsing is not supported on metro at this time, when it is added
> +  //  this function must be updated to skip capturing those pages
> +  _shouldCapture: function _shouldCapture(aBrowser) {

Naming nit: Can we call this _shouldCaptureThumbnail?  Alternately we could move both it and the listener out of BrowserUI and into a new object ("BrowserThumbnails"?).

@@ +807,5 @@
> +    if (aBrowser != Browser.selectedBrowser) {
> +
> +      Cu.reportError("not capturing - not selected tab");
> +      return false;
> +    }

I see this logic came from desktop Firefox but I'm curious *why* they don't capture background tabs...

@@ +870,5 @@
> +        return false;
> +      }
> +
> +      // Don't capture HTTPS pages unless the user explicitly enabled it.
> +      if (uri.schemeIs("https") && !this._sslDiskCacheEnabled) {

Looks like we'll need to define _sslDiskCacheEnabled somewhere.

::: browser/metro/theme/platform.css
@@ +344,2 @@
>    background: #fff;
>  }

I think just ".richgrid-item-content { background: #fff; }" should be enough here, since we override the background style when we set any custom image or color.

@@ +364,1 @@
>    color: #f1f1f1;

While we're modifying this, could you change it to avoid the descendant selector (for performance reasons)?  I think you can just remove " description" from the selectors, and set "color" directly on the richgriditem instead; the description will inherit it.

@@ +370,5 @@
>  }
>  
> +richgriditem[customImagePresent="true"] .richgrid-item-content {
> +  height: 120px; /*anaaktge - there should be something better than hardcode*/
> +  width: 200px;

Here we can change the descendant selector to a child selector:

  richgriditem[customImagePresent] > .richgrid-item-content

Having a default height/width hardcoded here is fine with me; it won't prevent us from changing these styles elsewhere if we need to.

@@ +380,5 @@
> +
> +/* hide icon if there is an image background */
> +richgriditem[customImagePresent="true"] .richgrid-icon-container {
> +  visibility: collapse;
> +}

We could also avoid this descendant selector by using inherits="customImagePresent" on the .richgrid-icon-container element in the binding.  However, there are much worse selector problems elsewhere in these richgrid styles; we could file a separate bug to deal with them all in one swoop if you prefer.
Attachment #719232 - Flags: feedback?(mbrubeck) → feedback+
Assignee

Updated

6 years ago
Attachment #719232 - Flags: feedback?(fyan)

Updated

6 years ago
Whiteboard: [part-1-completed] [part-2-inprogress] → [part-1-completed] [part-2-inprogress] feature=work
Assignee

Comment 22

6 years ago
Posted patch draft 0 part 2/2 (obsolete) — Splinter Review
I tried to leave comments in the code in places where the behavior is odd, ie we are missing a pref for the user override for ssl (and I'm going to say that until Asa has a pressing user case for it, I'm not too worried about it), and whether the layout engine does not respond as expected to refreshing the image in xbl.

Afaik, the reason desktop only does the current tab is they no longer load the background tab the user foregrounds it (probably because of users like me, who have >100 tabs between their devices :) ). I'm inclined to follow their lead on that for now unless there is a compelling reason not to?
Attachment #719232 - Attachment is obsolete: true
Attachment #720915 - Flags: review?(mbrubeck)
Comment on attachment 720915 [details] [diff] [review]
draft 0 part 2/2

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

Great!  Aside from a few style nits, I'd like to take one more crack at forceReloadOfThumbnail before landing, and also get some UX feedback on a text color problem.  Details below.

::: browser/metro/base/content/TopSites.js
@@ +31,5 @@
>      return true; // operation was successful
>    }
>  };
>  
> +function TopSitesView(aGrid, maxSites, useThumbnails) {

Style nit: aMaxSites, aUseThumbnails

(I know the former is already there -- I must have missed it before, sorry.)

@@ +43,5 @@
>  
>    let history = Cc["@mozilla.org/browser/nav-history-service;1"].
>                  getService(Ci.nsINavHistoryService);
>    history.addObserver(this, false);
> +  Services.obs.addObserver(this, "Metro:RefreshTopsiteThumbnail", false);

Please put "if (this._useThumbs)" around this (and the matching removeObserver).

@@ +161,5 @@
>    },
>  
> +  forceReloadOfThumbnail: function forceReloadOfThumbnail(url) {
> +      let nodes = this._set.querySelectorAll('richgriditem[value="'+url+'"]');
> +      let item = nodes[0];

Rather than take the first match, let's do "for (let item of nodes)", in case there are zero matches, or multiple matches.

(Or if we do know there will be exactly one match, we can use querySelector() here.)

@@ +167,5 @@
> +      // _box is leaking richgrid implmentation, which is bad. However,
> +      //  for unknown reason(s), this code in xbl does not induce the
> +      //  layout engine to reload the page with the new and/or now valid image
> +      item._box.style.removeProperty("background-image");
> +      item._box.style.setProperty("background-image", bkgrd);

Moving this to a richgrid-item method like this seemed to work for me, though I haven't tested it extensively:

      <method name="refreshBackgroundImage">
        <body><![CDATA[
          if (this.backgroundImage) {
            this._box.style.removeProperty("background-image");
            this._box.style.setProperty("background-image", this.backgroundImage);
          }
        ]]></body>
      </method>

If there's some reason we can't get this to work, putting the code here is okay -- but let's see if we can work through the issues first.

::: browser/metro/base/content/browser-ui.js
@@ +786,5 @@
>          this.goToURI(json.uri);
>          break;
> +      case "Content:StateChange":
> +        let currBrowser = Browser.selectedBrowser;
> +        if( this.shouldCaptureThumbnails(currBrowser)) {

whitespace nit: misplaced space after "if"

@@ +789,5 @@
> +        let currBrowser = Browser.selectedBrowser;
> +        if( this.shouldCaptureThumbnails(currBrowser)) {
> +          PageThumbs.captureAndStore(currBrowser);
> +          let currPage = currBrowser.currentURI.spec;
> +          Services.obs.notifyObservers(null, "Metro:RefreshTopsiteThumbnail", currPage);

After the StateChange notification, desktop Firefox waits for 1 second before capturing the thumbnail.  I haven't looked into exactly why they do this, but I'm guessing it's to avoid / spread out any jank when the page is just loading.  Can we do the same here?

::: browser/metro/theme/platform.css
@@ +365,2 @@
>    color: #f1f1f1;
>  }

This color is unreadable on tiles where we show the default background because we can't actually load a thumbnail (because it's not captured yet, or because SSL or cache-control prevents it), and also on thumbnails of sites with light backgrounds.  This is bad enough that I think it will interfere with dogfooding and testing.

Stephen, do we have a design spec that addresses this already?  If not, can we give this text a background or a text-shadow or something, at least until until bug 826556 is fixed?

Ally, if we don't have a final design before this lands, let's deal with it in a follow-up bug and just land with a temporary solution, like:

.richgrid-item-content[customImagePresent] > .richgrid-item-desc {
  background: rgba(0, 0, 0, 0.4);
}

@@ +369,5 @@
>    opacity: 0.8;
>    background-color: #fff;
>  }
>  
> +.richgrid-item-content[customImagePresent="true"] {

Minor style suggestion:  I prefer [attribute] instead of [attribute="true"] when the attribute has only one possible value -- mostly just to make the selectors more concise.

Since the existing code here uses [attribute="true"], you can either stick with that style, or update both the old and new code to use [attribute] if you feel like it.
Attachment #720915 - Flags: review?(mbrubeck) → review+
Stephen, can we get a design decision for how to display text so it will be readable on thumbnails with both light and dark backgrounds?  (See comment 23 for more details.)
Flags: needinfo?(shorlander)
Assignee

Comment 25

6 years ago
Posted image screenshot
the white text currently used for bookmarks (custom color tiles) and topsites. sometimes this is ok (see pandora tile) other times not (4 of the tiles are https/loading/ etc and are not captured, so remain white squares) (others are predominantly white pages, see corporette)
(In reply to Matt Brubeck (:mbrubeck) from comment #23)
> Stephen, do we have a design spec that addresses this already?  If not, can
> we give this text a background or a text-shadow or something, at least until
> until bug 826556 is fixed?

Sorry, I guess the original mockup never got linked here: https://bug747789.bugzilla.mozilla.org/attachment.cgi?id=617342

The title should be in a bar at the bottom. Originally the spec called for pulling the thumbnail dominant color, not sure if we can do that yet? Also it will probably have to wait on bug 826556.

In the meantime we can just do something like:

.tileTitleOrSomeClass {
  background-color: hsl(0,2%,98%);
  color: #1a1a1a
}
Flags: needinfo?(shorlander)
Oops meant:

.tileTitleOrSomeClass {
  background-color: hsla(0,2%,98%,.95);
  color: #1a1a1a
}
Assignee

Updated

6 years ago
Blocks: 848134
Assignee

Comment 28

6 years ago
1)Desktop waiting one second to snapshot - according to the owner, this is just a not-very-smart heuristic to try and not distrub the page while loading. He isn't thrilled with it, so I'm inclined to not copy this code until there is a compelling reason (like a perf hit).

2)Expiration Filters - I had noticed that, but wasn't sure if they were needed (its not covered by my user story). After talking with Tim it sounds like we do.

3)Text coloring - Yea, this wasn't really a case covered in the original spec I had (which seems to assume that your topsites are safe to snapshot all the time and did not have the bars at bottom). I'll do the easy change for now, and it looks like followup is going to be needed in general.

4)Css style nit - I am conflicted because I agree with you, but it has been beaten into my head that consistency of the file trumps all and I am concerned about scope bloat. So, I'll compromise: we now have bug 848134 and it already has a big patch on it. Seems we had this issue in a bunch of places. Everyone wins?
Assignee

Updated

6 years ago
Blocks: 848155
Assignee

Comment 29

6 years ago
I know a prior version has an r+, but the desired behavior has been modified and the functionality is sufficiently different that I'd like a quick once-over on the new version.
Attachment #720915 - Attachment is obsolete: true
Attachment #721488 - Attachment is obsolete: true
Attachment #721898 - Flags: review?(mbrubeck)
Comment on attachment 721898 [details] [diff] [review]
draft 2, part 2/2 post spec change

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

Looks good except for the missing expiration filter code (see below).

::: browser/metro/base/content/TopSites.js
@@ +46,5 @@
>    let history = Cc["@mozilla.org/browser/nav-history-service;1"].
>                  getService(Ci.nsINavHistoryService);
>    history.addObserver(this, false);
> +  if (this._useThumbs) {
> +    PageThumbs.addExpirationFilter(this);

I don't see a filterForThumbnailExpiration method in TopSitesView.prototype.  Is part of the patch missing?

::: browser/metro/theme/platform.css
@@ +391,1 @@
>  richgriditem description {

This can change to .richgrid-item-desc.

@@ +397,5 @@
>  
> +.richgrid-item-content[customImagePresent] > .richgrid-item-desc {
> +  background: hsla(0,2%,98%,.95);
> +  margin-bottom: 0px;
> +  margin-right: 0px;

Minor style nit: Is it possible to just use "margin: 0;" here?
Attachment #721898 - Flags: review?(mbrubeck) → feedback+
Assignee

Comment 31

6 years ago
Sorry about the missing bit. Must train myself to qref more aggressively :)

I'm not sure solid on the ExpirationFilter function. I tried to test it as best I could.
Attachment #721898 - Attachment is obsolete: true
Attachment #722495 - Flags: review?(mbrubeck)
Comment on attachment 722495 [details] [diff] [review]
draft 3, part 2/2

>+  filterForThumbnailExpiration: function filterForThumbnailExpiration(aCallback) {
>+    aCallback([browser.currentURI.spec for (browser of Browser.browsers)]);
>+  },

I think we want to pass back a list of the top sites, rather than a list of current tabs.  That way, items currently in the grid will not expire, even if they are not currently loaded in a tab.  For now we can loop over the current tiles:

  [item.getAttribute("value") for (item of this._set.children)]

And after bug 808770 lands, it looks like NewTabUtils will add its own expiration filter that keeps "top sites" from expiring:
http://hg.mozilla.org/mozilla-central/file/71395a927025/toolkit/modules/NewTabUtils.jsm#l770

...so maybe we don't need to add one here after all (or we can remove it when bug 808770 lands -- but it looks like both of these will land very soon).

So... r=mbrubeck either with the alternate filter above or with no addExpirationFilter at all.
Attachment #722495 - Flags: review?(mbrubeck) → review+
 https://hg.mozilla.org/integration/mozilla-inbound/rev/a1586c3b0da8
Status: NEW → ASSIGNED
Whiteboard: [part-1-completed] [part-2-inprogress] feature=work → [part-1-completed] [part-2-completed] feature=work
Target Milestone: --- → Firefox 22
https://hg.mozilla.org/mozilla-central/rev/a1586c3b0da8
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Reporter

Updated

6 years ago
Attachment #722518 - Attachment is patch: true
Reporter

Updated

6 years ago
Depends on: 849819
QA Contact: jbecerra

Updated

6 years ago
Depends on: 872243
No longer depends on: 872243
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.