Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Communicate speedreader distill result back to tab helper #17355

Closed
kkuehlz opened this issue Aug 4, 2021 · 2 comments · Fixed by brave/brave-core#9659
Closed

Communicate speedreader distill result back to tab helper #17355

kkuehlz opened this issue Aug 4, 2021 · 2 comments · Fixed by brave/brave-core#9659

Comments

@kkuehlz
Copy link
Contributor

kkuehlz commented Aug 4, 2021

This can cause pages that pass the URL heuristic to show up as being Speedread.

One known example is cnet.com/news/

Screenshot_20210802_134015

@kkuehlz kkuehlz self-assigned this Aug 4, 2021
@kkuehlz kkuehlz added priority/P3 The next thing for us to work on. It'll ride the trains. feature/speedreader OS/Desktop QA/Yes release-notes/include bug labels Aug 4, 2021
kkuehlz pushed a commit to brave/brave-core that referenced this issue Aug 5, 2021
kkuehlz pushed a commit to brave/brave-core that referenced this issue Aug 27, 2021
kkuehlz pushed a commit to brave/brave-core that referenced this issue Sep 1, 2021
kkuehlz pushed a commit to brave/brave-core that referenced this issue Sep 2, 2021
@kkuehlz kkuehlz added this to the 1.31.x - Nightly milestone Sep 3, 2021
@kkuehlz
Copy link
Contributor Author

kkuehlz commented Sep 3, 2021

Test Plan

(1) Go to sites that work in Speedreader (Guardian, HuffPost, whatever you like). Ensure forward and back navigation between Speedreadable pages and non-Speedreadable pages (for example the news homepage) works as expected.

(2) Ensure that closing and restoring the browser with distilled pages restores with them and distilled, and with the icon present.

(3) cnet.com/news should no longer have the Speedreader icon.

@stephendonner
Copy link

Verified PASSED using

Brave 1.31.25 Chromium: 93.0.4577.63 (Official Build) nightly (x86_64)
Revision ff5c0da2ec0adeaed5550e6c7e98417dac77d98a-refs/branch-heads/4577@{#1135}
OS macOS Version 11.5.2 (Build 20G95)

(1) Steps:

  1. new profile
  2. launched Brave
  3. enabled Speedreader via brave://settings/appearance
  4. confirmed that going back and forth between sfgate.com and https://www.sfgate.com/streaming/article/Netflix-s-Outer-Banks-is-the-well-made-trash-16440428.php?IPID=SFGate-HP-CP-Spotlight and huffpost.com and https://www.huffpost.com/entry/civil-rights-groups-lawsuits-texas-voting-law_n_6137ac0de4b04778c00ac652 showed the default (non-Speedreader) and Speedreader views accordingly.
sfgate.com sfgate.com article huffpost.com huffpost.com article
Screen Shot 2021-09-07 at 3 20 34 PM Screen Shot 2021-09-07 at 3 20 38 PM Screen Shot 2021-09-07 at 3 20 25 PM Screen Shot 2021-09-07 at 3 20 29 PM

(2) Steps:

  1. new profile
  2. launched Brave
  3. enabled Speedreader via brave://settings/appearance
  4. loaded https://www.huffpost.com/entry/civil-rights-groups-lawsuits-texas-voting-law_n_6137ac0de4b04778c00ac652 and https://www.sfgate.com/streaming/article/Netflix-s-Outer-Banks-is-the-well-made-trash-16440428.php?IPID=SFGate-HP-CP-Spotlight
  5. shut down
  6. restarted Brave
  7. confirmed both of the sites in Setup antimuon repo and use it instead of muon #4 were restored upon relaunch

Screen Shot 2021-09-07 at 3 16 33 PM

(3) Steps:

  1. new profile
  2. launched Brave
  3. enabled Speedreader via brave://settings/appearance
  4. visited https://www.cnet.com/news/ and confirmed I saw no Speedreader icon

Screen Shot 2021-09-07 at 3 01 52 PM

@rebron rebron changed the title Communicate Speedreader distill result back to tab helper Communicate speedreader distill result back to tab helper Oct 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants