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.

Sign up for GitHub

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

Jump to bottom

[Settings] Fixes fonts subpage flakiness in search. #23064

Merged
merged 1 commit into from
Apr 15, 2024
Merged

[Settings] Fixes fonts subpage flakiness in search. #23064

merged 1 commit into from
Apr 15, 2024

Conversation

mkarolin
Copy link
Collaborator

Resolves brave/brave-browser#37555

Submitter Checklist:

  • I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
  • 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 wiki
    • npm run presubmit wiki, 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:

@mkarolin mkarolin self-assigned this Apr 13, 2024
@github-actions github-actions bot added the CI/storybook-url Deploy storybook and provide a unique URL for each build label Apr 13, 2024
@brave-builds
Copy link
Collaborator

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

simonhong
simonhong reviewed Apr 13, 2024
View reviewed changes
browser/resources/settings/brave_overrides/people_page.ts
@@ -50,6 +50,14 @@ RegisterPolymerTemplateModifications({
} else {
console.error('[Brave Settings Overrides] People Page cannot find syncSetup/advanced template')
}
const syncSetupPageContent = templateContent.querySelector(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q) It's also leftover and could be deleted as we have our own sync page?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we don't seem to be using this page. It seems to have something to do with the Privacy Guide, which we disable anyway. When using search in settings, if not removed, this template causes an exception because it can't find the trigger control for the page.

browser/resources/settings/brave_overrides/appearance_page.ts
@@ -126,6 +126,13 @@ RegisterPolymerTemplateModifications({
} else {
customizeFontsSubpageTrigger.remove()
}
const customizeFontsTemplate = templateContent.querySelector(
'template[is=dom-if][route-path="/fonts"]')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

Suggested change
'template[is=dom-if][route-path="/fonts"]')
'template[is=dom-if][route-path="/braveContents/fonts"]')

browser/resources/settings/brave_routes.ts
if (r.FONTS) {
delete r.FONTS
}
r.FONTS = r.BRAVE_CONTENT.createChild('/fonts');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about having brave://settings/braveContent/fonts instead of brave://settings/fonts?

Suggested change
r.FONTS = r.BRAVE_CONTENT.createChild('/fonts');
r.FONTS = r.BRAVE_CONTENT.createChild('/braveContent/fonts');

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally have no objections. The only thing I wonder if people who are used to going to brave://settings/fonts would be unhappy with the URL change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, i see. Using current one would be helpful for many users.

simonhong
simonhong approved these changes Apr 13, 2024
View reviewed changes
Copy link
Member

@simonhong simonhong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

++

@mkarolin mkarolin merged commit 2e9203f into master Apr 15, 2024
18 checks passed
@mkarolin mkarolin deleted the maxk-fix-fonts-subpage branch April 15, 2024 13:36
@github-actions github-actions bot added this to the 1.67.x - Nightly milestone Apr 15, 2024
brave-builds added a commit that referenced this pull request Apr 15, 2024
@brave-builds
Uplift of #23064 (squashed) to beta
9ed5b40
brave-builds added a commit that referenced this pull request Apr 15, 2024
@brave-builds
Uplift of #23064 (squashed) to release
27a4b94
This was referenced Apr 15, 2024
Merged
Merged
@kjozwiak
Copy link
Member

Verification PASSED on Win 11 x64 using the following build(s):

Brave | 1.67.12 Chromium: 124.0.6367.29 (Official Build) nightly (64-bit)
-- | --
Revision | b0d1dace522b9609178a71acf3bbc54515c4b8c2
OS | Windows 11 Version 23H2 (Build 22631.3447)

Using 1.67.6 Chromium: 124.0.6367.29 and the STR/Cases outlined via brave/brave-browser#37555 (comment), reproduced the original issue by visiting brave://settings/fonts?search=fonts as per the following:

reproduced

Using the same STR/Cases as mentioned above, went through the following:

  • upgraded from 1.67.6 Chromium: 124.0.6367.29 -> 1.67.12 Chromium: 124.0.6367.29 and ensured that brave://settings/fonts?search=fonts was loading without any issues
  • ensured that you can load brave://settings/fonts via the omnibox without any issues/failures
  • ensured that clicking through brave://settings -> Content -> Customise fonts works without any issues/failures

fixed

kjozwiak pushed a commit that referenced this pull request Apr 15, 2024
@brave-builds
[Settings] Fixes fonts subpage flakiness in search. (uplift to 1.66.x) (
3911989 
#23079)

Uplift of #23064 (squashed) to beta
kjozwiak pushed a commit that referenced this pull request Apr 15, 2024
@brave-builds
[Settings] Fixes fonts subpage flakiness in search. (uplift to 1.65.x) (
6be4d62 
#23080)

Uplift of #23064 (squashed) to release
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simonhong simonhong simonhong approved these changes

@sangwoo108 sangwoo108 Awaiting requested review from sangwoo108

Assignees

@mkarolin mkarolin

Labels
CI/storybook-url Deploy storybook and provide a unique URL for each build
Projects
None yet
Milestone
1.67.x - Release
4 participants
@mkarolin @brave-builds @kjozwiak @simonhong