Added theme change support in speedreader. #14307

boocmp · 2022-07-23T04:46:11Z

Submitter Checklist:

I confirm that no security/privacy review is needed, or that I have requested one
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

NOTE: Downloadable component isn't updated yet. So to check this PR you should use built-in css stylesheet.
Make sure the component with id 'jicbkmdloagakknpihibphagfckhjdih' is removed from the profile before running browser for testing.

Select some mode in Appearance->Brave Colors Settings.
Navigate to the speedreadable page (f.e. https://www.bbc.com/news/business-58559179)
Check mode corresponds to the selected in 1
Open DevTools js console
Paste document.documentElement.setAttribute('data-theme', 'dark') <- 'dark' or 'light' or 'sepia' should change mode.
Paste document.documentElement.removeAttribute('data-theme') should return mode to the selected in 1.

NOTE: If something goes wrong, check component 'jicbkmdloagakknpihibphagfckhjdih' is not downloaded.

brave-builds · 2022-07-23T05:31:36Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2022-07-23T10:59:06Z

A Storybook has been deployed to preview UI for the latest push

browser/speedreader/speedreader_tab_helper.h

browser/speedreader/speedreader_tab_helper.cc

nullhook · 2022-07-25T23:09:09Z

components/speedreader/resources/speedreader-desktop.css

@@ -82,6 +87,7 @@ a[href] {
 #article {
  text-rendering: optimizeLegibility;
  font-family: "Georgia", "Times New Roman", Times, serif;
+  user-select: unset;


why are we disabling user's ability to select text in the article?

I took the entire speedreader-desktop.css file from the downloadable components. I don't know why :/

UPD: This rule doesn't disable text selection. It should be 'none' to disable, am i wrong ?

oh, right. still not sure which value it's unsetting from or why it's needed

nullhook · 2022-07-25T23:27:16Z

components/speedreader/rust/lib/src/readability/src/extractor.rs

@@ -281,6 +282,11 @@ pub fn extract_dom<S: ::std::hash::BuildHasher>(
        let title_blob = format!("<title>{}</title>", &meta.title);
        content = title_blob + &content;
    }
+
+    if let Some(theme) = theme {
+        content = format!("<html theme-data=\"{}\">", theme) + &content + "</html>";


custom attributes are prefixed with data-*

brave-builds · 2022-07-26T07:05:20Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2022-07-26T10:36:02Z

A Storybook has been deployed to preview UI for the latest push

nullhook · 2022-07-26T14:02:21Z

browser/speedreader/speedreader_tab_helper.h

-  std::string GetSystemTheme();
+  void SetTheme(Theme theme);
+  Theme GetTheme();
+  std::string GetThemeName();


why do we need ::GetThemeName here again? We're probably using the service method or is this a left over?

Yep, should be deleted.

nullhook · 2022-07-26T14:12:24Z

components/speedreader/speedreader_util.h

@@ -11,6 +11,14 @@ class HostContentSettingsMap;

 namespace speedreader {

+// Enum is used in prefs. Be careful when changing or extending.
+enum class Theme {
+  kDefault = 0,  // Use browser's theme


this should be kNone which implies the user hasn't set a theme.

browser/speedreader/speedreader_tab_helper.cc

nullhook · 2022-07-26T14:26:13Z

components/speedreader/speedreader_service.cc

+    case Theme::kDark:
+      return "dark";
+  }
+  NOTREACHED();


why NOTREACHED and not a default: switch case? a default switch case will always fallback

switch (s) { // cases default: return {}; } // nothing needed here

brave-builds · 2022-07-26T16:15:10Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2022-07-27T04:06:27Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2022-07-27T05:35:25Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2022-07-27T06:30:21Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2022-07-27T08:55:18Z

A Storybook has been deployed to preview UI for the latest push

components/speedreader/speedreader_service.cc

nullhook · 2022-07-27T17:21:10Z

browser/speedreader/speedreader_tab_helper.cc

@@ -291,7 +291,7 @@ void SpeedreaderTabHelper::SetTheme(Theme theme) {
 Theme SpeedreaderTabHelper::GetTheme() {
  const Theme theme =
      SpeedreaderServiceFactory::GetForProfile(GetProfile())->GetTheme();
-  if (theme == Theme::kDefault) {
+  if (theme == Theme::kNone) {
    switch (dark_mode::GetActiveBraveDarkModeType()) {
      case dark_mode::BraveDarkModeType::BRAVE_DARK_MODE_TYPE_DARK:


what is ::BRAVE_DARK_MODE_TYPE_DEFAULT ?

// DEFAULT type acts as two ways depends on system theme mode.
// If system dark mode is not supported, we override it with channel based
// policy. In this case, user can see dark or light option in settings.
// Otherwise, it acts like system dark mode mode. It respects system's dark
// mode. In this case, user can see all three options in theme settings.

GetActiveBraveDarkModeType never returns default.

brave-builds · 2022-07-28T05:15:04Z

A Storybook has been deployed to preview UI for the latest push

iefremov · 2022-07-29T13:50:17Z

components/speedreader/speedreader_service.cc

@@ -7,6 +7,7 @@

 #include "base/feature_list.h"
 #include "base/metrics/histogram_macros.h"
+#include "base/notreached.h"


iefremov · 2022-07-29T13:52:07Z

browser/brave_content_browser_client.cc

 #include "brave/browser/speedreader/speedreader_tab_helper.h"
 #include "brave/components/speedreader/speedreader_throttle.h"
+#include "brave/components/speedreader/speedreader_util.h"


used in line 744

brave-builds · 2022-08-01T06:29:51Z

A Storybook has been deployed to preview UI for the latest push

nullhook

lgtm

boocmp requested a review from iefremov as a code owner July 23, 2022 04:46

github-actions bot assigned boocmp Jul 23, 2022

github-actions bot added the CI/storybook-url Deploy storybook and provide a unique URL for each build label Jul 23, 2022

boocmp requested a review from nullhook July 23, 2022 04:46

boocmp force-pushed the issues/23447 branch from fe503ba to 0524b9b Compare July 23, 2022 10:28

nullhook suggested changes Jul 25, 2022

View reviewed changes

browser/speedreader/speedreader_tab_helper.h Outdated Show resolved Hide resolved

browser/speedreader/speedreader_tab_helper.cc Outdated Show resolved Hide resolved

nullhook reviewed Jul 25, 2022

View reviewed changes

boocmp added 5 commits July 26, 2022 20:41

Added theme change support in speedreader.

8d4c72b

Fixed position of 'original page' link.

30a7d4e

Renamed theme-data to data-theme.

bc3599a

Theme enum instead of string.

8f813a5

Fixed test.

e34f448

nullhook suggested changes Jul 26, 2022

View reviewed changes

Review issues are addressed.

aa9bb03

boocmp force-pushed the issues/23447 branch from c662ad0 to aa9bb03 Compare July 26, 2022 15:03

boocmp force-pushed the issues/23447 branch 2 times, most recently from 8f632cb to 137bd35 Compare July 27, 2022 05:31

Fix SetTheme logic.

122da45

boocmp force-pushed the issues/23447 branch from 137bd35 to 122da45 Compare July 27, 2022 08:01

nullhook suggested changes Jul 27, 2022

View reviewed changes

iefremov reviewed Jul 29, 2022

View reviewed changes

iefremov approved these changes Jul 29, 2022

View reviewed changes

Review.

b921e63

boocmp force-pushed the issues/23447 branch from 6a0448d to b921e63 Compare August 1, 2022 05:28

boocmp requested a review from nullhook August 1, 2022 11:14

nullhook approved these changes Aug 1, 2022

View reviewed changes

boocmp merged commit a9d5f12 into master Aug 2, 2022

boocmp deleted the issues/23447 branch August 2, 2022 08:45

github-actions bot added this to the 1.44.x - Nightly milestone Aug 2, 2022

boocmp mentioned this pull request Aug 3, 2022

Themes support CSS update. brave-experiments/SpeedReader#36

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added theme change support in speedreader. #14307

Added theme change support in speedreader. #14307

boocmp commented Jul 23, 2022 •

edited

Loading

brave-builds commented Jul 23, 2022

brave-builds commented Jul 23, 2022

nullhook Jul 25, 2022

boocmp Jul 26, 2022

boocmp Jul 26, 2022

nullhook Jul 26, 2022

nullhook Jul 25, 2022 •

edited

Loading

brave-builds commented Jul 26, 2022

brave-builds commented Jul 26, 2022

nullhook Jul 26, 2022

boocmp Jul 26, 2022

nullhook Jul 26, 2022

nullhook Jul 26, 2022

nullhook Jul 26, 2022

brave-builds commented Jul 26, 2022

brave-builds commented Jul 27, 2022

brave-builds commented Jul 27, 2022

brave-builds commented Jul 27, 2022

brave-builds commented Jul 27, 2022

nullhook Jul 27, 2022

boocmp Jul 28, 2022

brave-builds commented Jul 28, 2022

iefremov Jul 29, 2022

iefremov Jul 29, 2022

boocmp Aug 1, 2022

brave-builds commented Aug 1, 2022

nullhook left a comment •

edited

Loading

Added theme change support in speedreader. #14307

Added theme change support in speedreader. #14307

Conversation

boocmp commented Jul 23, 2022 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

brave-builds commented Jul 23, 2022

brave-builds commented Jul 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nullhook Jul 25, 2022 • edited Loading

Choose a reason for hiding this comment

brave-builds commented Jul 26, 2022

brave-builds commented Jul 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brave-builds commented Jul 26, 2022

brave-builds commented Jul 27, 2022

brave-builds commented Jul 27, 2022

brave-builds commented Jul 27, 2022

brave-builds commented Jul 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brave-builds commented Jul 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brave-builds commented Aug 1, 2022

nullhook left a comment • edited Loading

Choose a reason for hiding this comment

boocmp commented Jul 23, 2022 •

edited

Loading

nullhook Jul 25, 2022 •

edited

Loading

nullhook left a comment •

edited

Loading