When the status card has an action button attached instead of included in the card, they should still swipe in sync, and swiping any of them moves both.

Moving the conversation here from the CL (https://codereview.chromium.org/2340333002/) to get Patrick's input.

A preview of the changes is available at https://goo.gl/photos/ooDFBuAVCLauo4bbA

On 2016/09/16 10:07:50, Marc Treib wrote:
> Code mostly LG, but I have some general concerns:
> 
> - Dismissing a section has somewhat different semantics from dismissing an
> individual suggestion. That seems like it might be confusing for users?
> 

What it looks like to the users is dismissing a status card. But since there is nothing
else remaining when that happens, I implemented it as dismissing the section. Then when
the status changes, we show the section again, with a different status card, that the
user did not dismiss. So it should look all right.

> - I'm not completely convinced that "when the status changes" is exactly the
> right trigger to make the section reappear - just seems a bit arbitrary. E.g.
> each Chrome restart will make it reappear, which on Android roughly translates
> to "whenever".

Because of the way status changes, they would get updated during the startup sequence so even if we persist it, tracking the latest state would not quite work. I guess we could have some logic to un-stick dismissed status cards when we enter other specific states, but I'm also afraid users completely remove suggestions and can't get them back.

For suggestions, that would be the signin promo, and for bookmarks we would lose the entry point to the bookmarks manager. I can do it in a follow up CL if that's the expected behaviour.

Re "dismissing a status card": Looking at it like this makes a lot of sense, thanks! I withdraw my concerns in that regard :)

What I'm not clear on is: Under what circumstances do we *want* a section to re-appear? (independent of concrete implementation for now)
The current implementation is probably good enough for a first iteration, but I'd like us to figure this out properly before M55 :)

Currently, we only have 2 status cards. I suppose we can go with:

=> Sign In (articles): if dismissed, we show it again if the user has signed in since.

=> No Suggestions (articles, bookmarks): if dismissed, we show it again if new content has been received.

All other status are either anomalies or ignored and not shown to the user.


# Special case:

I dismiss the "No Suggestions" card, then sign out from Chrome. I can now see a sign in promo that tells me to sign in to get content suggestions. I do sign in, but for some reason there are no suggestions. So I don't see anything new coming up and I'm confused.

So we could clear all sticky dismissals if any of the exit conditions are reached. Alternatively, if a new status card is shown. Meaning that we'd be tracking only the latest status card dismissal, for each category


# No below the fold

Also, allowing to dismiss status cards means that we can end up in a state where we have nothing below the fold. That state can be seen in the video linked in #c8, at 0:50. Do we want to do something special in that case? +rachelis@

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/212feea3b3df08ddb7d25d8b6293344ebc5d5b9b

commit 212feea3b3df08ddb7d25d8b6293344ebc5d5b9b
Author: dgn <dgn@chromium.org>
Date: Fri Sep 16 15:08:20 2016

[NTP Client] Make status cards swipable

When the status card is swiped away, the entire section is dismissed and
stays that way on NTPs opened afterwards. If the status of the section
changes (new snippets loaded, user logged in, etc.) the section will be
added back.

Preview: https://goo.gl/photos/ooDFBuAVCLauo4bbA

BUG= 638580 

