box-shadow should be ignored when applied to native-themed elements

VERIFIED FIXED in mozilla1.9.2a1

Status

()

Core
Layout: Form Controls
VERIFIED FIXED
9 years ago
8 years ago

People

(Reporter: philippe (part-time), Assigned: roc)

Tracking

({verified1.9.1})

Trunk
mozilla1.9.2a1
verified1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
wanted1.9.1 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

9 years ago
Follow up on bug 475197.
Currently box-shadow is applied to form controls without dropping the native look. This looks rather bad, especially on OS X.
Box-shadow should be treated as author style and consequently drop the native look.

WebKit Mac ignores the box-shadow for otherwise unstyled widgets.

testcase: attachment (id=358660) in bug 475197

screenshot: attachment (id=359253) in bug 475197

-------  Bug 475197 Comment #12 From  Robert O'Callahan (:roc) (Mozilla Corporation)   2009-01-28 14:08:55 PST   (-) [reply] -------

Yeah, that's just a Webkit bug.

If we wanted to do something here, we would treat box-shadow as author style
and drop the native look for widgets with shadows.
Hardware: x86 → All

Comment 1

9 years ago
> testcase: attachment (id=358660) [details] in bug 475197
> 
> screenshot: attachment (id=359253) [details] in bug 475197

This is attachment 358788 [details] (not 358660)

We keep radio buttons and checkboxes like the screenshot shows, yes?
This should probably block or at least be wanted
Flags: blocking1.9.1?
Flags: blocking1.9.1? → wanted1.9.1+
Assignee: ventnor.bugzilla → roc
Created attachment 365356 [details] [diff] [review]
fix

I think this is right.

I can't figure out a good way to reftest it though. The best I can come up with is a set of != tests that compare a form element with box-shadow (but the shadow moved out of view) to an unstyled form element. But that would break if native themes are disabled or not available.
Attachment #365356 - Flags: superreview?(dbaron)
Attachment #365356 - Flags: review?(dbaron)
Whiteboard: [needs review]
We already have a bunch of reftests with the same issue in layout/reftests/native-theme/.  I think you should probably just add another one.
Attachment #365356 - Flags: superreview?(dbaron)
Attachment #365356 - Flags: superreview+
Attachment #365356 - Flags: review?(dbaron)
Attachment #365356 - Flags: review+
Comment on attachment 365356 [details] [diff] [review]
fix

Could you call the variable boxShadow firstBoxShadow instead?

Withthat, and an appropriate test, r+sr=dbaron.
Created attachment 365368 [details] [diff] [review]
fix v2

Actually it's easy to test, I was being dumb.
Attachment #365356 - Attachment is obsolete: true
Attachment #365368 - Flags: superreview+
Attachment #365368 - Flags: review+
Whiteboard: [needs review] → [needs landing]
http://hg.mozilla.org/mozilla-central/rev/0ef8d47d1261
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs landing] → [needs 191 approval]
Comment on attachment 365368 [details] [diff] [review]
fix v2

