994915 - Implement edit panel on New Tab tiles for customization

Status: RESOLVED INVALID

(Firefox :: New Tab Page, defect)

Description

[Jennifer Morrow [:Boriss] (UX)]

This is a followup implementation bug to bug 989202 

This edit panel allows users to select or specify a site to add to a tile to New Tab via an Edit Panel.

In its first iteration for this bug, this panel appears in only two instances:

a. The user clicking a "blank" tile, which is produced by removing tiles until none are left in the tile backlog and a blank tile is displayed instead
b. Right-clicking a tile and clicking "Edit" in the context menu

Acceptance criteria:
1. User clicks a blank tile (described in "a"), and an Edit Panel is displayed.  In this Panel, the user can specify a URL for the blank tab or select one of four suggestion options
2. User right-clicks a tile and, in the context menu, sees "Edit...," which produces an Edit Panel for that tile
3. Edit Panel, produced by a or b, allows the user to manually add a URL or add one of four suggested (from backlog if possible) sites to add to that slot
4. Once edited via the Edit Panel, the specified site is pinned to that location in the Tile Grid

Comment 1

[Jennifer Morrow [:Boriss] (UX)]

Attachment: Spec: Adding a custom site to New Tab via the Edit Panel

Making a slight amendment to above description: this panel would appear:

a. The user clicking a "blank" tile.
b. Right-clicking a tile and clicking "Edit" in the context menu
c. The user clicking the gear icon on a tile

c is new.  I'm recommending we remove the "pin" icon from the tile and instead put it in this Edit menu.

Reflected in the attached mockup are a few changes to current behavior:

- A gear icon appears on all tiles in the top right (where the pin icon currently displays)
- Clicking that icon displays the edit panel
- In this edit panel, a tile can be changed either via manual URL entry or clicking one of four suggestions.  These four suggestions, for this bug, will be next four items in the tile backlog.  In the future they may be sponsored tiles.
- If the user clicks one of the four suggested tiles or enters a URL, the edit panel is dismissed and the new tile is added.  A throbber displays while the title and thumbnail are generated (if it is not already in backlog).
- If the user types a URL that cannot connect or is invalid, the string they typed displays under an empty tile

We're gonna need a gear icon - I'll attach it here.

Comment 2

[Jennifer Morrow [:Boriss] (UX)]

Attachment: Icon: Gear for panel .png

Comment 3

[Jennifer Morrow [:Boriss] (UX)]

Attachment: Icons: controls@2x_withgears.png

Comment 4

[Jennifer Morrow [:Boriss] (UX)]

Attachment: Icons: controls_withgears.png

Comment 5

[Jennifer Morrow [:Boriss] (UX)]

Attachment: Icon: Gear for panel 2x .png

Comment 6

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Part 1 - v1 (move CSS to /shared/)

Part 1 of X. This moves almost everything in the theme CSS into /shared/, which makes working on this *so* much easier. Sadly, the CSS is a bit of a mess, as it's split between theme and content - I'm purposefully *not* dealing with that here.

Would like to land this part right away, as it's very prone to bitrot.

Comment 7

[:Gijs (he/him)]

Comment on attachment 8441153 [details] [diff] [review]
Part 1 - v1 (move CSS to /shared/)

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

Sweet!

::: browser/themes/windows/newtab/newTab.css
@@ +1,4 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * 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/. */
> + 

Nit: whitespace

@@ +9,1 @@
>    color: rgb(0,102,204);

Did you figure out what the double color definition here was about? :-\

Comment 8

[:Gijs (he/him)]

Comment on attachment 8441153 [details] [diff] [review]
Part 1 - v1 (move CSS to /shared/)

Landed this immediately to avoid bitrot:

remote:   https://hg.mozilla.org/integration/fx-team/rev/6e84b211041f

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6e84b211041f

Comment 10

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to :Gijs Kruitbosch from comment #7)
> Did you figure out what the double color definition here was about? :-\

