Last Comment Bug 700221 - Fix for bug 495798 makes us not drop native theming for any style changes other than background color when author styles are disabled
: Fix for bug 495798 makes us not drop native theming for any style changes oth...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: P2 normal (vote)
: mozilla12
Assigned To: Boris Zbarsky [:bz]
:
Mentors:
Depends on: 797040
Blocks: 495798
  Show dependency treegraph
 
Reported: 2011-11-06 21:08 PST by Boris Zbarsky [:bz]
Modified: 2012-10-03 07:32 PDT (History)
2 users (show)
bzbarsky: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Don't pay attention to whether author color are allowed when deciding whether a change to a non-color property should drop native theming. (3.64 KB, patch)
2011-11-06 21:24 PST, Boris Zbarsky [:bz]
dbaron: review+
Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] 2011-11-06 21:08:17 PST
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 [:bz] 2011-11-06 21:24:21 PST
Created 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.
Comment 2 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2011-12-09 14:39:34 PST
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 [:bz] 2011-12-09 20:40:13 PST
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 5 Ed Morley [:emorley] 2011-12-22 03:50:02 PST
https://hg.mozilla.org/mozilla-central/rev/88663cf7f5c1

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