reasonably safe patch, fixes bug in new feature
I suppose this breaks bug 483378. Would have been nicer if the region for the box-shadow would have been fixed, as with the radio button in attachment 359253 [details].
(In reply to comment #9)
> I suppose this breaks bug 483378.

Sorry... Can't you work around it by putting a shadow or background on a container element instead?

> Would have been nicer if the region for the
> box-shadow would have been fixed, as with the radio button in attachment
> 359253 [details].

Discovering accurate information about the shape of native form controls sounds really hard. I think radio buttons only looked right because our style sheet gave them a border-radius that happens to match that native theme.
(In reply to comment #10)
> (In reply to comment #9)
> > I suppose this breaks bug 483378.
> 
> Sorry... Can't you work around it by putting a shadow or background on a
> container element instead?

Nope, it's an inset shadow.
I think this bug needs to be re-evaluated. Even if the correct shape can't be used easily, drawing a rectangular shadow seems closer to that ideal solution, and closer to what Webkit does.

The result is uglier with this fix than without it if you consider something like this:

button:focus {
  -moz-box-shadow: 0 0 .5em .1em #fc6;
}

I'm using this in bug 465076, and in the equivalent extension for Firefox 3.5.
So you think we should just suppress box-shadow for elements with -moz-appearance not 'none'?
That would require me to use some workaround for bug 465076 / the extension as well. (Also bug 483378, but my use of box-shadow there can be regarded as a hack.)

I don't think anything needs to be done in this bug, it hasn't caused any trouble in the wild.
Someone who uses box-shadow on form elements and sees that:

- the native appearance is suddenly dropped (likely depending on :focus, which is even worse) can avoid box-shadow or style the elements manually.

- box-shadow simply doesn't work, will likely be confused, but might be able to figure out that dropping the native appearance helps.

- box-shadow doesn't follow the native shape (mostly only relevant for buttons on OS X) can hack around it with border-radius, avoid box-shadow, or style the elements manually. The author has the most freedom here, and neither the initial brokenness nor the possible actions are worse than with the fixes.
(In reply to comment #14)
> (mostly only relevant for buttons on OS X)

Speaking of which, if box-shadow could use a 100% border-radius in that particular case automatically (without really figuring out the shape of the widget), that would help a bit. That would obviously be a hack.
If we don't do anything, then a lot of authors on Windows will not notice any problem with box-shadow on Windows' square-corner buttons, and may create pages that look bad on Mac and also on GTK themes that use buttons with slightly rounded corners.

Hacking in 100% border-radius for buttons on Mac will still leave us with those GTK themes looking bad. Plus it'd be a huge hack.

I honestly think what we have here is safe and reasonable. It's unfortunate that it breaks some potential use of box-shadow in chrome but we have to focus on the use of features in Web content, not chrome.
Then I suggest ignoring the box-shadow, which would be more forward-compatible, if it should ever be possible to use the native widget border. Dropping the native look is probably not what the author wants, except when it's done by specifying a border or background.
And I suppose that's why Webkit does what it does.
As for my personal cases (bugs mentioned above) -- they don't seem to be affected, now that I've used an up-to-date build. box-shadow continues to work for XUL widgets. That's somewhat inconsistent, but then again, a CSS border doesn't drop the native appearance either.

That makes me, personally, unaffected, but doesn't change my view on what's better for web content.
David, can you give me your opinion here? Do you think dropping box-shadow on elements with -moz-appearance is preferable?
I don't have strong feelings, but I'm generally in favor of the native appearance being used more rather than less... dropping native theming when border and background are specified is needed for compatibility, but I don't think that argument applies to box-shadow.
Alright, I'll redo it to drop the box-shadow when -moz-appearance is present.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [needs 191 approval]
Created attachment 369950 [details] [diff] [review]
fix 2

Disable box-shadow when -moz-appearance is nonzero.
Attachment #369950 - Flags: review?(dbaron)
Whiteboard: [needs review]
Attachment #369950 - Flags: superreview+
Attachment #369950 - Flags: review?(dbaron)
Attachment #369950 - Flags: review+
Comment on attachment 369950 [details] [diff] [review]
fix 2

>+  if (!styleBorder->mBoxShadow || aForFrame->GetStyleDisplay()->mAppearance)
>+  if (!styleBorder->mBoxShadow || aForFrame->GetStyleDisplay()->mAppearance)
>+  if (boxShadows && !aFrame->GetStyleDisplay()->mAppearance) {

s/GetStyleDisplay()->mAppearance/IsThemed()/ for all three of these.

(Was there some other place you got this pattern from?)

r+sr=dbaron with that
No, I made that pattern up.

http://hg.mozilla.org/mozilla-central/rev/42183a8ce3eb
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED
Whiteboard: [needs review] → [needs 191 approval]
(00:08:47) philor: dao: so, after bug 483378, what was not-found on Windows supposed to look like? I'm hoping not the white-on-white that I'm seeing...
What's the problem exactly? The change I landed is what you wanted, isn't it?
I suggested it because dropping the appearance didn't seem desirable, but that was done only for Web content. Apparently it affects chrome / XUL this time.
Depends on: 486425
Hmm. I'm not sure if we should always allow box-shadow with -moz-appearance for chrome. People can write extensions that would look bad on other platforms, the same way they can write bad Web content.
Extensions can provide different stylesheets for different platforms. My Ctrl+Tab extension does exactly this while using box-shadow on a xul button.
OK but what if they forget to do that?
What if they forget other platforms and want the box-shadow anyway? Comment 10 seems like an obvious workaround, but it won't help the fact that they forgot other platforms.

It's an edge case, as most extensions integrate with the application chrome, which means doing only little styling. An extension that doesn't take care of that will have numerous glitches with different OS and application themes anyway. We won't be able to disable everything that extension authors could mess with.
Comment on attachment 369950 [details] [diff] [review]
fix 2

a1.9.1=dbaron, although I'm not sure exactly what the discussion above means we should do.  (i.e., not sure whether people are saying we shouldn't do this)
Attachment #369950 - Flags: approval1.9.1? → approval1.9.1+
I think Dao wants -moz-appearance to not turn off box-shadow in chrome documents. Is that right? David, if you think that makes sense, I'll do another patch for it.
Yes, that's right.
Whiteboard: [needs 191 approval] → [needs review]
(In reply to comment #36)
> Created an attachment (id=370770) [details]
> fix 3

I have verified this patch fixes the issue reported in bug 486425 under both Windows and Linux.
Lets reopen for now.
Status: RESOLVED → REOPENED
OS: Mac OS X → All
Resolution: FIXED → ---
Comment on attachment 370770 [details] [diff] [review]
fix 3

I'm a little uncomfortable with the chrome/content distinction here, but I suppose it's probably the right thing to do.  r+sr=dbaron
Attachment #370770 - Flags: superreview+
Attachment #370770 - Flags: review?(dbaron)
Attachment #370770 - Flags: review+
Pushed http://hg.mozilla.org/mozilla-central/rev/1c2980fbd280
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED
Whiteboard: [needs review] → [needs 191 approval]
Comment on attachment 370770 [details] [diff] [review]
fix 3

a1.9.1=dbaron
Attachment #370770 - Flags: approval1.9.1? → approval1.9.1+
Whiteboard: [needs 191 approval] → [needs 191 landing]
Created attachment 372214 [details]
Current state (left side)

Roc, means we drop box-shadow for native form controls? If yes, the file picker and inputs of type image still shows it. Shall I file a follow-up bug?
(In reply to comment #43)
> Roc, means we drop box-shadow for native form controls? If yes, the file picker
> and inputs of type image still shows it.

type="image" inputs don't have a native appearance.
(In reply to comment #44)
> type="image" inputs don't have a native appearance.

True, but what about the file picker?
The file picker is not a native control, it's a combination of native things. Webkit doesn't hide box-shadow on it.
Based on latest comments we are doing well now. Verified on 1.9.2 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2pre) Gecko/20100115 Namoroka/3.6pre ID:20100115050432
Status: RESOLVED → VERIFIED
Target Milestone: --- → mozilla1.9.2a1
Same for Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1.8pre) Gecko/20100104 Shiretoko/3.5.8pre ID:20100104030751
Keywords: fixed1.9.1 → verified1.9.1
Summary: box-shadow applied to form controls should drop the native look → box-shadow should be ignored when applied to native-themed elements
You need to log in before you can comment on or make changes to this bug.