Oh, yea, was just a leftover from a previous revision. Review comment requested a hard-coded color, old one was accidentally left in and no one noticed.

Comment 11

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: WiP Part 2 - Implement basic panel

WiP patch to start implementing the panel itself. Going to split the following things out:
* suggestion tiles
* autocomplete
* automatic title fetching

To make this patch of a more reasonable scope.

Comment 12

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Part 2, v1 - Handle bad URLs in BackgrondPageThumbs

First of a couple of patches to add additional error handling to the thumbnail code. This one deals with the case of an bad URL - eg, a protocol we don't know how to handle.

Pretty basic. I think we might want to make it so such tiles get a thumb of an error icon, but I want to do that in a followup.

Comment 13

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: WiP Panel v2

Updated WiP for the actual panel. A lot of time has been spent dealing with coping with edge cases and hooking up page title fetching (which turned out to be necessary to do right away).

Comment 14

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: WiP Suggestions v1

WiP for the suggestions - just an early iteration for the UI part at this stage.

Comment 15

[Drew Willcoxon :adw]

Comment on attachment 8445090 [details] [diff] [review]
Part 2, v1 - Handle bad URLs in BackgrondPageThumbs

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

Thanks.  This is a good idea, but as far as the implementation goes, I'd prefer not to introduce another message type but instead use didCapture and msg.data.imageData to determine whether the capture was successful.  So the backgroundPageThumbsContent.js changes can stay, but we should change Capture.prototype._done to return early, skipping the PageThumbs._store(), if !data || !data.imageData.  And then Capture.prototype.receiveMessage should pass the appropriate telemetry reason to done based on whether imageData is present in the message.  receiveMessage should also skip the tel() if !imageData, since CAPTURE_SERVICE_TIME_MS is for successful captures.

::: toolkit/components/thumbnails/BackgroundPageThumbs.jsm
@@ +16,5 @@
>  const TEL_CAPTURE_DONE_OK = 0;
>  const TEL_CAPTURE_DONE_TIMEOUT = 1;
>  // 2 and 3 were used when we had special handling for private-browsing.
>  const TEL_CAPTURE_DONE_CRASHED = 4;
> +const TEL_CAPTURE_DONE_INVALID = 5;

Maybe a better name would be _BAD_URI?

Comment 16

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Part 2, v2 - Handle bad URLs in BackgrondPageThumbs

You made me remember that I wanted to be able to handle other errors too (eg, URL doesn't resolve), so coping with that better in this patch.

Doing the global lookup hack to avoid having to maintain a duplicate list of the constants.

Comment 17

[Drew Willcoxon :adw]

Comment on attachment 8447046 [details] [diff] [review]
Part 2, v2 - Handle bad URLs in BackgrondPageThumbs

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

::: toolkit/components/thumbnails/BackgroundPageThumbs.jsm
@@ +333,5 @@
> +      return;
> +
> +    if (msg.data.imageData) {
> +      this._done(msg.data, TEL_CAPTURE_DONE_OK);
> +    } else {

Hmm, I kind of don't like how this means that !imageData implies failReason is defined.  I'm wondering if it's possible for imageData to be falsey even in the success case, i.e., if canvas.toBlob() or fileReader.readAsArrayBuffer() silently fails.  What do you think about flipping the branches: if (failReason) {} else {}.  That would preserve, in the absence of failReason, the tried and tested logic that BackgroundPageThumbs has been using here for a while now.

Comment 18

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Ah, good point. Done and pushed:

https://hg.mozilla.org/integration/fx-team/rev/4dda0b28a459

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4dda0b28a459

Comment 20

[Marco Mucci [:MarcoM]]

Removed from Iteration 33.3.  De-prioritized over other higher priority work contained in the Iteration Priority List.

Comment 21

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Part 3: Panel, WiP v2

Ensuring this has my most recent patches. Haven't looked at these in a long time.

Comment 22

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Part 4: Suggestions, WiP v2

Comment 23

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:tspurway, maybe it's time to close this bug?