Closed Bug 1209273 Opened 9 years ago Closed 8 years ago

implement the 'color-adjust' CSS property to allow pages to opt in to printing background colors and images (-webkit-print-color-adjust)

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
platform-rel --- +
firefox44 --- affected
firefox48 --- fixed

People

(Reporter: dbaron, Assigned: tschneider)

References

(Blocks 1 open bug)

Details

(Keywords: css3, Whiteboard: [platform-rel-Google][platform-rel-GoogleDocs])

Attachments

(3 files, 13 obsolete files)

963 bytes, patch
dbaron
: review+
Details | Diff | Splinter Review
9.89 KB, patch
Details | Diff | Splinter Review
2.20 KB, patch
Details | Diff | Splinter Review
By default, we don't print background colors and images because many pages were not designed for printing, and many of those pages would waste large amounts of ink if they were printed.

However, we should have a mechanism for pages to declare that they were designed for printing and that we should print their background colors and images.  This makes it much easier for pages to work around this limitation (which they can work around anyway, just with more work).  It's fine for it to be easy to work around, given that the underlying problem was really dealing with wasting ink due to pages with dark backgrounds that weren't designed for printing.

There have been a number of proposals for this:

https://developer.mozilla.org/en-US/docs/Web/CSS/-webkit-print-color-adjust documents the -webkit-print-color-adjust property

https://wiki.csswg.org/ideas/print-backgrounds has a discussion about ideas

https://drafts.csswg.org/css-color-4/#color-adjust has a spec draft for one of those ideas, similar to -webkit-print-color-adjust


We should implement this, probably as color-adjust.  (I wonder if we should push for the name to be print-color-adjust instead, though.)
According to Google, this is one of 2 missing features that keeps Google Docs from printing "natively", like they do in Chrome. Firefox users have to download a PDF rendition of the document instead.
Assignee: nobody → tschneider
Blocks: 1246805
Comment on attachment 8725088 [details] [diff] [review]
Part 1: Support for adjust-color CSS property

>   // Don't add ANY members to this struct!  We can achieve caching in the rule
>   // tree (rather than the style tree) by letting color stay by itself! -dwh
>   nscolor mColor;                 // [inherited]
>+  uint8_t mAdjust;                // [inherited]

Please see comment (quoted)!
Stupid... What you think about making it a member of nsStyleDisplay instead?
nsStyleDisplay is for non-inherited properties.  I think nsStyleVisibility is probably the best choice.
Move style struct member to nsStyleVisibility.
Attachment #8725088 - Attachment is obsolete: true
Attachment #8725095 - Flags: review?(dbaron)
Attachment #8725096 - Flags: review?(dbaron)
Comment on attachment 8725095 [details] [diff] [review]
Part 1 (v2): Support for adjust-color CSS property

Could you add the commit message to the patch (and also call it color-adjust rather than adjust-color)?

>diff --git a/layout/style/nsComputedDOMStyle.cpp b/layout/style/nsComputedDOMStyle.cpp
>diff --git a/layout/style/nsComputedDOMStyle.h b/layout/style/nsComputedDOMStyle.h

