Closed Bug 1111145 Opened 10 years ago Closed 5 years ago

[Windows] Dropdowns of URL bar and search box should use a better-looking shadow and border

Categories

(Core :: Widget: Win32, enhancement, P5)

Product:
Core
Component:
Widget: Win32
Version:
36 Branch
Platform:
x86
Windows 8
Type:
enhancement
Points:
5

Tracking

()

Status:
RESOLVED DUPLICATE of bug 1575360

People

(Reporter: phlsa, Unassigned)

References

Details

(Whiteboard: [qx:spec][outreachy-12][tpi:+])

Attachments

(3 files)

Current and desired shadow typ
76.26 KB, image/png
Details
Bug 1111145 - The URL bar and search box dropdowns shadow should match the Firefox menu's.
58 bytes, text/x-review-board-request
Details
WIP patch changing widget code
5.72 KB, patch
Details | Diff | Splinter Review 
Part of why the dropdowns of the URL bar and the search box look a little dated at the moment are their borders and shadows.
We can solve this by giving them a similar styling as the panel menus which already get this part right (see attachment).
AFAIK the border and the drop shadow are drawn natively (and should match the border and drop shadow of other panels and menu popups across Windows).
Component: Theme → Widget: Win32
Product: Firefox → Core
Hm, I'm not sure how they are drawn, but they are not the same as native shadows (particularly visible in the bottom right corner). The border is also different than the one of native dropdowns.
Whiteboard: [qx]
Part of the problem seems to be that we don't use -moz-appearance:menupopup for these popups on Windows. Not sure why. We do use -moz-appearance:menupopup on Linux and get a much more polished look there (depends on the desktop environment, though).
I'm confused. I'm 99% sure the shadows on the panels are custom (ie CSS box shadows or similar), and I also can't see a difference between the shadow the url bar dropdown currently has on my Win8.1 machine, and the shadow that the context menu has (which we do set -moz-appearance:menupopup on).

