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

[De-AMP] Add support for checking de_amp_enabled user pref in cosmetic js filter #22228

Closed
ShivanKaul opened this issue Apr 12, 2022 · 6 comments · Fixed by brave/brave-core#12923

Comments

@ShivanKaul
Copy link
Collaborator

Add support for checking of DeAmp pref and flag in Cosmetic Filters JS Handlers, and add the boolean to the wrapping pre-init script.

This is done so that we can add a new scriptlet for removing jsaction HTML attributes from anchor tags on Google SERP.

More details here: https://docs.google.com/document/d/1kvHbQMdE_vxmRWRz0LfAHzf6XoTT_pQXSwxth5rVhsk/edit#heading=h.okf43yiuecg5

@kjozwiak
Copy link
Member

The above will require 1.38.105 or higher for 1.38.x verification 👍 Examples verification being completed can be found via brave/brave-core#12923 (comment).

@MadhaviSeelam
Copy link

MadhaviSeelam commented Apr 26, 2022

Verification Passed

Brave 1.38.106 Chromium: 101.0.4951.41 (Official Build) (64-bit)
Revision 93c720db8323b3ec10d056025ab95c23a31997c9-refs/branch-heads/4951@{#904}
OS Windows 11 Version 21H2 (Build 22000.613)

Pre-requisites: Mobile emulation via DevTools and set to Samsung Galaxy S8+

de-AMP Disabled

ex1 ex2 ex3 ex4
de-Amp_settings page ss1 de-Amp1 <de-Amp2

de-AMP Enabled

  • ensured that Auto-redirect AMP pages was enabled via brave://settings/shields
  • loaded https://www.google.co.uk and then searched for Top Stories
  • inspected the first article via DevTools and ensured that jsaction attribute wasn't appeared in the anchor link
  • clicked on the link and ensured that the de-AMP version of the website/news article was being loaded
  • validated with google.com as well and results were same as expected
ex1 ex2 ex3 ex4
Enable DeAmp ss2 ss3 ss4

@stephendonner
Copy link

stephendonner commented Apr 26, 2022

Verified PASSED using

Brave 1.38.107 Chromium: 101.0.4951.41 (Official Build) (x86_64)
Revision 93c720db8323b3ec10d056025ab95c23a31997c9-refs/branch-heads/4951@{#904}
OS macOS Version 11.6.5 (Build 20G527)

Pre-requisites: Mobile emulation via DevTools and set to Samsung Galaxy S8+

de-AMP Disabled

  • ensured that Auto-redirect AMP pages was disabled via brave://settings/shields
  • Loaded https://www.google.co.uk and then searched for Top Stories
  • inspected the first article via DevTools and ensured that jsaction attribute appeared in the anchor link
  • clicked on the link and ensured that the AMP version of the website/news article was being loaded
    • URL: https://www.nytimes.com/live/2022/04/25/world/ukraine-russia-war-news.amp.html
  • validated with google.com as well and results were same as expected
ex1 ex2 ex3 ex4
Screen Shot 2022-04-26 at 9 14 18 AM Screen Shot 2022-04-26 at 10 02 06 AM Screen Shot 2022-04-26 at 9 48 45 AM Screen Shot 2022-04-26 at 9 53 32 AM

de-AMP Enabled

  • ensured that Auto-redirect AMP pages was enabled via brave://settings/shields
  • loaded https://www.google.co.uk and then searched for Top Stories
  • inspected the first article via DevTools and ensured that jsaction attribute didn't appear in the anchor link
  • clicked on the link and ensured that the de-AMP version of the website/news article was being loaded
  • validated with google.com as well and results were same as expected
ex1 ex2 ex3 ex4
Screen Shot 2022-04-26 at 11 16 47 AM Screen Shot 2022-04-26 at 11 12 08 AM Screen Shot 2022-04-26 at 11 11 35 AM Screen Shot 2022-04-26 at 11 12 34 AM

@kjozwiak
Copy link
Member

kjozwiak commented Apr 26, 2022

Verification PASSED on Samsung S10+ running Android 12 using the following build(s):

Brave | 1.38.107 Chromium: 101.0.4951.41 (Official Build) (32-bit)
--- | ---
Revision | 93c720db8323b3ec10d056025ab95c23a31997c9-refs/branch-heads/4951@{#904}
OS | Android 12; Build/SP1A.210812.016

Went through the STR/Cases outlined via brave/brave-core#12923 (comment).

de-AMP Disabled

  • ensured that Auto-redirect AMP pages was disabled via brave://settings/privacy
  • loaded https://www.google.com and then searched for Top Stories
  • inspected the first article via DevTools and ensured that jsaction attribute appeared in the anchor link
  • clicked on the link and ensured that the AMP version of the website/news article was being loaded
    • Example --> https://www.cbc.ca/amp/1.6425708
Example Example Example Example
Screenshot_20220426-140000_Brave image Screenshot_20220426-140048_Brave Screenshot_20220426-140256_Brave

de-AMP Enabled

  • ensured that Auto-redirect AMP pages was enabled via brave://settings/privacy
  • loaded https://www.google.com and then searched for Top Stories
  • inspected the first article via DevTools and ensured that jsaction attribute wasn't appeared in the anchor link
  • clicked on the link and ensured that the de-AMP version of the website/news article was being loaded
Example Example Example
Screenshot_20220426-141338_Brave image Screenshot_20220426-141441_Brave

Quick note: I also tried https://www.google.ca & https://www.google.co.uk

@kjozwiak
Copy link
Member

After speaking with @ShivanKaul, running through the mobile cases should be enough and tablet can be skipped. Tablet doesn't use AMP as much as the mobiles due to the view usually being Desktop due to the bigger screens. However, double checked and ensured that the feature has been enabled by default.

@btlechowski
Copy link

Verification passed on

Brave 1.38.107 Chromium: 101.0.4951.41 (Official Build) (64-bit)
Revision 93c720db8323b3ec10d056025ab95c23a31997c9-refs/branch-heads/4951@{#904}
OS Ubuntu 18.04 LTS

Pre-requisites: Mobile emulation via DevTools and set to Samsung Galaxy S8+

de-AMP Disabled

image
image
image

de-AMP Enabled

  • ensured that Auto-redirect AMP pages was enabled via brave://settings/shields
  • loaded https://www.google.co.uk and then searched for Top Stories
  • inspected the first article via DevTools and ensured that jsaction attribute wasn't appeared in the anchor link
  • clicked on the link and ensured that the de-AMP version of the website/news article was being loaded
  • validated with google.com as well and results were same as expected

image
image
image

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