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

C77 - "Distill page" button shows in omnibox #5951

Closed
bsclifton opened this issue Sep 9, 2019 · 4 comments · Fixed by brave/brave-core#3397
Closed

C77 - "Distill page" button shows in omnibox #5951

bsclifton opened this issue Sep 9, 2019 · 4 comments · Fixed by brave/brave-core#3397

Comments

@bsclifton
Copy link
Member

bsclifton commented Sep 9, 2019

Test plan

Make sure the Distill page button (pictured below) does not show up on:

  • public web sites
  • internal pages
  • guest window
  • tor window
  • different profiles

Description

Starting with Chromium 77, a "Distill page" button is shown in the omnibox
Screen Shot 2019-09-09 at 12 29 16 PM

This is used to provide a reader view of a given page. However, when using on internal pages like brave://welcome, it breaks:
image

Steps to Reproduce

  1. Have a build with Chromium 77 in it
  2. Visit any page; verify Distill page button shows in omnibox
  3. Visit brave://welcome
  4. Click the Distill page button

Actual result:

Error (as pictured in description)

Expected result:

Either:

  • no button
    OR
  • don't show button on internal pages

Reproduces how often:

100%

@bsclifton bsclifton added this to the 0.69.x - Release milestone Sep 9, 2019
@rebron
Copy link
Collaborator

rebron commented Sep 9, 2019

Let's hide the distill button in all cases from the url bar.

@bridiver
Copy link
Contributor

  // TODO(gilmanmh): Display icon for only those pages that are likely to
  // render well in Reader Mode.
  SetVisible(true);

I think we can do a chromium_src override of ReaderModeIconView in OmniboxPageActionIconContainerView and override Update to

bool ReaderModeIconView::Update() {
    SetVisible(false);
    return false;
 }

@petemill
Copy link
Member

Reminder for myself and @bsclifton - this still needs uplift to 69, 70 and 71

@GeetaSarvadnya
Copy link

GeetaSarvadnya commented Sep 11, 2019

Verification passed on

Brave 0.68.134 Chromium: 77.0.3865.75 (Official Build) (64-bit)
Revision 201e747d032611c5f2785cae06e894cf85be7f8a-refs/branch-heads/3865@{#776}
OS Windows 10 OS Version 1803 (Build 17134.523)

Verified passed with

Brave 0.68.134 Chromium: 77.0.3865.75 (Official Build) (64-bit)
Revision 201e747d032611c5f2785cae06e894cf85be7f8a-refs/branch-heads/3865@{#776}
OS macOS Version 10.13.6 (Build 17G5019)

Verification passed on

Brave 0.68.134 Chromium: 77.0.3865.75 (Official Build) (64-bit)
Revision 201e747d032611c5f2785cae06e894cf85be7f8a-refs/branch-heads/3865@{#776}
OS Ubuntu 18.04 LTS

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment