700221 - Fix for bug 495798 makes us not drop native theming for any style changes other than background color when author styles are disabled

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

The logic added in that bug is like so, in pseudocode:

  if (one of our whitelisted props is set) {
    if (aAuthorColorsAllowed ||
        (i == backColorIndex &&
         !values[i]->IsNonTransparentColor())) {
      drop native theming
    } else {
      keep going
    }
  }

On the face of it, this seems to be incorrect for the non-color properties.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Don't pay attention to whether author color are allowed when deciding whether a change to a non-color property should drop native theming.

Comment 2

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

Comment on attachment 572386 [details] [diff] [review]
Don't pay attention to whether author color are allowed when deciding whether a change to a non-color property should drop native theming.

Given that -moz-appearance isn't marked CSS_PROPERTY_IGNORED_WHEN_COLORS_DISABLED, I think this makes sense.

An alternative view would be to assume that the colors-disabled pref also applies to native theming (since that's mostly color).  I think that was perhaps the worldview that led to the code being the way it was -- except that -moz-appearance doesn't have CSS_PROPERTY_IGNORED_WHEN_COLORS_DISABLED so authors who disable native theming explicitly can do so.

I suppose I don't have strong feelings about which direction we go.  This patch does make things more consistent, so r=dbaron.

But I think a reasonable alternative would be to keep this code as-is and put -moz-apperanace in the CSS_PROPERTY_IGNORED_WHEN_COLORS_DISABLED set.  I might even prefer that slightly.

Comment 3

[Boris Zbarsky [:bzbarsky]]

I think there's enough margin stuff and whatnot going on with native theming that authors sometimes disable it because they just want sane layout...  If you feel strongly that we should add -moz-appearance to CSS_PROPERTY_IGNORED_WHEN_COLORS_DISABLED, I can do that, but I think I prefer this patch as-is.

Comment 4

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/88663cf7f5c1

Comment 5

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/88663cf7f5c1