Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1044043004:
Don't add an aura tooltip to bubble close buttons on Windows. (Closed)
|
Created:
5 years, 8 months ago by benwells Modified:
5 years, 8 months ago CC:
chromium-reviews, tfarina, alicet1, msw+watch_chromium.org, chrome-apps-syd-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDon't add an aura tooltip to bubble close buttons on Windows.
The Windows system automatically adds a tooltip to these buttons as the button returns HTCLOSE when nc hit tested. If we add the aura tooltip as well there are two tooltips shown.
BUG=471981
Committed: https://crrev.com/5c30d9f20cde79aff3ae8d90c8e1909e351bea44
Cr-Commit-Position: refs/heads/master@{#324183}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Use define #Messages
Total messages: 19 (3 generated)
benwells@chromium.org changed reviewers: + msw@chromium.org
msw@chromium.org changed reviewers: + dmazzoni@chromium.org, pkasting@chromium.org, sky@chromium.org
https://codereview.chromium.org/1044043004/diff/1/ui/views/bubble/bubble_fram... File ui/views/bubble/bubble_frame_view.cc (left): https://codereview.chromium.org/1044043004/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view.cc:129: return HTCLOSE; Hmm, do screen readers, software that assists the visually impaired, or 3rd party windowing enhancement software (WindowBlinds, etc.) rely on this? +CC Dominic, Scott, and Peter for any thoughts on this.
https://codereview.chromium.org/1044043004/diff/1/ui/views/bubble/bubble_fram... File ui/views/bubble/bubble_frame_view.cc (left): https://codereview.chromium.org/1044043004/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view.cc:129: return HTCLOSE; On 2015/03/31 16:45:30, msw wrote: > Hmm, do screen readers, software that assists the visually impaired, or 3rd > party windowing enhancement software (WindowBlinds, etc.) rely on this? > +CC Dominic, Scott, and Peter for any thoughts on this. Yes, they do call hit testing functions so at first glance this might be an issue. Can you think of an alternative fix, or do we need to dig into this some more to see how much of a problem it is?
https://codereview.chromium.org/1044043004/diff/1/ui/views/bubble/bubble_fram... File ui/views/bubble/bubble_frame_view.cc (left): https://codereview.chromium.org/1044043004/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view.cc:129: return HTCLOSE; On 2015/03/31 16:55:46, dmazzoni wrote: > On 2015/03/31 16:45:30, msw wrote: > > Hmm, do screen readers, software that assists the visually impaired, or 3rd > > party windowing enhancement software (WindowBlinds, etc.) rely on this? > > +CC Dominic, Scott, and Peter for any thoughts on this. > > Yes, they do call hit testing functions so at first glance this might be an > issue. > > Can you think of an alternative fix, or do we need to dig into this some more to > see how much of a problem it is? We could hide the other tooltip when the native one will be shown.
https://codereview.chromium.org/1044043004/diff/1/ui/views/bubble/bubble_fram... File ui/views/bubble/bubble_frame_view.cc (left): https://codereview.chromium.org/1044043004/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view.cc:129: return HTCLOSE; On 2015/03/31 16:57:22, msw wrote: > On 2015/03/31 16:55:46, dmazzoni wrote: > > On 2015/03/31 16:45:30, msw wrote: > > > Hmm, do screen readers, software that assists the visually impaired, or 3rd > > > party windowing enhancement software (WindowBlinds, etc.) rely on this? > > > +CC Dominic, Scott, and Peter for any thoughts on this. > > > > Yes, they do call hit testing functions so at first glance this might be an > > issue. > > > > Can you think of an alternative fix, or do we need to dig into this some more > to > > see how much of a problem it is? > > We could hide the other tooltip when the native one will be shown. I think that would look like adding an #if !defined(OS_WIN) around the SetToolTip call. I have a feeling this is the only place in Chrome that the native tooltip comes up. Do we have a11y problems with the aura tooltip? Meaning, in other places where we use the aura tooltip (like bookmarks, chrome://apps, and everywhere else that uses tooltips) are there a11y problems?
https://codereview.chromium.org/1044043004/diff/1/ui/views/bubble/bubble_fram... File ui/views/bubble/bubble_frame_view.cc (left): https://codereview.chromium.org/1044043004/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view.cc:129: return HTCLOSE; On 2015/03/31 22:06:57, benwells wrote: > On 2015/03/31 16:57:22, msw wrote: > > On 2015/03/31 16:55:46, dmazzoni wrote: > > > On 2015/03/31 16:45:30, msw wrote: > > > > Hmm, do screen readers, software that assists the visually impaired, or > 3rd > > > > party windowing enhancement software (WindowBlinds, etc.) rely on this? > > > > +CC Dominic, Scott, and Peter for any thoughts on this. > > > > > > Yes, they do call hit testing functions so at first glance this might be an > > > issue. > > > > > > Can you think of an alternative fix, or do we need to dig into this some > more > > to > > > see how much of a problem it is? > > > > We could hide the other tooltip when the native one will be shown. > > I think that would look like adding an #if !defined(OS_WIN) around the > SetToolTip call. > > I have a feeling this is the only place in Chrome that the native tooltip comes > up. > > Do we have a11y problems with the aura tooltip? Meaning, in other places where > we use the aura tooltip (like bookmarks, chrome://apps, and everywhere else that > uses tooltips) are there a11y problems? What do browser frames do? Those seem to show native tooltips for their caption buttons. Gating on OS_WIN (and desktop/ash_shell?) might be ok.
Since I was CCed: I think returning HTCLOSE is preferable, and in general if we have both native and Aura tooltips, we should probably try to prefer the native ones.
On 2015/04/01 06:18:11, Peter Kasting wrote: > Since I was CCed: I think returning HTCLOSE is preferable, and in general if we > have both native and Aura tooltips, we should probably try to prefer the native > ones. I think we use aura tooltips everywhere on Windows now, except close buttons / other caption buttons. Using native tooltips everywhere would probably be easy, but for some reason we have a different look and feel. E.g. on windows 8 there is no shadow on the native one, and they are beige-ish, while the aura one has a shadow and is grey. I'd assumed using the aura style was deliberate. Anyway, I agree HTCLOSE should be returned if it affects a11y. I can't see any way to return HTCLOSE and not have the native tooltip, so that means we should not add the tooltip in these cases. msw: I can't see any easy way to gate here on ash shell, do you know of one? Alternatively we could just prevent the tooltip being shown somewhere in the aura tooltip implementation if the control is HTCLOSE (or minimize, maximize). For M43 I'll probably just revert the change that added the tooltip.
Does the Windows native hint also appear in Ash shell (metro / windows mode)? If so, just excluding OS_WIN should give you what you want. Otherwise, excluding OS_WIN for now (and losing the new tooltip on ash shell on Win) seems okay. The ui layer can't access the chrome::HostDesktopType to check for Ash, and I'm not sure what conditions to check otherwise (maybe you can disambiguate between DesktopNativeWidgetAura versus NativeWidgetAura (but even on the desktop, child windows like these dialogs might use NativeWidgetAura...).
OK, here it is using a define https://codereview.chromium.org/1044043004/diff/1/ui/views/bubble/bubble_fram... File ui/views/bubble/bubble_frame_view.cc (left): https://codereview.chromium.org/1044043004/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view.cc:129: return HTCLOSE; On 2015/03/31 22:11:26, msw wrote: > On 2015/03/31 22:06:57, benwells wrote: > > On 2015/03/31 16:57:22, msw wrote: > > > On 2015/03/31 16:55:46, dmazzoni wrote: > > > > On 2015/03/31 16:45:30, msw wrote: > > > > > Hmm, do screen readers, software that assists the visually impaired, or > > 3rd > > > > > party windowing enhancement software (WindowBlinds, etc.) rely on this? > > > > > +CC Dominic, Scott, and Peter for any thoughts on this. > > > > > > > > Yes, they do call hit testing functions so at first glance this might be > an > > > > issue. > > > > > > > > Can you think of an alternative fix, or do we need to dig into this some > > more > > > to > > > > see how much of a problem it is? > > > > > > We could hide the other tooltip when the native one will be shown. > > > > I think that would look like adding an #if !defined(OS_WIN) around the > > SetToolTip call. > > > > I have a feeling this is the only place in Chrome that the native tooltip > comes > > up. > > > > Do we have a11y problems with the aura tooltip? Meaning, in other places where > > we use the aura tooltip (like bookmarks, chrome://apps, and everywhere else > that > > uses tooltips) are there a11y problems? > > What do browser frames do? Those seem to show native tooltips for their caption > buttons. Gating on OS_WIN (and desktop/ash_shell?) might be ok. Ah, yes the frames use the native hint as well. I'd say that is the same mechanism: windows automatically adding a native tooltop based on hit testing. That doesn't seem so bad as the frame looks like a part of windows, so having the native tooltip is ok. But these dialog close buttons don't feel like part of windows so it would be nice if they were consistent with all the other tooltips. But it probably doesn't matter that much. I'll look at (a) can the native tooltip be suppressed while still returning HTCLOSE, and if that doesn't work (b) I'll add some conditional around setting the tooltip.
On 2015/04/08 00:19:13, benwells wrote: > OK, here it is using a define > > https://codereview.chromium.org/1044043004/diff/1/ui/views/bubble/bubble_fram... > File ui/views/bubble/bubble_frame_view.cc (left): > > https://codereview.chromium.org/1044043004/diff/1/ui/views/bubble/bubble_fram... > ui/views/bubble/bubble_frame_view.cc:129: return HTCLOSE; > On 2015/03/31 22:11:26, msw wrote: > > On 2015/03/31 22:06:57, benwells wrote: > > > On 2015/03/31 16:57:22, msw wrote: > > > > On 2015/03/31 16:55:46, dmazzoni wrote: > > > > > On 2015/03/31 16:45:30, msw wrote: > > > > > > Hmm, do screen readers, software that assists the visually impaired, > or > > > 3rd > > > > > > party windowing enhancement software (WindowBlinds, etc.) rely on > this? > > > > > > +CC Dominic, Scott, and Peter for any thoughts on this. > > > > > > > > > > Yes, they do call hit testing functions so at first glance this might be > > an > > > > > issue. > > > > > > > > > > Can you think of an alternative fix, or do we need to dig into this some > > > more > > > > to > > > > > see how much of a problem it is? > > > > > > > > We could hide the other tooltip when the native one will be shown. > > > > > > I think that would look like adding an #if !defined(OS_WIN) around the > > > SetToolTip call. > > > > > > I have a feeling this is the only place in Chrome that the native tooltip > > comes > > > up. > > > > > > Do we have a11y problems with the aura tooltip? Meaning, in other places > where > > > we use the aura tooltip (like bookmarks, chrome://apps, and everywhere else > > that > > > uses tooltips) are there a11y problems? > > > > What do browser frames do? Those seem to show native tooltips for their > caption > > buttons. Gating on OS_WIN (and desktop/ash_shell?) might be ok. > > Ah, yes the frames use the native hint as well. I'd say that is the same > mechanism: windows automatically adding a native tooltop based on hit testing. > > That doesn't seem so bad as the frame looks like a part of windows, so having > the native tooltip is ok. But these dialog close buttons don't feel like part of > windows so it would be nice if they were consistent with all the other tooltips. > > But it probably doesn't matter that much. > > I'll look at (a) can the native tooltip be suppressed while still returning > HTCLOSE, and if that doesn't work (b) I'll add some conditional around setting > the tooltip. Oh that previous comment is confusing. I added that last week and forgot to publish... you can just ignore it.
Please update the CL description and then this lgtm.
The CQ bit was checked by benwells@chromium.org
On 2015/04/08 01:44:03, msw wrote: > Please update the CL description and then this lgtm. k, done. landing now. Thanks!
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1044043004/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/5c30d9f20cde79aff3ae8d90c8e1909e351bea44 Cr-Commit-Position: refs/heads/master@{#324183} |
