The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla12

Status

()

Core
CSS Parsing and Computation
P2
normal
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

Trunk
mozilla12
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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.
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.
Attachment #572386 - Flags: review?(dbaron)
Assignee: nobody → bzbarsky
OS: Mac OS X → All
Priority: -- → P2
Hardware: x86 → All
Whiteboard: [need review]
Version: 9 Branch → Trunk
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.
Attachment #572386 - Flags: review?(dbaron) → review+
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.
Whiteboard: [need review] → [need landing]
https://hg.mozilla.org/integration/mozilla-inbound/rev/88663cf7f5c1
Flags: in-testsuite?
Whiteboard: [need landing]
Target Milestone: --- → mozilla12
https://hg.mozilla.org/mozilla-central/rev/88663cf7f5c1
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Depends on: 797040
You need to log in before you can comment on or make changes to this bug.