240117 - Native theme IsWidgetStyled needs to stop depending on forms.css

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

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

Comment 1

[Ernst Sjostrand]

Attachment: Bluecurve native look

My first very simple attempt to make a native LAF for Bluecurve (GTK/QT-theme).

Comment 2

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

Comment on attachment 160695 [details]
Bluecurve native look

Whatever this is, it's not related to this bug.

Comment 3

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

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.

Comment 4

[[e-mail]]

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

Comment 5

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

One problem with what I propose is that it requires re-resolving style all the way up to the top of the tree.

Comment 6

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

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.

Comment 7

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

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.

Comment 8

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

Attachment: patch 1: make nsIStyleRule::MapRuleInfoInto able to map multiple structs

Comment 9

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

Attachment: patch 2: rewrite IsWidgetStyled to check whether the author styled borders or backgrounds

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.

Comment 10

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

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.

Comment 11

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

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

Comment 12

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

Attachment: patch 1: make nsIStyleRule::MapRuleInfoInto able to map multiple structs

This version moves the declaration of NS_STYLE_INHERIT_BIT so I don't need to drag in more headers.

Comment 13

[Boris Zbarsky [: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?

Comment 14

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

Attachment: patch 1: make nsIStyleRule::MapRuleInfoInto able to map multiple structs

Comment 15

[Boris Zbarsky [: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

Comment 16

[Boris Zbarsky [:bzbarsky]]

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

Comment 17

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

Patches checked in yesterday, reftests checked in just now.  Didn't seem to have any effect on any of the performance tests.

Comment 18

[philippe (part-time)]

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 ?

Comment 19

[Boris Zbarsky [:bzbarsky]]

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.

Comment 20

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

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

Comment 21

[Boris Zbarsky [:bzbarsky]]

Ah, yes.  Comment 3.

Comment 22

[philippe (part-time)]

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