Closed Bug 240117 Opened 16 years ago Closed 12 years ago

Native theme IsWidgetStyled needs to stop depending on forms.css

Categories

(Core :: CSS Parsing and Computation, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9beta2

People

(Reporter: bzbarsky, Assigned: dbaron)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Whiteboard: [patch])

Attachments

(2 files, 3 obsolete files)

The problem is that right now we drop theming on form controls if they are
"styled by the page" (which is a rather ill-defined concept anyway).  The way we
decide to do this is that we look at some computed style values on the control
and compare them to what forms.css says for the same control.  If the two
differ, we drop theming.

There are multiple problems here:

1)  Handling platform-forms.css is rather painful without code duplication.
2)  User sheets cause theming to drop (should they? maybe.)
3)  Changes to forms.css require changes to the C++ code in IsWidgetStyled.
    General fragility of code, harder to customize the browser, etc.

I feel that the "right" way to do this is to resolve a style context for the
node using only the UA (and maybe user) level and then compare to the normal
style context to see whether the widget is styled (and to clearly decide on what
it means to be styled, too).  Hopefully, presentational attributes are a
non-issue here....
Attached file Bluecurve native look (obsolete) —
My first very simple attempt to make a native LAF for Bluecurve (GTK/QT-theme).
Comment on attachment 160695 [details]
Bluecurve native look

Whatever this is, it's not related to this bug.
Attachment #160695 - Attachment is obsolete: true
There are a whole bunch of inconsistencies in the Mac code that I suspect ends up turning off native theming in a bunch of cases -- the Mac native theme code twiddles the default values in different cases than the forms.css modifies those.


In any case, I think we should fix this by adding an API to get the style data for a frame exclusive of author styles.