Review-Url: https://codereview.chromium.org/2340333002
Cr-Commit-Position: refs/heads/master@{#419170}

[modify] https://crrev.com/212feea3b3df08ddb7d25d8b6293344ebc5d5b9b/chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ActionItem.java
[modify] https://crrev.com/212feea3b3df08ddb7d25d8b6293344ebc5d5b9b/chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java
[modify] https://crrev.com/212feea3b3df08ddb7d25d8b6293344ebc5d5b9b/chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java
[modify] https://crrev.com/212feea3b3df08ddb7d25d8b6293344ebc5d5b9b/chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java
[modify] https://crrev.com/212feea3b3df08ddb7d25d8b6293344ebc5d5b9b/chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/StatusItem.java
[modify] https://crrev.com/212feea3b3df08ddb7d25d8b6293344ebc5d5b9b/chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java
[modify] https://crrev.com/212feea3b3df08ddb7d25d8b6293344ebc5d5b9b/chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/FakeSuggestionsSource.java
[modify] https://crrev.com/212feea3b3df08ddb7d25d8b6293344ebc5d5b9b/chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsBridge.java
[modify] https://crrev.com/212feea3b3df08ddb7d25d8b6293344ebc5d5b9b/chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SuggestionsSource.java
[modify] https://crrev.com/212feea3b3df08ddb7d25d8b6293344ebc5d5b9b/chrome/android/junit/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapterTest.java
[modify] https://crrev.com/212feea3b3df08ddb7d25d8b6293344ebc5d5b9b/chrome/browser/android/ntp/ntp_snippets_bridge.cc
[modify] https://crrev.com/212feea3b3df08ddb7d25d8b6293344ebc5d5b9b/chrome/browser/android/ntp/ntp_snippets_bridge.h
[modify] https://crrev.com/212feea3b3df08ddb7d25d8b6293344ebc5d5b9b/components/ntp_snippets/content_suggestions_service.cc
[modify] https://crrev.com/212feea3b3df08ddb7d25d8b6293344ebc5d5b9b/components/ntp_snippets/content_suggestions_service.h

Thanks for the video in Comment 8. This LGTM.

Rachel?

Thanks Nicolas, looks great!

Two things I'm wondering about here:
- Should we allow the "more" section to be swiped away?
- And if we allow sections to be swiped away, does that mean that the card above the fold should disappear?

On one hand, this may be the user intent - they could even use the card reappearing as a way of letting them know that there's more content. On the other hand, it means that there's no way for them to manually reload via the card if they accidentally dismiss. I guess that this currently has the same effect as reloading the page?

> Should we allow the "more" section to be swiped away?

It is swipeable only when it is "attached" to the status card, so the "more" section can't be swiped away when there are suggestions. Do you mean you would prefer it to stay static, we would only swipe and animate the status card itself? Patrick commented on the PRD: "discarding an empty state card would disard the more card as well and vice-versa." So I implemented what I understood from that.

> And if we allow sections to be swiped away, does that mean that the card above the fold should disappear?

Do you want a "nothing to see here" card for the whole of the feature? Maybe we could put entry points to various things there (I remember a prototype with bookmarks, recents, downloads, etc.)

> Reloading

If the user dismiss the card allowing them to force a new fetch, they can't do it anymore, indeed. Reloading the NTP would not get new suggestions, they would have to wait until a scheduled fetch returns new results to get suggestions on the NTP

Removing the zine UX label, since this doesn't currently need my input. (Please re-add as needed)

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5291472f4339aca714470a1359ffb4e444de7960

commit 5291472f4339aca714470a1359ffb4e444de7960
Author: dgn <dgn@chromium.org>
Date: Tue Oct 18 10:28:42 2016

[NTP Client] Persist category dismissals

- Persists the ids of dismissed categories to preferences as a
  list of category ids
- Restores the dismissed categories from the list of dismissed
  ids when the ContentSuggestionService is created, and matches
  with providers when they are registered.
- Changes the way dismissals are cleared, from happening when
  anything about the category changes to only happen when a
  non empty list of suggestions is received.

BUG= 638580 

Review-Url: https://codereview.chromium.org/2406573002
Cr-Commit-Position: refs/heads/master@{#425939}

[modify] https://crrev.com/5291472f4339aca714470a1359ffb4e444de7960/chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java
[modify] https://crrev.com/5291472f4339aca714470a1359ffb4e444de7960/chrome/browser/prefs/browser_prefs.cc
[modify] https://crrev.com/5291472f4339aca714470a1359ffb4e444de7960/components/ntp_snippets/category.h
[modify] https://crrev.com/5291472f4339aca714470a1359ffb4e444de7960/components/ntp_snippets/category_factory.cc
[modify] https://crrev.com/5291472f4339aca714470a1359ffb4e444de7960/components/ntp_snippets/category_factory_unittest.cc
[modify] https://crrev.com/5291472f4339aca714470a1359ffb4e444de7960/components/ntp_snippets/content_suggestions_service.cc
[modify] https://crrev.com/5291472f4339aca714470a1359ffb4e444de7960/components/ntp_snippets/content_suggestions_service.h
[modify] https://crrev.com/5291472f4339aca714470a1359ffb4e444de7960/components/ntp_snippets/content_suggestions_service_unittest.cc
[modify] https://crrev.com/5291472f4339aca714470a1359ffb4e444de7960/components/ntp_snippets/pref_names.cc
[modify] https://crrev.com/5291472f4339aca714470a1359ffb4e444de7960/components/ntp_snippets/pref_names.h

The blocking bug was fixed. Is now something left do to here?

I'm working on an issue with UMA (659728) and we might have also to do something about accessibility. There is no context menu and no announcement at the moment.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/be9cf09ebbbb0990e0a5c82b5a68c4523259b35b

commit be9cf09ebbbb0990e0a5c82b5a68c4523259b35b
Author: dgn <dgn@chromium.org>
Date: Wed Nov 16 20:36:18 2016

[NTP Client] Add a context menu to remove status items

Also modifies the ContextMenuHelper and related interfaces
to make it easier to select which items to show.

BUG= 638580 , 663747 

Review-Url: https://codereview.chromium.org/2459033002
Cr-Commit-Position: refs/heads/master@{#432613}

[delete] https://crrev.com/085701ef9a4ad12ba89368704c4f92f6d8dbccfe/chrome/android/java/src/org/chromium/chrome/browser/ntp/ContextMenuHandler.java
[add] https://crrev.com/be9cf09ebbbb0990e0a5c82b5a68c4523259b35b/chrome/android/java/src/org/chromium/chrome/browser/ntp/ContextMenuManager.java
[modify] https://crrev.com/be9cf09ebbbb0990e0a5c82b5a68c4523259b35b/chrome/android/java/src/org/chromium/chrome/browser/ntp/MostVisitedItem.java
[modify] https://crrev.com/be9cf09ebbbb0990e0a5c82b5a68c4523259b35b/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
[modify] https://crrev.com/be9cf09ebbbb0990e0a5c82b5a68c4523259b35b/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageScrollView.java
[modify] https://crrev.com/be9cf09ebbbb0990e0a5c82b5a68c4523259b35b/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java
[modify] https://crrev.com/be9cf09ebbbb0990e0a5c82b5a68c4523259b35b/chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ActionItem.java
[modify] https://crrev.com/be9cf09ebbbb0990e0a5c82b5a68c4523259b35b/chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/CardViewHolder.java
[modify] https://crrev.com/be9cf09ebbbb0990e0a5c82b5a68c4523259b35b/chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageAdapter.java
[modify] https://crrev.com/be9cf09ebbbb0990e0a5c82b5a68c4523259b35b/chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerView.java
[modify] https://crrev.com/be9cf09ebbbb0990e0a5c82b5a68c4523259b35b/chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SignInPromo.java
[modify] https://crrev.com/be9cf09ebbbb0990e0a5c82b5a68c4523259b35b/chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/StatusCardViewHolder.java
[modify] https://crrev.com/be9cf09ebbbb0990e0a5c82b5a68c4523259b35b/chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java
[modify] https://crrev.com/be9cf09ebbbb0990e0a5c82b5a68c4523259b35b/chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetArticleViewHolder.java
[modify] https://crrev.com/be9cf09ebbbb0990e0a5c82b5a68c4523259b35b/chrome/android/java_sources.gni
[modify] https://crrev.com/be9cf09ebbbb0990e0a5c82b5a68c4523259b35b/chrome/android/javatests/src/org/chromium/chrome/browser/ntp/NewTabPageTest.java
[modify] https://crrev.com/be9cf09ebbbb0990e0a5c82b5a68c4523259b35b/chrome/android/javatests/src/org/chromium/chrome/browser/ntp/cards/NewTabPageRecyclerViewTest.java
[modify] https://crrev.com/be9cf09ebbbb0990e0a5c82b5a68c4523259b35b/chrome/android/javatests/src/org/chromium/chrome/browser/ntp/snippets/ArticleSnippetsTest.java