Could you put the new function right before DoGetOpacity in both the .h and .cpp?  (They're currently inconsistent about position.)

>diff --git a/layout/style/nsRuleNode.cpp b/layout/style/nsRuleNode.cpp

>+  const nsCSSValue* adjustValue = aRuleData->ValueForColorAdjust();
>+  if (adjustValue->GetUnit() == eCSSUnit_Enumerated) {
>+    NS_ASSERTION(adjustValue->GetIntValue() == NS_STYLE_COLOR_ADJUST_ECONOMY ||
>+                 adjustValue->GetIntValue() == NS_STYLE_COLOR_ADJUST_EXACT,
>+                 "Unexpected value");
>+    visibility->mColorAdjust = adjustValue->GetIntValue();
>+  }
>+  else if (adjustValue->GetUnit() == eCSSUnit_Initial) {
>+    visibility->mColorAdjust = NS_STYLE_COLOR_ADJUST_ECONOMY;
>+  }
>+  else {
>+    visibility->mColorAdjust = parentVisibility->mColorAdjust;
>+    conditions.SetUncacheable();
>+  }

This should be a call to SetDiscrete instead, and should pass SETDSC_ENUMERATED | SETDSC_UNSET_INHERIT (since you're currently failing to handle the 'unset' value).

>@@ -3184,6 +3186,9 @@ nsChangeHint nsStyleVisibility::CalcDifference(const nsStyleVisibility& aOther)
>       NS_UpdateHint(hint, nsChangeHint_NeedReflow);
>       NS_UpdateHint(hint, nsChangeHint_NeedDirtyReflow); // XXX remove me: bug 876085
>     }
>+    if (mColorAdjust != aOther.mColorAdjust) {
>+      NS_UpdateHint(hint, NS_STYLE_HINT_VISUAL);
>+    }

I'm not sure that this even makes sense.  I think you should just use nsChangeHint_NeutralChange instead, with the explanation that color-adjust only affects media where dynamic changes can't happen.

(You'll need to update nsStyleVisibility::MaxDifference for that, though.)



The one other question is whether this needs to be behind a preference.  I guess it probably doesn't, but you should send an intent to implement and ship to dev-platform, per https://wiki.mozilla.org/WebAPI/ExposureGuidelines .


Marking review- because I'd like to take a look at the revisions, although I think this is fine other than the things I said above.
Attachment #8725095 - Flags: review?(dbaron) → review-
Comment on attachment 8725096 [details] [diff] [review]
Part 2: Force printing background if color-adjust: exact

Generally preferable to have the commit message in the patch for review.

Current style guidelines prefer "* " over " *" when declaring pointers.

r=dbaron with that
Attachment #8725096 - Flags: review?(dbaron) → review+
Addressed review comments.
Attachment #8725095 - Attachment is obsolete: true
Match style guide.
Attachment #8725096 - Attachment is obsolete: true
The patches still don't contain the commit messages. Sorry for that. I'm using SourceTree to create patches, trying to find out how to make it adding commit messages.
Added parachute pref as suggested in response to intent to ship mail.
Attachment #8725358 - Attachment is obsolete: true
Attachment #8727615 - Flags: review?(dbaron)
Comment on attachment 8727615 [details] [diff] [review]
Part 1 (v4): Support for adjust-color CSS property

What about all the other things in comment 8?
Attachment #8727615 - Flags: review?(dbaron) → review-
Sorry, submitted wrong patch previously.
Attachment #8727615 - Attachment is obsolete: true
Attachment #8727949 - Flags: review?(dbaron)
Include commit message.
Attachment #8725359 - Attachment is obsolete: true
Attachment #8727952 - Flags: review?(dbaron)
Comment on attachment 8727949 [details] [diff] [review]
Part 1 (v5): Support for adjust-color CSS property




Please fix the commit message to say color-adjust property instead of adjust-color property.


>diff --git a/layout/style/nsStyleStruct.h b/layout/style/nsStyleStruct.h

>   static nsChangeHint MaxDifference() {
>-    return NS_STYLE_HINT_FRAMECHANGE;
>+    return NS_CombineHint(NS_STYLE_HINT_FRAMECHANGE,
>+                          nsChangeHint_NeutralChange);
>   }

Please write this as
  return NS_STYLE_HINT_FRAMECHANGE |
         nsChangeHint_NeutralChange;

>   static nsChangeHint DifferenceAlwaysHandledForDescendants() {
>     // CalcDifference never returns the reflow hints that are sometimes
>@@ -2072,6 +2073,7 @@ struct nsStyleVisibility
>   uint8_t mPointerEvents;              // [inherited] see nsStyleConsts.h
>   uint8_t mWritingMode;                // [inherited] see nsStyleConsts.h
>   uint8_t mTextOrientation;            // [inherited] see nsStyleConsts.h
>+  uint8_t mColorAdjust;                // [inherited]

Please add the "see nsStyleConsts.h" like all the others



r=dbaron with that
Attachment #8727949 - Flags: review?(dbaron) → review+
Comment on attachment 8727952 [details] [diff] [review]
Part 2 (v3): Force printing background if color-adjust: exact

I already reviewed this; you shouldn't have asked again.
Attachment #8727952 - Flags: review?(dbaron) → review+
Attachment #8727949 - Attachment is obsolete: true
Final patch.
Attachment #8729109 - Attachment is obsolete: true
Keywords: checkin-needed
Fix CSS autocompletion tests by checking other properties then color, since the number of suggestions for color can vary now that there is color-adjust which might or might not be enabled.
Jordan: As discussed, please assign someone to review my changes to the CSS autocompletion tests.
Flags: needinfo?(jsantell)
Comment on attachment 8730353 [details] [diff] [review]
Part 3: Make CSS autocompletion tests pass

Review of attachment 8730353 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/sourceeditor/test/css_autocompletion_tests.json
@@ +18,4 @@
>    [[12, 20], ['none', 'number-input']],
>    [[12, 22], ['none']],
>    [[17, 22], ['hsl', 'hsla']],
> +  [[19, 10], ['background', 'background-attachment', 'background-blend-mode', 

NIT, whitespace
Attachment #8730353 - Flags: review?(gl) → review+
No trailing whitespace.
Attachment #8730353 - Attachment is obsolete: true
Flags: needinfo?(jsantell)
Keywords: checkin-needed
part 3 does not apply cleanly:

patching file devtools/client/inspector/rules/test/browser_rules_completion-new-property_02.js
Hunk #1 FAILED at 23
1 out of 1 hunks FAILED -- saving rejects to file devtools/client/inspector/rules/test/browser_rules_completion-new-property_02.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh bug1209273_3.diff

can you take a look ?
Flags: needinfo?(tschneider)
Keywords: checkin-needed
Carsten: Sorry, should work now.
Attachment #8730754 - Attachment is obsolete: true
Flags: needinfo?(tschneider) → needinfo?(cbook)
Keywords: checkin-needed
Flags: needinfo?(cbook)
I had to back this out because something from the push that included this seems to have broken Windows mochitests like https://treeherder.mozilla.org/logviewer.html#?job_id=24152010&repo=mozilla-inbound
https://hg.mozilla.org/integration/mozilla-inbound/rev/58dc7aaefa9c
Pretty sure this is at fault, unless https://treeherder.mozilla.org/logviewer.html#?job_id=24152031&repo=mozilla-inbound starting at the same time was a total coincidence.
Flags: needinfo?(tschneider)
Pretty sure just a race condition. https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ae25e2ec0f0 looks fine to me.
Flags: needinfo?(tschneider)
Keywords: checkin-needed
You pushed this to Try on top of a parent revision from January, which is way too old to effectively judge what your patch will do now. Please rebase your patches on top of current mozilla-central and then re-push to Try.
Keywords: checkin-needed
You're right. I rebased and re-pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=266e8960f020.
Keywords: checkin-needed
backed this out seems since the last part cause merge conflicts when trying to merge m-i to m-c like warning: conflicts while merging devtools/client/inspector/rules/test/browser_rules_completion-new-property_02.js! (edit, then use 'hg resolve --mark')
Flags: needinfo?(tschneider)
Rebased to m-c.
Attachment #8731405 - Attachment is obsolete: true
Flags: needinfo?(tschneider)
Keywords: checkin-needed
And now I'm hitting conflicts just trying to apply the new part 3 to inbound... :\

Hate to ask it again, but could you try rebasing again? :(
Flags: needinfo?(tschneider)
Rebased to m-i.
Attachment #8736348 - Attachment is obsolete: true
Flags: needinfo?(tschneider)
sorry tobias, had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=24952492&repo=mozilla-inbound
Flags: needinfo?(tschneider)
Seems there was another patch coming in conflicting with mine. Thanks for bearing with me on this! Started a new try run to make sure tests are passing this time: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b45035a9011
Attachment #8736938 - Attachment is obsolete: true
Flags: needinfo?(tschneider)
Keywords: checkin-needed
platform-rel: --- → +
Whiteboard: [platform-rel-Google][platform-rel-GoogleDocs]
Hi guys it looks like the feature has a bug described here: https://bugzilla.mozilla.org/show_bug.cgi?id=1436758
Removing dev-doc-needed keyword since there's a separate bug for documenting this; see bug 1456323.
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.