Closed
Bug 476062
Opened 16 years ago
Closed 16 years ago
box-shadow should be ignored when applied to native-themed elements
Categories
(Core :: Layout: Form Controls, defect)
Core
Layout: Form Controls
Tracking
()
VERIFIED
FIXED
mozilla1.9.2a1
People
(Reporter: phiw2, Assigned: roc)
References
Details
(Keywords: verified1.9.1)
Attachments
(4 files, 1 obsolete file)
(deleted),
patch
|
roc
:
review+
roc
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
dbaron
:
review+
dbaron
:
superreview+
dbaron
:
approval1.9.1+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
dbaron
:
review+
dbaron
:
superreview+
dbaron
:
approval1.9.1+
|
Details | Diff | Splinter Review |
(deleted),
image/jpeg
|
Details |
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.
Updated•16 years ago
|
Hardware: x86 → All
> 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?
Assignee | ||
Comment 2•16 years ago
|
||
This should probably block or at least be wanted
Flags: blocking1.9.1?
Assignee | ||
Updated•16 years ago
|
Flags: blocking1.9.1? → wanted1.9.1+
Assignee | ||
Updated•16 years ago
|
Assignee: ventnor.bugzilla → roc
Assignee | ||
Comment 3•16 years ago
|
||
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)
Assignee | ||
Updated•16 years ago
|
Whiteboard: [needs review]
Comment 4•16 years ago
|
||
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.
Updated•16 years ago
|
Attachment #365356 -
Flags: superreview?(dbaron)
Attachment #365356 -
Flags: superreview+
Attachment #365356 -
Flags: review?(dbaron)
Attachment #365356 -
Flags: review+
Comment 5•16 years ago
|
||
Comment on attachment 365356 [details] [diff] [review]
fix
Could you call the variable boxShadow firstBoxShadow instead?
Withthat, and an appropriate test, r+sr=dbaron.
Assignee | ||
Comment 6•16 years ago
|
||
Actually it's easy to test, I was being dumb.
Attachment #365356 -
Attachment is obsolete: true
Attachment #365368 -
Flags: superreview+
Attachment #365368 -
Flags: review+
Assignee | ||
Updated•16 years ago
|
Whiteboard: [needs review] → [needs landing]
Assignee | ||
Comment 7•16 years ago
|
||
Status: NEW → RESOLVED
Closed: 16 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs landing] → [needs 191 approval]
Assignee | ||
Updated•16 years ago
|
Attachment #365368 -
Flags: approval1.9.1?
Assignee | ||
Comment 8•16 years ago
|
||
Comment on attachment 365368 [details] [diff] [review]
fix v2
reasonably safe patch, fixes bug in new feature
Comment 9•16 years ago
|
||
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].
Assignee | ||
Comment 10•16 years ago
|
||
(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•16 years ago
|
||
(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•16 years ago
|
||
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.
Assignee | ||
Comment 13•16 years ago
|
||
So you think we should just suppress box-shadow for elements with -moz-appearance not 'none'?
Comment 14•16 years ago
|
||
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•16 years ago
|
||
(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.
Assignee | ||
Comment 16•16 years ago
|
||
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•16 years ago
|
||
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•16 years ago
|
||
And I suppose that's why Webkit does what it does.
Comment 19•16 years ago
|
||
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.
Assignee | ||
Comment 20•16 years ago
|
||
David, can you give me your opinion here? Do you think dropping box-shadow on elements with -moz-appearance is preferable?
Comment 21•16 years ago
|
||
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.
Assignee | ||
Updated•16 years ago
|
Attachment #365368 -
Flags: approval1.9.1?
Assignee | ||
Comment 22•16 years ago
|
||
Alright, I'll redo it to drop the box-shadow when -moz-appearance is present.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [needs 191 approval]
Assignee | ||
Comment 23•16 years ago
|
||
Disable box-shadow when -moz-appearance is nonzero.
Attachment #369950 -
Flags: review?(dbaron)
Assignee | ||
Updated•16 years ago
|
Whiteboard: [needs review]
Updated•16 years ago
|
Attachment #369950 -
Flags: superreview+
Attachment #369950 -
Flags: review?(dbaron)
Attachment #369950 -
Flags: review+
Comment 24•16 years ago
|
||
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
Assignee | ||
Comment 25•16 years ago
|
||
No, I made that pattern up.
http://hg.mozilla.org/mozilla-central/rev/42183a8ce3eb
Status: REOPENED → RESOLVED
Closed: 16 years ago → 16 years ago
Resolution: --- → FIXED
Whiteboard: [needs review] → [needs 191 approval]
Assignee | ||
Updated•16 years ago
|
Attachment #369950 -
Flags: approval1.9.1?
Comment 26•16 years ago
|
||
(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...
Assignee | ||
Comment 27•16 years ago
|
||
What's the problem exactly? The change I landed is what you wanted, isn't it?
Comment 28•16 years ago
|
||
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.
Assignee | ||
Comment 29•16 years ago
|
||
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•16 years ago
|
||
Extensions can provide different stylesheets for different platforms. My Ctrl+Tab extension does exactly this while using box-shadow on a xul button.
Assignee | ||
Comment 31•16 years ago
|
||
OK but what if they forget to do that?
Comment 32•16 years ago
|
||
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•16 years ago
|
||
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+
Assignee | ||
Comment 34•16 years ago
|
||
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•16 years ago
|
||
Yes, that's right.
Assignee | ||
Comment 36•16 years ago
|
||
Attachment #370770 -
Flags: review?(dbaron)
Assignee | ||
Updated•16 years ago
|
Whiteboard: [needs 191 approval] → [needs review]
Comment 37•16 years ago
|
||
(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•16 years ago
|
||
Lets reopen for now.
Status: RESOLVED → REOPENED
OS: Mac OS X → All
Resolution: FIXED → ---
Comment 39•16 years ago
|
||
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+
Assignee | ||
Comment 40•16 years ago
|
||
Status: REOPENED → RESOLVED
Closed: 16 years ago → 16 years ago
Resolution: --- → FIXED
Whiteboard: [needs review] → [needs 191 approval]
Assignee | ||
Updated•16 years ago
|
Attachment #370770 -
Flags: approval1.9.1?
Comment 41•16 years ago
|
||
Comment on attachment 370770 [details] [diff] [review]
fix 3
a1.9.1=dbaron
Attachment #370770 -
Flags: approval1.9.1? → approval1.9.1+
Assignee | ||
Updated•16 years ago
|
Whiteboard: [needs 191 approval] → [needs 191 landing]
Assignee | ||
Comment 42•16 years ago
|
||
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
Keywords: fixed1.9.1
Whiteboard: [needs 191 landing]
Comment 43•16 years ago
|
||
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•16 years ago
|
||
(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•16 years ago
|
||
(In reply to comment #44)
> type="image" inputs don't have a native appearance.
True, but what about the file picker?
Assignee | ||
Comment 46•16 years ago
|
||
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•15 years ago
|
||
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
Comment 48•15 years ago
|
||
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
Assignee | ||
Updated•14 years ago
|
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.
Description
•