-
Notifications
You must be signed in to change notification settings - Fork 879
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
Fixed link in wdb infobar is not clickable in some situation #17121
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
// copied from WIP | ||
// PR(https://chromium-review.googlesource.com/c/chromium/src/+/3934027) to | ||
// skip layout when it doesn't need layout. Layout() happens between | ||
// OnMousePressed() and OnMouseReleased(). As Layout() deletes all children, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoa, this must have been hard to find out! Well spotted!
Layout() seems to be called whenever mouse hovers and exits the LinkFragment. And it's caused by SetFontList() even it already has a font list with desired style(underlined), which seems to be a bug. Resetting font list triggers full cycle of layout from the top of browser view tree. So following code seems to make it not to relayout everyting.
void LinkFragment::RecalculateFont() {
// Check whether any link fragment should be underlined.
bool should_be_underlined = IsUnderlined();
for (LinkFragment* current_fragment = next_fragment_;
!should_be_underlined && current_fragment != this;
current_fragment = current_fragment->next_fragment_) {
should_be_underlined = current_fragment->IsUnderlied();
}
// If the style differs from the current one, update.
if ((font_list().GetFontStyle() & gfx::Font::UNDERLINE) !=
should_be_underlined) {
auto MaybeUpdateStyle = [should_be_underlined](LinkFragment* fragment) {
const int style = fragment->font_list().GetFontStyle();
const int intended_style = should_be_underlined
? (style | gfx::Font::UNDERLINE)
: (style & ~gfx::Font::UNDERLINE);
+ if (style == intended_style)
+ return;
fragment->Label::SetFontList(
fragment->font_list().DeriveWithStyle(intended_style));
fragment->SchedulePaint();
};
And a potential problem with the current code could be when we set a new font to a fragment, then it won't be laid out without explictly changing the size of the fragment. But now that this is not our case, I'm okay with the current fix. It's up to you! But could you update comments here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for digging more. I added more comments about it and let's wait fixing from upstream.
One thing I'm still curious thing is this doesn't happen when StyleLable is not multilined.
Needs to investigate more later :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesn't happen when StyleLable is not multilined.
Yeah, there seems to be multiple LinkFragments, and it doesn't look working well :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see. it's LinkFragment
issue.
d52eaab
to
5f325a7
Compare
Set |
fix brave/brave-browser#28139
Resolves
Submitter Checklist:
QA/Yes
orQA/No
;release-notes/include
orrelease-notes/exclude
;OS/...
) to the associated issuenpm run test -- brave_browser_tests
,npm run test -- brave_unit_tests
wikinpm run lint
,npm run presubmit
wiki,npm run gn_check
,npm run tslint
git rebase master
(if needed)Reviewer Checklist:
gn
After-merge Checklist:
changes has landed on
Test Plan: