New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 919961 link

Starred by 5 users

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: 2019-01-22
OS: Windows , Mac
Pri: 2
Type: Feature


Sign in to add a comment

[Dark mode] Support dark mode on the NTP

Project Member Reported by ramyan@chromium.org, Jan 8

Issue description

Overall tracking for dark mode support on the NTP.

Discussion of what's required is in https://docs.google.com/document/d/1PjgxBWgSGsnsoRWufN7GL4XGwwXXAPeq27blotoSEP8/edit
 
Blockedon: 918582 914982
Cc: -namratakannan@chromium.org
Owner: namratakannan@chromium.org
Status: Assigned (was: Available)
Namrata / Joel - can you provide a bit more detail on the elements called out in the linked doc? Thanks!
Labels: KR-NTP-Dark-Mode
Cc: most...@vewd.com
Blocking: 850098
Project Member

Comment 6 by bugdroid1@chromium.org, Jan 14

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

commit 9d6ffd96fca2ea364fea8cece7516ef94750805a
Author: Kristi Park <kristipark@chromium.org>
Date: Mon Jan 14 18:58:22 2019

[NTP] Set up dark mode config, and apply dark mode background when enabled

Set up the dark mode config to be used in all NTP components. Also apply
a dark background for the default NTP when dark mode is enabled.

Default NTP: https://screenshot.googleplex.com/DtOZNv99hfd.png
Third-party NTP: https://screenshot.googleplex.com/kOcDzLmwTmj.png

Bug: 919961
Change-Id: I253cb6655493a5497dcba1fcf16bec4a4e4d19bc
Reviewed-on: https://chromium-review.googlesource.com/c/1407951
Commit-Queue: Kristi Park <kristipark@chromium.org>
Reviewed-by: Kyle Milka <kmilka@chromium.org>
Cr-Commit-Position: refs/heads/master@{#622540}
[modify] https://crrev.com/9d6ffd96fca2ea364fea8cece7516ef94750805a/chrome/browser/resources/local_ntp/local_ntp.js
[modify] https://crrev.com/9d6ffd96fca2ea364fea8cece7516ef94750805a/chrome/browser/search/local_ntp_source.cc

Update on my side. Just getting up to speed and talking to namratakannan@ about NTP dark mode spec. Ill be helping and feel free to ping me if needed. 
Cc: markchang@chromium.org
I noticed two issues in latest Canary Version 73.0.3672.0 starting with --force-dark-mode:

- The Dark New Tab Page flashes white when it opens.

- Not sure if only macOS-related, but if rubber band scrolling happens on bottom of the Dark NTP, there appears a white area which doesn't disappears. Looks like the NTP doesn't bounce back to its normal position after the rubber banding animation ends. Would be also very nice, if the rubber banding background could be dark, too.

Please let me know, if you need more information or if I should file separate reports for the issues.

Thanks :)
Currently investigating the white flash. It seems like this has been a recurring issue for a number of years and is a bit complicated to tackle.

Fix for the rubberband scrolling is in review.
Project Member

Comment 11 by bugdroid1@chromium.org, Jan 16

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

commit 0a51840f2600ac4de036575c0998a8491b28958e
Author: Kristi Park <kristipark@chromium.org>
Date: Wed Jan 16 04:27:22 2019

[NTP] Fix white flash when opening new tab in dark mode

The default NTP background color is excluded from the dark mode spec,
which causes a white flash when a new tab is opened in dark mode.

Now that the NTP is being updated to support dark mode, this exception
is no longer required.

Bug: 919961
Change-Id: Ic545ae2a41bcea7aec1f764cafd78aaa1d749841
Reviewed-on: https://chromium-review.googlesource.com/c/1413240
Commit-Queue: Kristi Park <kristipark@chromium.org>
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#623101}
[modify] https://crrev.com/0a51840f2600ac4de036575c0998a8491b28958e/chrome/browser/themes/theme_properties.cc

Comment 12 by meh...@chromium.org, Jan 16 (6 days ago)

