Do something better than showing blank squares for the initial about:newtab experience with migrated history

VERIFIED FIXED in Firefox 51

Status

()

Firefox
New Tab Page
VERIFIED FIXED
7 months ago
3 months ago

People

(Reporter: Gijs, Assigned: dao)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 53
Points:
---

Firefox Tracking Flags

(firefox51 verified, firefox52 fixed, firefox53 fixed)

Details

(Whiteboard: pref: browser.newtabpage.thumbnailPlaceholder = true)

Attachments

(1 attachment, 3 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

6 months ago
Bug 1322731 solves this for the "without migration" case, right?
Depends on: 1322731
Flags: needinfo?(gijskruitbosch+bugs)
(Reporter)

Comment 2

6 months ago
(In reply to Dão Gottwald [:dao] from comment #1)
> Bug 1322731 solves this for the "without migration" case, right?

Yes. Note that it might (and I haven't tested this) even affect the "with" migration case where visits are to the same domains as the ones we preload.

It still feels like we need a generic mechanism to create 'styled' tiles, though. I'm not entirely sure how to go about doing this. I think we can basically hyphenate the domain name, and take the starting letters of each syllable, to do the 'right' thing for most domains ("fb" for facebook, "yt" for youtube, "nyt" for new york times, etc.) -- except I am not aware of us having a (chrome-)JS-accessible API to use the hyphenation algorithm/dictionaries. All I know if is the -moz-hyphens CSS property, and I can't think of a way to use that to get the right display (ie first letters horizontally next to each other).

(The random background color is somewhat easier to do, of course...)
Flags: needinfo?(gijskruitbosch+bugs)

Comment 3

6 months ago
Are we still going try using https://github.com/mozilla/tippy-top-sites before going with an abbreviation and color?
(Reporter)

Updated

6 months ago
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Comment 4

6 months ago
I discussed this with dolske last week and there was some confusion about what problem we're trying to solve here, and consequentially what relative importance this should have in the context of the funnelcake. AFAIK the situation is that we /initially/ show blank tiles for top sites without thumbnails (such as imported history) but use BackgroundPageThumbs.jsm to get screenshots, and we do update tiles live as we get those screenshots. Naturally this takes a few seconds depending on the user's connection. It is not the case that tiles would normally stay blank until the user manually loads pages or something. Am I missing something?

If I'm right, I think we have a rather weak case for a hacky short-term solution (e.g. first letter of domain on random background color). I think it might even add confusion as we'd progressively replace those placeholder tiles with real ones. This could hurt the metrics we'd want to test / see improved with bug 1322738 and bug 1322731 as part of this funnelcake.
Flags: needinfo?(mverdi)
Flags: needinfo?(dolske)
(Reporter)

Comment 5

6 months ago
(clearing ni for me pending answers for comment #4)
Flags: needinfo?(gijskruitbosch+bugs)

Comment 6

6 months ago
(In reply to Dão Gottwald [:dao] from comment #4)
> I discussed this with dolske last week and there was some confusion about
> what problem we're trying to solve here, and consequentially what relative
> importance this should have in the context of the funnelcake. AFAIK the
> situation is that we /initially/ show blank tiles for top sites without
> thumbnails (such as imported history) but use BackgroundPageThumbs.jsm to
> get screenshots, and we do update tiles live as we get those screenshots.
> Naturally this takes a few seconds depending on the user's connection. It is
> not the case that tiles would normally stay blank until the user manually
> loads pages or something. Am I missing something?

In practice, these tiles are blank for a significant amount of time. In testing, some users don't see screenshots at all in their first session. This feature, we think, helps us make Firefox instantly usable. 


> 
> If I'm right, I think we have a rather weak case for a hacky short-term
> solution (e.g. first letter of domain on random background color). I think
> it might even add confusion as we'd progressively replace those placeholder
> tiles with real ones. This could hurt the metrics we'd want to test / see
> improved with bug 1322738 and bug 1322731 as part of this funnelcake.

The UX team doesn't think a "filled-in" tile being replaced with a screenshot will be confusing. Further we think this will be a better experience than beginning with blank tiles which have confused people in testing.
Flags: needinfo?(mverdi)
(Assignee)

Comment 7

6 months ago
(In reply to Verdi [:verdi] from comment #6)
> In practice, these tiles are blank for a significant amount of time.

This statement is too broad and/or imprecise. In practice, it depends on the connection like I said. The tiles load about as quick as these sites would load in a tab. With my below-average cable connection in a dense city area it really doesn't take long. In my grandparents' village it admittedly would take forever (but then again their connection is so bad that clicking on the tiles is going to be a frustrating experience too.)

> In testing, some users don't see screenshots at all in their first session.

Do you have data on this? Do we know how common this problem is?
Flags: needinfo?(mverdi)
(Assignee)

Comment 8

6 months ago
I'm working on this given the tight schedule, but I think we're still not quite on the same page about what exactly the problem is, how big it is, and how big the impact of the solution is supposed to be.
Assignee: nobody → dao+bmo
(Assignee)

Comment 9

6 months ago
Created attachment 8824285 [details] [diff] [review]
patch
Attachment #8824285 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Updated

6 months ago
Whiteboard: pref: browser.newtabpage.thumbnailPlaceholder = true
(Assignee)

Updated

6 months ago
No longer depends on: 1322731
Summary: Do something better than showing a load of blank squares for the initial about:newtab experience (with/without migration) → Do something better than showing blank squares for the initial about:newtab experience with migrated history
(quickly, since I'm about to crash)

Ok, from some quick testing we do indeed dynamically update about:newtab tiles (i.e., without reloading the page). I thought Gijs had looked at this as said it required a page reload to show up -- maybe I misunderstood.

But even on my modern hardware and connection, it takes a number of seconds after startup before that process seems to start, and takes about a second per tile to populate. I think it's entirely reasonable to expect that this process will take much longer for people with slow computers / connections. Hence the desire to show "something" while that process is completing. This seems analogous to the shift in thinking around file I/O performance some time ago -- it's not "prove it's slow", it's "prove it's fast."

I don't think we need strong evidence for testing a simple patch like this in a funnelcake. But since we're about to run it, if someone wants to whip up a patch to add a telemetry probe to measure this we could ship it to get some hard data... Although Gijs had problems actually looking at the data from previous funnelcake telemetry, so it's probably not that simple.
Flags: needinfo?(dolske)
(Reporter)

Comment 11

6 months ago
Comment on attachment 8824285 [details] [diff] [review]
patch

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

This looks OK from the code, but it doesn't seem to work for me. I don't see it working at all - IME refreshThumbnail is only called by the time we already have a bgURL.

::: browser/base/content/newtab/sites.js
@@ +249,5 @@
> +    let bgColor = link.bgColor || "";
> +    let textContent = "";
> +    if (!bgURL &&
> +        link.type == "history" &&
> +        Services.prefs.getBoolPref("browser.newtabpage.thumbnailPlaceholder")) {

From reading the code, I can't quickly convince myself that we don't care about any other types of links (I think there's "organic" as well as the "enhanced" and "sponsored" ones, and it's not clear to me what that means), or that history entries are guaranteed to have a baseDomain, which this code seems to assume. Because any JS errors here presumably break about:newtab completely, I would suggest:

if (!bgURL && Services.prefs.getBoolPref(...)) {
  // cribbed from https://dxr.mozilla.org/mozilla-central/source/browser/modules/DirectoryLinksProvider.jsm#985
  let baseDomain = link.baseDomain || NewTabUtils.extractSite(link.url || "");
  if (baseDomain) {
    textContent = baseDomain.substr(0, 1).toUpperCase();
  }
}

which I think should be relatively safe. We could also keep the link.type check - not sure if there's a reason not apply this to other tiles with no bgURL.
Attachment #8824285 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 12

6 months ago
(In reply to :Gijs from comment #11)
> This looks OK from the code, but it doesn't seem to work for me. I don't see
> it working at all - IME refreshThumbnail is only called by the time we
> already have a bgURL.

Hmmm. refreshThumbnail is called from _render which is called in the Site constructor. It doesn't at all depend on the image being available from what I can tell.

> ::: browser/base/content/newtab/sites.js
> @@ +249,5 @@
> > +    let bgColor = link.bgColor || "";
> > +    let textContent = "";
> > +    if (!bgURL &&
> > +        link.type == "history" &&
> > +        Services.prefs.getBoolPref("browser.newtabpage.thumbnailPlaceholder")) {
> 
> From reading the code, I can't quickly convince myself that we don't care
> about any other types of links (I think there's "organic" as well as the
> "enhanced" and "sponsored" ones, and it's not clear to me what that means),
> or that history entries are guaranteed to have a baseDomain, which this code
> seems to assume. Because any JS errors here presumably break about:newtab
> completely, I would suggest:

History type -> baseDomain is already assumed in _render, and I don't think it should be possible for baseDomain to be empty in that case (it would make the tile's label empty too).

I think we should keep the history type check. This is basically what this bug is about, imported history. If we want to extend this to something else then we need better understanding of what exactly it is that we want.
(Assignee)

Comment 13

6 months ago
(In reply to Dão Gottwald [:dao] from comment #12)
> (In reply to :Gijs from comment #11)
> > This looks OK from the code, but it doesn't seem to work for me. I don't see
> > it working at all - IME refreshThumbnail is only called by the time we
> > already have a bgURL.
> 
> Hmmm. refreshThumbnail is called from _render which is called in the Site
> constructor. It doesn't at all depend on the image being available from what
> I can tell.

So the problem is that we always have a moz-page-thumb:// URL that we try to open regardless of whether that thumbnail actually exists.
(Reporter)

Comment 14

6 months ago
(In reply to Dão Gottwald [:dao] from comment #13)
> (In reply to Dão Gottwald [:dao] from comment #12)
> > (In reply to :Gijs from comment #11)
> > > This looks OK from the code, but it doesn't seem to work for me. I don't see
> > > it working at all - IME refreshThumbnail is only called by the time we
> > > already have a bgURL.
> > 
> > Hmmm. refreshThumbnail is called from _render which is called in the Site
> > constructor. It doesn't at all depend on the image being available from what
> > I can tell.
> 
> So the problem is that we always have a moz-page-thumb:// URL that we try to
> open regardless of whether that thumbnail actually exists.

It looks like PageThumbsStorage.fileExistsForURL(url); returns a promise that resolves to a bool indicating whether we have a thumbnail or not, based on https://dxr.mozilla.org/mozilla-release/source/toolkit/components/thumbnails/BackgroundPageThumbs.jsm#97 . Though the comment preceding it saying it "may be incorrect" is ominous...
(Assignee)

Comment 15

6 months ago
Created attachment 8824394 [details] [diff] [review]
patch v2
Attachment #8824285 - Attachment is obsolete: true
Attachment #8824394 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 16

6 months ago
Created attachment 8824396 [details] [diff] [review]
patch v3

I made the placeholder color a function of the base domain rather than random. This should help users recognize tiles in case the thumbnail is missing for a longer time.
Attachment #8824394 - Attachment is obsolete: true
Attachment #8824394 - Flags: review?(gijskruitbosch+bugs)
Attachment #8824396 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 17

6 months ago
Created attachment 8824397 [details] [diff] [review]
patch v3

Oops, that was still the old patch...
Attachment #8824396 - Attachment is obsolete: true
Attachment #8824396 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Updated

6 months ago
Attachment #8824397 - Flags: review?(gijskruitbosch+bugs)
(Reporter)

Comment 18

6 months ago
Comment on attachment 8824397 [details] [diff] [review]
patch v3

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

Nice, thanks!
Attachment #8824397 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 19

6 months ago
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ca23d313302
Implement ability to show basic placeholders for missing about:newtab thumbnails. r=gijs
(Assignee)

Comment 20

6 months ago
Comment on attachment 8824397 [details] [diff] [review]
patch v3

Approval Request Comment
[Feature/Bug causing the regression]: see bug 1322718 comment 1

[User impact if declined]: about:newtab shows blank tiles on first run after importing history from another browser. It's unclear how many users are severely affected by this.

[Is this code covered by automated tests?]: no

[Has the fix been verified in Nightly?]: not yet

[Needs manual test from QE? If yes, steps to reproduce]: will be part of the funnelcake QA process

[List of other uplifts needed for the feature/fix]: /

[Is the change risky?]: hardly

[Why is the change risky/not risky?]: it's pref'd off by default so it will only affect the funnelcake build. When activated, this code is also pretty straightforward with little error potential.

[String changes made/needed]: /
Attachment #8824397 - Flags: approval-mozilla-beta?
Attachment #8824397 - Flags: approval-mozilla-aurora?

Comment 21

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9ca23d313302
Status: NEW → RESOLVED
Last Resolved: 6 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Comment on attachment 8824397 [details] [diff] [review]
patch v3

Another first run change for the onboarding funnelcake. OK to uplift to beta and aurora. Should be in the beta 13 build today.
Attachment #8824397 - Flags: approval-mozilla-beta?
Attachment #8824397 - Flags: approval-mozilla-beta+
Attachment #8824397 - Flags: approval-mozilla-aurora?
Attachment #8824397 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 23

6 months ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/9fb0bcde1a7f566356097330bf5b565b33bc00fc
status-firefox52: --- → fixed
(Assignee)

Comment 24

6 months ago
https://hg.mozilla.org/releases/mozilla-beta/rev/adb88a603efb0efe74e23cd87066317bea2c82f7
status-firefox51: --- → fixed
Flagging this for verification (and making sure Grover is in the loop), bug description in Comment 20.
Flags: qe-verify+
Flags: needinfo?(gwimberly)
Flags: needinfo?(gwimberly)
(Assignee)

Updated

5 months ago
Flags: needinfo?(mverdi)

Updated

5 months ago
status-firefox51: fixed → verified

Comment 26

5 months ago
[testday-20170203]
The bug is verified. Not a bug hence fixed.
OPERATING SYSTEM: windows 10.0
BROWSER:Firefox beta 51.0
(In reply to fahimazulfath.a from comment #26)
> [testday-20170203]
> The bug is verified. Not a bug hence fixed.
> OPERATING SYSTEM: windows 10.0
> BROWSER:Firefox beta 51.0

This bug was set for verification by the Mozilla QA team in comment 25 (that's what the "qe-verify+" flag means). 

Please do not change the status flags until Mozilla QA has had a look.
status-firefox51: verified → fixed
Sorry about that. This was verified quite a while ago. Never changed the status.
Status: RESOLVED → VERIFIED
(In reply to Grover Wimberly IV [:Grover-QA] from comment #28)
> Sorry about that. This was verified quite a while ago. Never changed the
> status.

Grover, could you also update the status flags so we know on which versions this was verified?
Flags: needinfo?(gwimberly)
status-firefox51: fixed → verified
Flags: needinfo?(gwimberly)
(Assignee)

Comment 30

4 months ago
I'm not sure fahimazulfath.a understood what this bug was about and how to properly verify it. It needs migrated history and it needs a hidden prefs set to actually enable this feature. However, I think we can consider this verified since it was tested as part of the funnelcake QA sign-off.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.