So it sounds like this is asking for custom shadows. Dão, am I missing something?
Flags: needinfo?(dao)
(In reply to :Gijs Kruitbosch from comment #4)
> and I also can't see a difference between the shadow
> the url bar dropdown currently has on my Win8.1 machine, and the shadow that
> the context menu has (which we do set -moz-appearance:menupopup on).

If this is so and if they don't match native shadows as comment 2 says, then this is a widget bug.

> So it sounds like this is asking for custom shadows.

You mean for these two popups or for all popups? Either way, for both internal and external consistency, we should default to native (and maybe make the arrow panel shadow better mimic native).
Flags: needinfo?(dao)
Whiteboard: [qx] → [qx:spec]
Blocks: 1247816
No longer blocks: fx-qx
Mentor: jaws
Whiteboard: [qx:spec] → [qx:spec][outreachy-12]
Points: --- → 5
Severity: normal → enhancement
Priority: P4 → P5
Whiteboard: [qx:spec][outreachy-12] → [qx:spec][outreachy-12][tpi:+]
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Comment on attachment 8792855 [details]
Bug 1111145 - The URL bar and search box dropdowns shadow should match the Firefox menu's.

This doesn't get us external consistency since AIUI our supposedly native popup shadows don't actually match the native ones elsewhere on Windows.

Besides, I don't think graytext is the right color here. Generally the shadow shouldn't have more opacity than the border, because that just looks broken depending on the background.
Attachment #8792855 - Flags: review?(dao+bmo) → review-
(In reply to Dão Gottwald [:dao] from comment #7)
> Comment on attachment 8792855 [details]
> Bug 1111145 - Use native looking shadows for arrow panels to improve
> internal and external consistency.
> 
> This doesn't get us external consistency since AIUI our supposedly native
> popup shadows don't actually match the native ones elsewhere on Windows.
> 
> Besides, I don't think graytext is the right color here. Generally the
> shadow shouldn't have more opacity than the border, because that just looks
> broken depending on the background.

Yeah, GrayText isn't right here. I had a different patch, I must not have uploaded the right one.
ThreeDDarkShadow was the closest system color I could find to match the shadow of the context menu in notepad.exe
Also, I couldn't find a suitable system color to use for the border color here.

Stephen, if we were to try to match the shadows of native popups on Windows, what would you recommend for the border-color of the popups? We shouldn't use hsla(210,4%,10%,.2) because it's slightly more transparent than the first pixel of ThreeDDarkShadow.
Flags: needinfo?(shorlander)
Comment on attachment 8792855 [details]
Bug 1111145 - The URL bar and search box dropdowns shadow should match the Firefox menu's.

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #10)
> ThreeDDarkShadow was the closest system color I could find to match the
> shadow of the context menu in notepad.exe

I'm not sure notepad is the right role model here. Windows applications aren't always consistent with these kind of things and notepad is probably one of the applications that are more likely to be outdated.

In any case, this patch is still clearly wrong for using a more opaque color for the shadow than for the border.

We don't necessarily need to use a platform color for the shadow. In fact, for non-default themes (e.g. high-contrast), we shouldn't use a platform color, because we need the shadow to be dark all the time even if it may not be visible with a dark theme. A white shadow wouldn't make sense visually.
Attachment #8792855 - Flags: review?(dao+bmo) → review-
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #11)
> Also, I couldn't find a suitable system color to use for the border color
> here.
> 
> Stephen, if we were to try to match the shadows of native popups on Windows,
> what would you recommend for the border-color of the popups? We shouldn't
> use hsla(210,4%,10%,.2) because it's slightly more transparent than the
> first pixel of ThreeDDarkShadow.

I am not entirely sure why we are changing this? The summary for this bug seemed to be saying "make the shadows on the Awesome Bar and Search Bar results dropdown match the arrow panels shadow". This patch appears to be doing the opposite unless I am misreading it.
Flags: needinfo?(shorlander) → needinfo?(jaws)
I've got a patch about half-way done to fix the styling per comment 0.
Flags: needinfo?(jaws)
Comment on attachment 8792855 [details]
Bug 1111145 - The URL bar and search box dropdowns shadow should match the Firefox menu's.

Why did you mess with Mac and Linux? This is a Windows-specific bug. The native shadow we get on Ubuntu for instance is much larger and nicely smooth; I don't think we can do better with custom CSS. I suspect this is true on Mac too.

This is also an awfully complicated patch to work around what seems like a Win32 widget bug to me. Why is the default popup shadow that we draw so ugly?
Attachment #8792855 - Flags: review?(florian) → review-
(In reply to Dão Gottwald [:dao] from comment #16)
> Comment on attachment 8792855 [details]
> Bug 1111145 - The URL bar and search box dropdowns shadow should match the
> Firefox menu's.
> 
> Why did you mess with Mac and Linux? This is a Windows-specific bug. The
> native shadow we get on Ubuntu for instance is much larger and nicely
> smooth; I don't think we can do better with custom CSS. I suspect this is
> true on Mac too.

Sorry, I undid the mac and linux changes.

> This is also an awfully complicated patch to work around what seems like a
> Win32 widget bug to me. Why is the default popup shadow that we draw so ugly?

This patch isn't as complicated as it looks. I added another container inside of the autocomplete popups which is the same as how arrow panels work. The shadow for these popups is also implemented the same as arrow panel shadows.
Comment on attachment 8792855 [details]
Bug 1111145 - The URL bar and search box dropdowns shadow should match the Firefox menu's.

I realize this only adds a container, but this change to the DOM structure affects various areas of code as your patch demonstrates, and you may well have missed some places.

I still think we should figure out what's going on on the widget side and whether we can do better rendering the default popup shadow. This would improve the situation across Firefox and not just for these two popups.
Attachment #8792855 - Flags: review?(dao+bmo)
This patch disables the shadow if the default Windows theme is being used, but we can't draw our own shadow because it is outside of the window rect for the popup. Jim, do you know of anything that we can do here to increase the window size?

I tried setting a margin on the popup but that only moves the window. We would need the intrinsic size of the popup to increase to leave room for the shadow.
Attachment #8796304 - Flags: feedback?(jmathies)
(In reply to Dão Gottwald [:dao] from comment #16)
> This is also an awfully complicated patch to work around what seems like a
> Win32 widget bug to me. Why is the default popup shadow that we draw so ugly?

Windows draws this on all WS_POPUP windows.
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #22)
> Created attachment 8796304 [details] [diff] [review]
> WIP patch changing widget code
> 
> This patch disables the shadow if the default Windows theme is being used,
> but we can't draw our own shadow because it is outside of the window rect
> for the popup. Jim, do you know of anything that we can do here to increase
> the window size?
> 
> I tried setting a margin on the popup but that only moves the window. We
> would need the intrinsic size of the popup to increase to leave room for the
> shadow.

Hmm, not sure. Down at the widget level you're going to break window input coords if you mess with the window position, size, or the offset of content.

Do you want to paint the shadow via standard css? If this is the case you should look at addressing this through the underlying toolkit widgets.
(In reply to Jim Mathies [:jimm] from comment #24)
> Do you want to paint the shadow via standard css? If this is the case you
> should look at addressing this through the underlying toolkit widgets.

Ideally yes, because that would allow us to change it easier. This is what my other patch does, so I'll re-request review on that from Dao.
Comment on attachment 8792855 [details]
Bug 1111145 - The URL bar and search box dropdowns shadow should match the Firefox menu's.

Re-requesting review on this. Right now it's limited to just the two navigation toolbar dropdowns. The only other route I could think of would be to use ::outside but that's not implemented and has no path forwards (https://wiki.csswg.org/ideas/pseudo-elements)
Attachment #8792855 - Flags: review?(dao+bmo)
Comment on attachment 8792855 [details]
Bug 1111145 - The URL bar and search box dropdowns shadow should match the Firefox menu's.

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #22)
> Created attachment 8796304 [details] [diff] [review]
> WIP patch changing widget code
> 
> This patch disables the shadow if the default Windows theme is being used,

There's also -moz-window-shadow:none, current only implemented for Mac.

> but we can't draw our own shadow because it is outside of the window rect
> for the popup. Jim, do you know of anything that we can do here to increase
> the window size?
> 
> I tried setting a margin on the popup but that only moves the window. We
> would need the intrinsic size of the popup to increase to leave room for the
> shadow.

Is there some sane way widget code could render a custom shadow outside of the popups client area?

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #25)
> (In reply to Jim Mathies [:jimm] from comment #24)
> > Do you want to paint the shadow via standard css? If this is the case you
> > should look at addressing this through the underlying toolkit widgets.
> 
> Ideally yes, because that would allow us to change it easier. This is what
> my other patch does, so I'll re-request review on that from Dao.

We don't want to change popup shadows all the time, so I don't buy your argument for why we'd want to use standard CSS here. Ideally the window manager would give us good looking shadows like on every other modern OS. It's almost embarrassing. Maybe we should reach out to Microsoft and ask if refining the Windows-95-style popup shadows is on their radar, or if there are technical reasons why they can't do it, if there's a better API coming that we should use, etc.
Attachment #8792855 - Flags: review?(dao+bmo)
Assignee: jaws → nobody
Status: ASSIGNED → NEW
Attachment #8796304 - Flags: feedback?(jmathies)
(In reply to Dão Gottwald [:dao] from comment #27)
> Comment on attachment 8792855 [details]
> Bug 1111145 - The URL bar and search box dropdowns shadow should match the
> Firefox menu's.
> 
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #22)
> > Created attachment 8796304 [details] [diff] [review]
> > WIP patch changing widget code
> > 
> > This patch disables the shadow if the default Windows theme is being used,
> 
> There's also -moz-window-shadow:none, current only implemented for Mac.
> > but we can't draw our own shadow because it is outside of the window rect
> > for the popup. Jim, do you know of anything that we can do here to increase
> > the window size?
> > 
> > I tried setting a margin on the popup but that only moves the window. We
> > would need the intrinsic size of the popup to increase to leave room for the
> > shadow.
> 
> Is there some sane way widget code could render a custom shadow outside of
> the popups client area?

Using the native drop shadow, but afaict here, you guys don't like the look of that. Maybe we should create a new panel that has a transparent container for the shadow?


> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #25)
> > (In reply to Jim Mathies [:jimm] from comment #24)
> > > Do you want to paint the shadow via standard css? If this is the case you
> > > should look at addressing this through the underlying toolkit widgets.
> > 
> > Ideally yes, because that would allow us to change it easier. This is what
> > my other patch does, so I'll re-request review on that from Dao.
> 
> We don't want to change popup shadows all the time, so I don't buy your
> argument for why we'd want to use standard CSS here. Ideally the window
> manager would give us good looking shadows like on every other modern OS.

Are you seeing differences in the native shadows Windows supplies for our popups? Just poking around in win7 and 8, I don't see any variance in system drop shadows.
(In reply to Jim Mathies [:jimm] from comment #28)
> (In reply to Dão Gottwald [:dao] from comment #27)
> > Comment on attachment 8792855 [details]
> > Bug 1111145 - The URL bar and search box dropdowns shadow should match the
> > Firefox menu's.
> > 
> > (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #22)
> > > Created attachment 8796304 [details] [diff] [review]
> > > WIP patch changing widget code
> > > 
> > > This patch disables the shadow if the default Windows theme is being used,
> > 
> > There's also -moz-window-shadow:none, current only implemented for Mac.
> > > but we can't draw our own shadow because it is outside of the window rect
> > > for the popup. Jim, do you know of anything that we can do here to increase
> > > the window size?
> > > 
> > > I tried setting a margin on the popup but that only moves the window. We
> > > would need the intrinsic size of the popup to increase to leave room for the
> > > shadow.
> > 
> > Is there some sane way widget code could render a custom shadow outside of
> > the popups client area?
> 
> Using the native drop shadow, but afaict here, you guys don't like the look
> of that. Maybe we should create a new panel that has a transparent container
> for the shadow?

A new panel that has a (configurable) transparent area around it would be great! We could possibly use it to rotate the contents of a panel with CSS transforms without having the panel get clipped.
Depends on: 1551598
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: