Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1849703004:
Handle bubble title resizing (growth) by reworking title layout (Closed)
|
Created:
4 years, 8 months ago by Evan Stade Modified:
4 years, 8 months ago CC:
chromium-reviews, tfarina, msw+watch_chromium.org, groby+bubble_chromium.org, hcarmona+bubble_chromium.org, rouslan+bubble_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionHandle bubble title resizing (growth) by reworking title layout
The easiest way to demonstrate the bug is to shrink the browser window
horizontally until the title starts to wrap, then drag it larger again.
Without this patch, the title doesn't unwrap.
I also noticed that one of the labels was jiggling in the collected cookies view during resize; left-aligning the text fixes that.
BUG=598985
TBR=sky@chromium.org
Committed: https://crrev.com/63302b696824ab46a212de5bc1a1cf80b678a88a
Cr-Commit-Position: refs/heads/master@{#384722}
Patch Set 1 #Patch Set 2 : dbg code for showing a bubble with an icon #Patch Set 3 : tweaks #
Total comments: 11
Patch Set 4 : msw feedback #Patch Set 5 : split out #Patch Set 6 : invisible title #Patch Set 7 : one more change #
Messages
Total messages: 40 (19 generated)
Description was changed from ========== WIP - handle bubble title resizing (growth) by reworking title layout The easiest way to demonstrate the bug is to shrink the browser window horizontally until the title starts to wrap, then drag it larger again. Without this patch, the title doesn't unwrap. BUG= ========== to ========== Handle bubble title resizing (growth) by reworking title layout The easiest way to demonstrate the bug is to shrink the browser window horizontally until the title starts to wrap, then drag it larger again. Without this patch, the title doesn't unwrap. I looked at removing the icon layout altogether but I found that it's still used in at least one place: the extension disabled warning. While I was at it, I made some adjustments to that bubble's layout code to improve its appearance. BUG=598985 ==========
estade@chromium.org changed reviewers: + msw@chromium.org
Description was changed from ========== Handle bubble title resizing (growth) by reworking title layout The easiest way to demonstrate the bug is to shrink the browser window horizontally until the title starts to wrap, then drag it larger again. Without this patch, the title doesn't unwrap. I looked at removing the icon layout altogether but I found that it's still used in at least one place: the extension disabled warning. While I was at it, I made some adjustments to that bubble's layout code to improve its appearance. BUG=598985 ========== to ========== Handle bubble title resizing (growth) by reworking title layout The easiest way to demonstrate the bug is to shrink the browser window horizontally until the title starts to wrap, then drag it larger again. Without this patch, the title doesn't unwrap. I looked at removing the icon layout altogether but I found that it's still used in at least one place: the extension disabled warning. While I was at it, I made some adjustments to that bubble's layout code to improve its appearance. I also noticed that one of the labels was jiggling in the collected cookies view during resize; left-aligning the text fixes that. BUG=598985 ==========
estade@chromium.org changed reviewers: + sky@chromium.org
estade@chromium.org changed reviewers: - sky@chromium.org
https://codereview.chromium.org/1849703004/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/global_error_bubble_view.cc (left): https://codereview.chromium.org/1849703004/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/global_error_bubble_view.cc:75: set_margins(gfx::Insets(0, kBubblePadding, kBubblePadding, kBubblePadding)); Please add before/after screenshots; maybe split this out if we intend to merge this cl back to a branch. https://codereview.chromium.org/1849703004/diff/40001/ui/views/bubble/bubble_... File ui/views/bubble/bubble_frame_view.cc (right): https://codereview.chromium.org/1849703004/diff/40001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view.cc:305: if (!title_->text().empty()) { Some WidgetDelegate may return a non-empty GetWindowTitle, but false for ShouldShowWindowTitle, which was previously handled via set_collapse_when_hidden(true) and pref size checking. Checking for a non-empty string might regress layout in those cases; maybe check title_->visible() too? https://codereview.chromium.org/1849703004/diff/40001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view.cc:307: padding = title_margins_.left(); Hmm, re-using the title margins (for outside the icon and label) as the inter-view spacing might be okay, but what is the actual px difference? Isn't BubbleDialogDelegateView's use of kPanelHorizMargin (13) here much bigger than the current value (5)? The (current?) global error bubble is ugly, but how does this change make it look? https://bugs.chromium.org/p/chromium/issues/detail?id=449737#c19 https://codereview.chromium.org/1849703004/diff/40001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view.cc:311: title_->SizeToFit(std::max(1, close_->x() - title_label_x)); q: why 1 over 0? (for my enlightenment) https://codereview.chromium.org/1849703004/diff/40001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view.cc:319: bounds.set_width(title_->bounds().right() - title_icon_->x()); nit: use bounds.x() instead of title_icon_->x()?
Description was changed from ========== Handle bubble title resizing (growth) by reworking title layout The easiest way to demonstrate the bug is to shrink the browser window horizontally until the title starts to wrap, then drag it larger again. Without this patch, the title doesn't unwrap. I looked at removing the icon layout altogether but I found that it's still used in at least one place: the extension disabled warning. While I was at it, I made some adjustments to that bubble's layout code to improve its appearance. I also noticed that one of the labels was jiggling in the collected cookies view during resize; left-aligning the text fixes that. BUG=598985 ========== to ========== Handle bubble title resizing (growth) by reworking title layout The easiest way to demonstrate the bug is to shrink the browser window horizontally until the title starts to wrap, then drag it larger again. Without this patch, the title doesn't unwrap. I also noticed that one of the labels was jiggling in the collected cookies view during resize; left-aligning the text fixes that. BUG=598985 ==========
https://codereview.chromium.org/1849703004/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/global_error_bubble_view.cc (left): https://codereview.chromium.org/1849703004/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/global_error_bubble_view.cc:75: set_margins(gfx::Insets(0, kBubblePadding, kBubblePadding, kBubblePadding)); On 2016/03/31 19:18:30, msw wrote: > Please add before/after screenshots; maybe split this out if we intend to merge > this cl back to a branch. sure, done https://codereview.chromium.org/1849703004/diff/40001/ui/views/bubble/bubble_... File ui/views/bubble/bubble_frame_view.cc (right): https://codereview.chromium.org/1849703004/diff/40001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view.cc:307: padding = title_margins_.left(); On 2016/03/31 19:18:31, msw wrote: > Hmm, re-using the title margins (for outside the icon and label) as the > inter-view spacing might be okay, but what is the actual px difference? > Isn't BubbleDialogDelegateView's use of kPanelHorizMargin (13) here much bigger > than the current value (5)? > The (current?) global error bubble is ugly, but how does this change make it > look? https://bugs.chromium.org/p/chromium/issues/detail?id=449737#c19 This screenshot captures both visual improvements/simplifications: https://screenshot.googleplex.com/yv3MVAqxAHB.png https://codereview.chromium.org/1849703004/diff/40001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view.cc:311: title_->SizeToFit(std::max(1, close_->x() - title_label_x)); On 2016/03/31 19:18:31, msw wrote: > q: why 1 over 0? (for my enlightenment) because I'm afraid of crazy things like infinite loops when you try to do the impossible (squeeze text into zero horizontal space) https://codereview.chromium.org/1849703004/diff/40001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view.cc:319: bounds.set_width(title_->bounds().right() - title_icon_->x()); On 2016/03/31 19:18:31, msw wrote: > nit: use bounds.x() instead of title_icon_->x()? Done.
The CQ bit was checked by estade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1849703004/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1849703004/80001
https://codereview.chromium.org/1849703004/diff/40001/ui/views/bubble/bubble_... File ui/views/bubble/bubble_frame_view.cc (right): https://codereview.chromium.org/1849703004/diff/40001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view.cc:305: if (!title_->text().empty()) { On 2016/03/31 19:18:31, msw wrote: > Some WidgetDelegate may return a non-empty GetWindowTitle, but false for > ShouldShowWindowTitle, which was previously handled via > set_collapse_when_hidden(true) and pref size checking. Checking for a non-empty > string might regress layout in those cases; maybe check title_->visible() too? Ping
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/1849703004/diff/40001/ui/views/bubble/bubble_... File ui/views/bubble/bubble_frame_view.cc (right): https://codereview.chromium.org/1849703004/diff/40001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view.cc:305: if (!title_->text().empty()) { On 2016/03/31 19:18:31, msw wrote: > Some WidgetDelegate may return a non-empty GetWindowTitle, but false for > ShouldShowWindowTitle, which was previously handled via > set_collapse_when_hidden(true) and pref size checking. Checking for a non-empty > string might regress layout in those cases; maybe check title_->visible() too? Done. Good catch.
The CQ bit was checked by estade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1849703004/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1849703004/100001
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
A unit test caught a regression --- I had to make one more change since title_ no longer has its position set unconditionally. PTAL
The CQ bit was checked by estade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1849703004/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1849703004/120001
still lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by estade@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1849703004/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1849703004/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Description was changed from ========== Handle bubble title resizing (growth) by reworking title layout The easiest way to demonstrate the bug is to shrink the browser window horizontally until the title starts to wrap, then drag it larger again. Without this patch, the title doesn't unwrap. I also noticed that one of the labels was jiggling in the collected cookies view during resize; left-aligning the text fixes that. BUG=598985 ========== to ========== Handle bubble title resizing (growth) by reworking title layout The easiest way to demonstrate the bug is to shrink the browser window horizontally until the title starts to wrap, then drag it larger again. Without this patch, the title doesn't unwrap. I also noticed that one of the labels was jiggling in the collected cookies view during resize; left-aligning the text fixes that. BUG=598985 TBR=sky@chromium.org ==========
estade@chromium.org changed reviewers: + sky@chromium.org
tbr sky for unit test logging improvement
The CQ bit was checked by estade@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1849703004/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1849703004/120001
Message was sent while issue was closed.
Description was changed from ========== Handle bubble title resizing (growth) by reworking title layout The easiest way to demonstrate the bug is to shrink the browser window horizontally until the title starts to wrap, then drag it larger again. Without this patch, the title doesn't unwrap. I also noticed that one of the labels was jiggling in the collected cookies view during resize; left-aligning the text fixes that. BUG=598985 TBR=sky@chromium.org ========== to ========== Handle bubble title resizing (growth) by reworking title layout The easiest way to demonstrate the bug is to shrink the browser window horizontally until the title starts to wrap, then drag it larger again. Without this patch, the title doesn't unwrap. I also noticed that one of the labels was jiggling in the collected cookies view during resize; left-aligning the text fixes that. BUG=598985 TBR=sky@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Handle bubble title resizing (growth) by reworking title layout The easiest way to demonstrate the bug is to shrink the browser window horizontally until the title starts to wrap, then drag it larger again. Without this patch, the title doesn't unwrap. I also noticed that one of the labels was jiggling in the collected cookies view during resize; left-aligning the text fixes that. BUG=598985 TBR=sky@chromium.org ========== to ========== Handle bubble title resizing (growth) by reworking title layout The easiest way to demonstrate the bug is to shrink the browser window horizontally until the title starts to wrap, then drag it larger again. Without this patch, the title doesn't unwrap. I also noticed that one of the labels was jiggling in the collected cookies view during resize; left-aligning the text fixes that. BUG=598985 TBR=sky@chromium.org Committed: https://crrev.com/63302b696824ab46a212de5bc1a1cf80b678a88a Cr-Commit-Position: refs/heads/master@{#384722} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/63302b696824ab46a212de5bc1a1cf80b678a88a Cr-Commit-Position: refs/heads/master@{#384722}
Message was sent while issue was closed.
LGTM |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
