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

Status: VERIFIED FIXED

(Core :: Layout: Form Controls, defect)

Description

[philippe (part-time)]

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.

Comment 1

[j.j.]

> 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?

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This should probably block or at least be wanted

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: 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.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 365356 [details] [diff] [review]
fix

Could you call the variable boxShadow firstBoxShadow instead?

Withthat, and an appropriate test, r+sr=dbaron.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix v2

Actually it's easy to test, I was being dumb.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/mozilla-central/rev/0ef8d47d1261

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 365368 [details] [diff] [review]
fix v2

reasonably safe patch, fixes bug in new feature

Comment 9

[Dão Gottwald [:dao]]

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].

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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.

Comment 11

[Dão Gottwald [:dao]]

(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.

Comment 12

[Dão Gottwald [:dao]]

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.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

So you think we should just suppress box-shadow for elements with -moz-appearance not 'none'?

Comment 14

[Dão Gottwald [:dao]]

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.

Comment 15

[Dão Gottwald [:dao]]

(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.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 17

[Dão Gottwald [:dao]]

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.

Comment 18

[Dão Gottwald [:dao]]

And I suppose that's why Webkit does what it does.

Comment 19

[Dão Gottwald [:dao]]

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.

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

David, can you give me your opinion here? Do you think dropping box-shadow on elements with -moz-appearance is preferable?

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Alright, I'll redo it to drop the box-shadow when -moz-appearance is present.

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix 2

Disable box-shadow when -moz-appearance is nonzero.

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

No, I made that pattern up.

http://hg.mozilla.org/mozilla-central/rev/42183a8ce3eb

Comment 26

[Dão Gottwald [:dao]]

(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...

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

What's the problem exactly? The change I landed is what you wanted, isn't it?

Comment 28

[Dão Gottwald [:dao]]

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.

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 30

[Dão Gottwald [:dao]]

Extensions can provide different stylesheets for different platforms. My Ctrl+Tab extension does exactly this while using box-shadow on a xul button.

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK but what if they forget to do that?

Comment 32

[Dão Gottwald [:dao]]

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 33

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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)

Comment 34

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 35

[Dão Gottwald [:dao]]

Yes, that's right.

Comment 36

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix 3

Comment 37

[Bill Gianopoulos [:WG9s]]

(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.

Comment 38

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Lets reopen for now.

Comment 39

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 40

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Pushed http://hg.mozilla.org/mozilla-central/rev/1c2980fbd280

Comment 41

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 370770 [details] [diff] [review]
fix 3

a1.9.1=dbaron

Comment 42

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0ed5b7c26d2f
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/07142f5971f8
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/24ecbaac8afa

Comment 43

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: 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?

Comment 44

[Dão Gottwald [:dao]]

(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.

Comment 45

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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

True, but what about the file picker?

Comment 46

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The file picker is not a native control, it's a combination of native things. Webkit doesn't hide box-shadow on it.

Comment 47

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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

Comment 48

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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