If user style sheets want to disable theming, they can use -moz-appearance: none.  At least, I think I prefer it that way.
(In reply to comment #3)
>If user style sheets want to disable theming, they can use -moz-appearance: none.
Note that several widgets don't work without -moz-appearance e.g. gnomestripe popups only use -moz-appearance and without that they become transparent.
Blocks: 196805
Depends on: 374907
One problem with what I propose is that it requires re-resolving style all the way up to the top of the tree.
Then again, most of the styles we want to check in IsWidgetStyled are not inherited -- we could probably make a significant improvement on what we have today by just re-resolving style on the element itself.
Then again, from the discussion in bug 396984, that's not even what we want -- we want to make something unstyled even if it's given an author-specified border-width that matches the default, since that may not match the native theme.
Assignee: general → dbaron
Priority: -- → P2
Target Milestone: --- → mozilla1.9 M9
Attachment #283417 - Flags: superreview?(bzbarsky)
Attachment #283417 - Flags: review?(bzbarsky)
I'm a bit worried about performance with this, though, since IsWidgetStyled is actually called from every call to ThemeSupportsWidget (although it only does work from some controls) which is in turn called from every call to nsIFrame::IsThemed.

I think we should probably make frames cache their themedness.  Maybe I'll be in the mood for the frame state bit dance later; I suspect the solution may be to cut into the frame-private bits and make block frames use an extra word for their private bits.
Attachment #283419 - Flags: superreview?(bzbarsky)
Attachment #283419 - Flags: review?(bzbarsky)
I should really add some testcases as well... so far my only testing has been attachment 281738 [details] (from bug 396984), where this makes us use the non-themed control for all but the "(auto)" control.
Status: NEW → ASSIGNED
Target Milestone: mozilla1.9 M9 → mozilla1.9 M10
Attachment #283417 - Flags: superreview?(bzbarsky)
Attachment #283417 - Flags: review?(bzbarsky)
(In reply to comment #9)
> work from some controls) which is in turn called from every call to
> nsIFrame::IsThemed.

er, every call to nsIFrame::IsThemed for a frame whose computed -moz-appearance is not none.
This version moves the declaration of NS_STYLE_INHERIT_BIT so I don't need to drag in more headers.
Attachment #283417 - Attachment is obsolete: true
Attachment #283431 - Flags: superreview?(bzbarsky)
Attachment #283431 - Flags: review?(bzbarsky)
Comment on attachment 283431 [details] [diff] [review]
patch 1: make nsIStyleRule::MapRuleInfoInto able to map multiple structs

>@@ -2019,17 +2019,18 @@ nsGenericHTMLElement::MapImageAlignAttri

>+      if (aRuleData->mSIDs & NS_STYLE_INHERIT_BIT(Display) && 

I'd prefer parens here around the '&' expression, just so it's clear for those who can't recall the relative precedence of & and &&.

There are several places in this patch where there's a code pattern like:

 if (aRuleData->mSIDs & NS_STYLE_INHERIT_BIT(X)) {
    ....
 } else if (aRuleData->mSIDs & NS_STYLE_INHERIT_BIT(Y)) {
    ....
 }

Which looks wrong to me if both bits are set.  Fix that and then I'll double-check and finish the review?
Attachment #283431 - Flags: superreview?(bzbarsky)
Attachment #283431 - Flags: superreview-
Attachment #283431 - Flags: review?(bzbarsky)
Attachment #283431 - Flags: review-
Attachment #283431 - Attachment is obsolete: true
Attachment #283622 - Flags: superreview?(bzbarsky)
Attachment #283622 - Flags: review?(bzbarsky)
Comment on attachment 283622 [details] [diff] [review]
patch 1: make nsIStyleRule::MapRuleInfoInto able to map multiple structs

>+++ b/content/html/content/src/nsHTMLHRElement.cpp
>+  if (aData->mSIDs & NS_STYLE_INHERIT_BIT(Position) ||
>+      aData->mSIDs & NS_STYLE_INHERIT_BIT(Border)) {

  aData->mSIDs & 
    (NS_STYLE_INHERIT_BIT(Position) | NS_STYLE_INHERIT_BIT(Border))

?

>+  if (aData->mSIDs & NS_STYLE_INHERIT_BIT(Border) && noshade) { // if not noshade, border styles are dealt with 

Parens around the first part?

>+++ b/content/html/content/src/nsHTMLSharedElement.cpp
>+  if (aData->mSIDs & NS_STYLE_INHERIT_BIT(Position) ||
>+      aData->mSIDs & NS_STYLE_INHERIT_BIT(Display)) {

Use bitwise or here too?

I suppose it doesn't matter much, but good to be consistent in the various places in this patch.... MapImageAlignAttributeInto uses bitwise or.  So the other option is to change that one to use boolean or.

r+sr=bzbarsky
Attachment #283622 - Flags: superreview?(bzbarsky)
Attachment #283622 - Flags: superreview+
Attachment #283622 - Flags: review?(bzbarsky)
Attachment #283622 - Flags: review+
Comment on attachment 283419 [details] [diff] [review]
patch 2: rewrite IsWidgetStyled to check whether the author styled borders or backgrounds

r+sr=bzbarsky, but yeah, this seems to want a framestate bit...
Attachment #283419 - Flags: superreview?(bzbarsky)
Attachment #283419 - Flags: superreview+
Attachment #283419 - Flags: review?(bzbarsky)
Attachment #283419 - Flags: review+
Attachment #283419 - Flags: approval1.9?
Attachment #283622 - Flags: approval1.9?
Assignee: dbaron → nobody
Status: ASSIGNED → NEW
Component: GFX → Style System (CSS)
QA Contact: ian → style-system
Assignee: nobody → dbaron
Whiteboard: [patch]
Patches checked in yesterday, reftests checked in just now.  Didn't seem to have any effect on any of the performance tests.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Flags: in-testsuite+
No longer blocks: 399273
Depends on: 399273
Puzzling behaviour:
Given the following code in userContent.css:
input[type="submit"] {
	border: 2px solid !important;
	background: #ccc !important;
	color: #000 !important;
}

load a page with a submit button that has been styled in the author stylesheet
- example:
input[type="submit"] {
	border: 2px solid;
	background: yellow;
	color: #080;
}

The submit button will keep its themed appearance (Aqua on OS X, where I'm testing this). I would expect somehow the border to be solid black with a grey background.

Any value for the border property will cause this. But only with shorthand notation.
border-style: /* value*/ !important;
border-width: /* value*/ !important;
is correctly applied when used in the user stylesheet.

Is that the intended consequence of these patches ?
As written, the patch ignores user stylesheets in deciding whether to drop native theming of form controls.  That is, the userContent.css stuff doesn't cause native theming to be dropped.  Since the userContent styles override every single style the page adds, the control effectively has no author style.  So we leave it native themed.

I don't think it's unreasonable to expect user sheets that want to drop native theming to explicitly use "-moz-appearance:none", but I could go either way on that.
(In reply to comment #19)
> I don't think it's unreasonable to expect user sheets that want to drop native
> theming to explicitly use "-moz-appearance:none", but I could go either way on
> that.

That expectation was my intent.
Ah, yes.  Comment 3.
(In reply to comment #19)
> As written, the patch ignores user stylesheets in deciding whether to drop
> native theming of form controls.  That is, the userContent.css stuff doesn't
> cause native theming to be dropped.  Since the userContent styles override
> every single style the page adds, the control effectively has no author style. 
> So we leave it native themed.

Ok, thanks. I commented above because I noticed that some combinations of rules (border) in a user stylesheet drop the theme, but some not (and it even varies depending on the type of form-control). Not really important, anyway, see below.
 
> I don't think it's unreasonable to expect user sheets that want to drop native
> theming to explicitly use "-moz-appearance:none", but I could go either way on
> that.

"-moz-appearance:none" now works perfectly fine in a user stylesheet, in my testing. Except for the file-upload control, that is.

input, select, button {-moz-appearance:none !important}.


Depends on: 419973
You need to log in before you can comment on or make changes to this bug.