Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2763373002:
Overlay scrollbars flush with window edge (Closed)
DescriptionOverlay scrollbars flush with window edge
In this patch, remove right stroke for vertical scrollbar, remove
bottom stroke for horizontal scrollbar and flip the scrollbar canvas to
remove left stroke for left vertical scrollbar.
BUG=669673
Review-Url: https://codereview.chromium.org/2763373002
Cr-Commit-Position: refs/heads/master@{#460565}
Committed: https://chromium.googlesource.com/chromium/src/+/609da81d151f9eb3bcc94852f10e4ebb649a5bed
Patch Set 1 #
Total comments: 3
Patch Set 2 : only flush 1 side #
Total comments: 2
Patch Set 3 : bokan comments addressed #
Total comments: 1
Patch Set 4 : bokan comment addressed #
Total comments: 5
Patch Set 5 : Evan comment addressed #Patch Set 6 : flip canvas for left scrollbar #
Total comments: 2
Patch Set 7 : evan and bokan comments addressed #
Total comments: 1
Messages
Total messages: 34 (14 generated)
Patchset #1 (id:1) has been deleted
chaopeng@chromium.org changed reviewers: + bokan@chromium.org
PTAL. Thank you.
https://codereview.chromium.org/2763373002/diff/20001/ui/native_theme/native_... File ui/native_theme/native_theme.h (right): https://codereview.chromium.org/2763373002/diff/20001/ui/native_theme/native_... ui/native_theme/native_theme.h:205: bool isVerticalScrollbar; You shouldn't need to pass this param from Blink, PaintScrollbarThumb is called form NativeThemeBase::paint with either kScrollbarHorizontalThumb or kScrollbarVerticalThumb so you can decide there. https://codereview.chromium.org/2763373002/diff/20001/ui/native_theme/native_... ui/native_theme/native_theme.h:207: bool hitStart; We should avoid repainting. A simpler solution would be to expand the minimum and maximum positions of the track by 1 pixel. i.e. the track should start at -1. Have you tried that? It looks like we already have something like this in the scrollbar "margin" though currently in Blink that appears to be used for both dimensions, you'd need to split it up into separate margins for scroll direction and perpendicular. On the compositor that gets plumbed through as TrackStart() which seems like what we're looking for, you'd just need to set a -1 margin. https://codereview.chromium.org/2763373002/diff/20001/ui/native_theme/native_... File ui/native_theme/native_theme_aura.cc (right): https://codereview.chromium.org/2763373002/diff/20001/ui/native_theme/native_... ui/native_theme/native_theme_aura.cc:240: #if defined(OS_ANDROID) This file is only compiled for Aura platforms so Chrome OS, Linux, Windows. You shouldn't need an ifdef here.
On 2017/03/22 20:07:26, bokan wrote: > https://codereview.chromium.org/2763373002/diff/20001/ui/native_theme/native_... > File ui/native_theme/native_theme.h (right): > > https://codereview.chromium.org/2763373002/diff/20001/ui/native_theme/native_... > ui/native_theme/native_theme.h:205: bool isVerticalScrollbar; > You shouldn't need to pass this param from Blink, PaintScrollbarThumb is called > form NativeThemeBase::paint with either kScrollbarHorizontalThumb or > kScrollbarVerticalThumb so you can decide there. > > https://codereview.chromium.org/2763373002/diff/20001/ui/native_theme/native_... > ui/native_theme/native_theme.h:207: bool hitStart; > We should avoid repainting. A simpler solution would be to expand the minimum > and maximum positions of the track by 1 pixel. i.e. the track should start at > -1. Have you tried that? > > It looks like we already have something like this in the scrollbar "margin" > though currently in Blink that appears to be used for both dimensions, you'd > need to split it up into separate margins for scroll direction and > perpendicular. On the compositor that gets plumbed through as TrackStart() which > seems like what we're looking for, you'd just need to set a -1 margin. > > https://codereview.chromium.org/2763373002/diff/20001/ui/native_theme/native_... > File ui/native_theme/native_theme_aura.cc (right): > > https://codereview.chromium.org/2763373002/diff/20001/ui/native_theme/native_... > ui/native_theme/native_theme_aura.cc:240: #if defined(OS_ANDROID) > This file is only compiled for Aura platforms so Chrome OS, Linux, Windows. You > shouldn't need an ifdef here. FYI: I had an in-progress patch that removed the border on one side: https://codereview.chromium.org/2572713005/
Description was changed from ========== Overlay scrollbars flush with window edge In this patch, we pass the scrollbar orientation and hit start/end to NativeThemeAura to make it paint flush. Also we will redraw scrollbar when it hit start/end state changed. BUG=669673 ========== to ========== Overlay scrollbars flush with window edge In this patch, we pass the scrollbar orientation to NativeThemeAura to make it paint flush. BUG=669673 ==========
Updated, PTAL. Thank you.
https://codereview.chromium.org/2763373002/diff/40001/ui/native_theme/native_... File ui/native_theme/native_theme_aura.cc (right): https://codereview.chromium.org/2763373002/diff/40001/ui/native_theme/native_... ui/native_theme/native_theme_aura.cc:252: // Flush the stroke when it close to border. A better comment would be "The edge to which the scrollbar is attached shouldn't have a border" https://codereview.chromium.org/2763373002/diff/40001/ui/native_theme/native_... ui/native_theme/native_theme_aura.cc:263: stroke_rect.Inset(leftStrokeWidth / 2.f, topStrokeWidth / 2.f, The drawRect call below is the one that draws the outline. Since we're going to paint over it with the fill anyway, there's no need to change the inset above (in fact I think this moves the stroke out a little). You should only change the inset after the drawRect call below so that the fill paints over that one side. (as I did in my patch). I also think it'd be clearer to just put the thumb_rect.Inset call itself inside the if statements and vary which parameter gets the 0 based on vertical|horizontal|isLeftVertical, rather than define left, right, top, bottom stroke widths.
Updated, PTAL. Thank you.
bokan@chromium.org changed reviewers: + estade@chromium.org
lgtm % comment issue +estade@ for OWNER. Evan, do we do pixel tests to test platform specific look or do we just rely on native theme painting to be visible enough that it'll be caught on use? https://codereview.chromium.org/2763373002/diff/60001/ui/native_theme/native_... File ui/native_theme/native_theme_aura.cc (right): https://codereview.chromium.org/2763373002/diff/60001/ui/native_theme/native_... ui/native_theme/native_theme_aura.cc:253: // The edge to which the scrollbar is attached shouldn't have a border. Please keep the comment this is currently replacing as well.
No, we don't have pixel tests. https://codereview.chromium.org/2763373002/diff/80001/chrome/browser/ui/libgt... File chrome/browser/ui/libgtkui/native_theme_gtk3.cc (right): https://codereview.chromium.org/2763373002/diff/80001/chrome/browser/ui/libgt... chrome/browser/ui/libgtkui/native_theme_gtk3.cc:483: const NativeTheme::ScrollbarThumbExtraParams&) const { nit: we don't leave off the parameter name even when it's unused (here and elsewhere) https://codereview.chromium.org/2763373002/diff/80001/ui/native_theme/native_... File ui/native_theme/native_theme.h (right): https://codereview.chromium.org/2763373002/diff/80001/ui/native_theme/native_... ui/native_theme/native_theme.h:205: bool isLeftVerticalScrollbar; nit: is_left_vertical_scrollbar but also, it seems like we should instead be splitting the part into two (one for left vertical bar and one for right vertical bar), or making this param a generic rtl parameter. https://codereview.chromium.org/2763373002/diff/80001/ui/native_theme/native_... File ui/native_theme/native_theme_aura.cc (right): https://codereview.chromium.org/2763373002/diff/80001/ui/native_theme/native_... ui/native_theme/native_theme_aura.cc:255: if (part == NativeTheme::kScrollbarVerticalThumb) { nit: this can be clearer/less nested, e.g. gfx::Insets insets(kStrokeWidth); if (part == Horizontal) insets -= gfx::Insets(0,0,kStrokeWidth,0); else if (isLeftVertical) insets -= gfx::Insets(0,kStrokeWidth,0,0); else insets -= gfx::Insets(0,0,0,kStrokeWidth); thumb_rect.Inset(insets);
Updated. PTAL. Thank you. https://codereview.chromium.org/2763373002/diff/80001/ui/native_theme/native_... File ui/native_theme/native_theme.h (right): https://codereview.chromium.org/2763373002/diff/80001/ui/native_theme/native_... ui/native_theme/native_theme.h:205: bool isLeftVerticalScrollbar; On 2017/03/28 00:01:56, Evan Stade wrote: > nit: is_left_vertical_scrollbar > > but also, it seems like we should instead be splitting the part into two (one > for left vertical bar and one for right vertical bar), or making this param a > generic rtl parameter. Sorry, I don't get this one. Why we need is right vertical scrollbar? We use IsLeftSideVerticalScrollbar everywhere like this. https://cs.chromium.org/chromium/src/cc/input/scrollbar.h?q=scrollba&l=26
The CQ bit was checked by chaopeng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2763373002/diff/80001/ui/native_theme/native_... File ui/native_theme/native_theme.h (right): https://codereview.chromium.org/2763373002/diff/80001/ui/native_theme/native_... ui/native_theme/native_theme.h:205: bool isLeftVerticalScrollbar; On 2017/03/28 02:58:40, chaopeng wrote: > On 2017/03/28 00:01:56, Evan Stade wrote: > > nit: is_left_vertical_scrollbar > > > > but also, it seems like we should instead be splitting the part into two (one > > for left vertical bar and one for right vertical bar), or making this param a > > generic rtl parameter. > > Sorry, I don't get this one. Why we need is right vertical scrollbar? We use > IsLeftSideVerticalScrollbar everywhere like this. > https://cs.chromium.org/chromium/src/cc/input/scrollbar.h?q=scrollba&l=26 I meant that the Part enum should have different values.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Overlay scrollbars flush with window edge In this patch, we pass the scrollbar orientation to NativeThemeAura to make it paint flush. BUG=669673 ========== to ========== Overlay scrollbars flush with window edge In this patch, remove right stroke for vertical scrollbar, remove bottom stroke for horizontal scrollbar and flip the scrollbar canvas to remove left stroke for left vertical scrollbar. BUG=669673 ==========
Change to flip canvas to remove left border for left scrollbar. PTAL. Thank you.
nice, lgtm. @bokan, wdyt? https://codereview.chromium.org/2763373002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scroll/ScrollbarThemeOverlay.cpp (right): https://codereview.chromium.org/2763373002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scroll/ScrollbarThemeOverlay.cpp:208: // Filp the canvas if it is left vertical scrollbar. nit: s/Filp/Horizontally flip/
Much cleaner, thanks. lgtm % same nit in other file https://codereview.chromium.org/2763373002/diff/120001/ui/native_theme/native... File ui/native_theme/native_theme_aura.cc (right): https://codereview.chromium.org/2763373002/diff/120001/ui/native_theme/native... ui/native_theme/native_theme_aura.cc:256: // For left vertical scrollbar, we will filp the canvas in Nit: same typo here.
chaopeng@chromium.org changed reviewers: + jbroman@chromium.org
jbroman@chromium.org: Please review changes in blink/platform Thank you.
rs lgtm https://codereview.chromium.org/2763373002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scroll/ScrollbarThemeOverlay.cpp (right): https://codereview.chromium.org/2763373002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scroll/ScrollbarThemeOverlay.cpp:208: // Horizontally flip the canvas if it is left vertical scrollbar. Is the scrollbar guaranteed to be horizontally symmetrical? Not everything should be visually flipped for RTL. (e.g. if we drew shadows, they would seem to look wrong with this flip)
On 2017/03/29 20:51:35, jbroman wrote: > rs lgtm > > https://codereview.chromium.org/2763373002/diff/140001/third_party/WebKit/Sou... > File third_party/WebKit/Source/platform/scroll/ScrollbarThemeOverlay.cpp > (right): > > https://codereview.chromium.org/2763373002/diff/140001/third_party/WebKit/Sou... > third_party/WebKit/Source/platform/scroll/ScrollbarThemeOverlay.cpp:208: // > Horizontally flip the canvas if it is left vertical scrollbar. > Is the scrollbar guaranteed to be horizontally symmetrical? Not everything > should be visually flipped for RTL. (e.g. if we drew shadows, they would seem to > look wrong with this flip) The GraphicsContext context is from scrollbar layer. eg https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/co...
The CQ bit was checked by chaopeng@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org, estade@chromium.org Link to the patchset: https://codereview.chromium.org/2763373002/#ps140001 (title: "evan and bokan comments addressed")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1490821140998650,
"parent_rev": "0a5a46198c69904bc7d92d27a2b1ab654a52ef1b", "commit_rev":
"609da81d151f9eb3bcc94852f10e4ebb649a5bed"}
Message was sent while issue was closed.
Description was changed from ========== Overlay scrollbars flush with window edge In this patch, remove right stroke for vertical scrollbar, remove bottom stroke for horizontal scrollbar and flip the scrollbar canvas to remove left stroke for left vertical scrollbar. BUG=669673 ========== to ========== Overlay scrollbars flush with window edge In this patch, remove right stroke for vertical scrollbar, remove bottom stroke for horizontal scrollbar and flip the scrollbar canvas to remove left stroke for left vertical scrollbar. BUG=669673 Review-Url: https://codereview.chromium.org/2763373002 Cr-Commit-Position: refs/heads/master@{#460565} Committed: https://chromium.googlesource.com/chromium/src/+/609da81d151f9eb3bcc94852f10e... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:140001) as https://chromium.googlesource.com/chromium/src/+/609da81d151f9eb3bcc94852f10e...
Message was sent while issue was closed.
On 2017/03/29 20:58:16, chaopeng wrote: > On 2017/03/29 20:51:35, jbroman wrote: > > rs lgtm > > > > > https://codereview.chromium.org/2763373002/diff/140001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/platform/scroll/ScrollbarThemeOverlay.cpp > > (right): > > > > > https://codereview.chromium.org/2763373002/diff/140001/third_party/WebKit/Sou... > > third_party/WebKit/Source/platform/scroll/ScrollbarThemeOverlay.cpp:208: // > > Horizontally flip the canvas if it is left vertical scrollbar. > > Is the scrollbar guaranteed to be horizontally symmetrical? Not everything > > should be visually flipped for RTL. (e.g. if we drew shadows, they would seem > to > > look wrong with this flip) > > The GraphicsContext context is from scrollbar layer. eg > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/co... I think Jeremy's point was more that this wouldn't work if we one day decided we wanted a shadow on the scrollbar. While that's a valid point, scrollbars already have plenty of symmetry assumptions baked in - e.g. Aura scroll buttons do exactly this. IMO, it's unlikely we'd make scrollbars asymmetric so keeping the theme API simple feels like a better choice to me.
Message was sent while issue was closed.
On 2017/03/30 11:57:19, bokan wrote: > On 2017/03/29 20:58:16, chaopeng wrote: > > On 2017/03/29 20:51:35, jbroman wrote: > > > rs lgtm > > > > > > > > > https://codereview.chromium.org/2763373002/diff/140001/third_party/WebKit/Sou... > > > File third_party/WebKit/Source/platform/scroll/ScrollbarThemeOverlay.cpp > > > (right): > > > > > > > > > https://codereview.chromium.org/2763373002/diff/140001/third_party/WebKit/Sou... > > > third_party/WebKit/Source/platform/scroll/ScrollbarThemeOverlay.cpp:208: // > > > Horizontally flip the canvas if it is left vertical scrollbar. > > > Is the scrollbar guaranteed to be horizontally symmetrical? Not everything > > > should be visually flipped for RTL. (e.g. if we drew shadows, they would > seem > > to > > > look wrong with this flip) > > > > The GraphicsContext context is from scrollbar layer. eg > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/co... > > I think Jeremy's point was more that this wouldn't work if we one day decided we > wanted a shadow on the scrollbar. While that's a valid point, scrollbars already > have plenty of symmetry assumptions baked in - e.g. Aura scroll buttons do > exactly this. IMO, it's unlikely we'd make scrollbars asymmetric so keeping the > theme API simple feels like a better choice to me. you mean an asymmetric shadow on the scrollbar? Wouldn't a symmetric shadow work alright? All the shadows I know of in our UI are horizontally symmetric if not vertically. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