Hey kristipark@, thanks for fixing the white flashing issue. Your CL is not in latest Canary yet, but I checked latest Chromium Snapshot Build Revision 623233, and it seems that your latest CL, that fixes the flash, has also fixed the rubber band scrolling issue. The rubber band background is now dark too (same color like the NTP) and no more white area at the bottom is to see - at least on macOS :) 

I can retest it when it is in Canary.

Comment 13 by lgrey@chromium.org, Jan 16 (6 days ago)

Prompted by issue 922574: the detection logic isn't quite right here. It keeps the NTP dark when the user switches to light mode with `--enable-features=DarkMode` and will fail in general when we remove the feature.

I'm not sure how this is architected, but if it's possible to call into NativeTheme, replacing this check 

https://cs.chromium.org/chromium/src/chrome/browser/search/local_ntp_source.cc?q=local_ntp_source&sq=package:chromium&dr=C&l=572

with

ui::NativeTheme::GetInstanceForNativeUi()->SystemDarkModeEnabled()

should fix it


Comment 14 by kristip...@chromium.org, Jan 16 (6 days ago)

Now using SystemDarkModeEnabled as of https://crrev.com/c/1407204.
Project Member

Comment 15 by bugdroid1@chromium.org, Jan 18 (4 days ago)

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

commit 737f310470c79fda62d36b7fea0b7f654b1fa50a
Author: Kristi Park <kristipark@chromium.org>
Date: Fri Jan 18 18:55:42 2019

[NTP] Support dark mode for toast notifications

This includes custom link notifications, error notifications, promo,
and the third-party NTP blacklist notification.

Custom link: https://screenshot.googleplex.com/6Y2AEjEDMbL.png
Error: https://screenshot.googleplex.com/wqxj7B6BF5S.png
Promo: https://screenshot.googleplex.com/MRQsvhTnnUe.png
Third-party NTP: https://screenshot.googleplex.com/CzFrGZgYZ94.png

Bug: 919961
Change-Id: I86420e3eaa18600c84e88d088529f940b3b535cf
Reviewed-on: https://chromium-review.googlesource.com/c/1419205
Commit-Queue: Kristi Park <kristipark@chromium.org>
Reviewed-by: Kyle Milka <kmilka@chromium.org>
Cr-Commit-Position: refs/heads/master@{#624217}
[modify] https://crrev.com/737f310470c79fda62d36b7fea0b7f654b1fa50a/chrome/browser/resources/local_ntp/local_ntp.css
[modify] https://crrev.com/737f310470c79fda62d36b7fea0b7f654b1fa50a/chrome/browser/resources/local_ntp/local_ntp.js

Project Member

Comment 16 by bugdroid1@chromium.org, Jan 18 (4 days ago)

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

commit 680ed8f316f5f4241b9a614043d88fa4f3ed8d83
Author: Kristi Park <kristipark@chromium.org>
Date: Fri Jan 18 20:09:39 2019

[NTP] Use white One Google Bar and white Google logo when in dark mode

If dark mode is enabled, use the white One Google Bar and white Google
logo. If a theme is installed, use the default theme values.

Bug: 919961, 918582
Change-Id: Id9fe74511a7f18e1256372ed3b782fd5266ca543
Reviewed-on: https://chromium-review.googlesource.com/c/1419362
Commit-Queue: Kristi Park <kristipark@chromium.org>
Reviewed-by: Kyle Milka <kmilka@chromium.org>
Cr-Commit-Position: refs/heads/master@{#624252}
[modify] https://crrev.com/680ed8f316f5f4241b9a614043d88fa4f3ed8d83/chrome/browser/resources/local_ntp/local_ntp.js

Comment 17 by kristip...@chromium.org, Jan 18 (4 days ago)

Blockedon: 923558

Comment 18 by monor...@bugs.chromium.org, Today (18 hours ago)

The NextAction date has arrived: 2019-01-22

Comment 19 by kristip...@chromium.org, Today (9 hours ago)

Blockedon: 923988

Sign in to add a comment