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

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

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

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

Comment 1

[Tobias Schneider [:tobytailor]]

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.

Comment 2

[Tobias Schneider [:tobytailor]]

Attachment: Part 1: Support for adjust-color CSS property

Comment 3

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

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)!

Comment 4

[Tobias Schneider [:tobytailor]]

Stupid... What you think about making it a member of nsStyleDisplay instead?

Comment 5

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

nsStyleDisplay is for non-inherited properties.  I think nsStyleVisibility is probably the best choice.

Comment 6

[Tobias Schneider [:tobytailor]]

Attachment: Part 1 (v2): Support for adjust-color CSS property

Move style struct member to nsStyleVisibility.

Comment 7

[Tobias Schneider [:tobytailor]]

Attachment: Part 2: Force printing background if color-adjust: exact

Comment 8

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

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.

Comment 9

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

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

Comment 10

[Tobias Schneider [:tobytailor]]

Attachment: Part 1 (v3): Support for adjust-color CSS property

Addressed review comments.

Comment 11

[Tobias Schneider [:tobytailor]]

Attachment: Part 2 (v2): Force printing background if color-adjust: exact

Match style guide.

Comment 12

[Tobias Schneider [:tobytailor]]

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.

Comment 13

[Tobias Schneider [:tobytailor]]

Attachment: Part 1 (v4): Support for adjust-color CSS property

Added parachute pref as suggested in response to intent to ship mail.

Comment 14

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

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?

Comment 15

[Tobias Schneider [:tobytailor]]

Attachment: Part 1 (v5): Support for adjust-color CSS property

Sorry, submitted wrong patch previously.

Comment 16

[Tobias Schneider [:tobytailor]]

Attachment: Part 2 (v3): Force printing background if color-adjust: exact

Include commit message.

Comment 17

[Tobias Schneider [:tobytailor]]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=261c8960d8f4

Comment 18

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

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

Comment 19

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

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.

Comment 20

[Tobias Schneider [:tobytailor]]

Attachment: Part 1 (v6): Support for color-adjust CSS property

Comment 21

[Tobias Schneider [:tobytailor]]

Attachment: Part 1 (v6): Support for color-adjust CSS property

Final patch.

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1901109091be
https://hg.mozilla.org/integration/mozilla-inbound/rev/85f7dbb76c15

Comment 23

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/b1389bbe138a for devtools failures like https://treeherder.mozilla.org/logviewer.html#?job_id=23523114&repo=mozilla-inbound and https://treeherder.mozilla.org/logviewer.html#?job_id=23523118&repo=mozilla-inbound

Comment 24

[Tobias Schneider [:tobytailor]]

Attachment: Part 3: Make CSS autocompletion tests pass

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.

Comment 25

[Tobias Schneider [:tobytailor]]

Jordan: As discussed, please assign someone to review my changes to the CSS autocompletion tests.

Comment 26

[Gabriel Luong [:gl] (ΦωΦ)]

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

Comment 27

[Tobias Schneider [:tobytailor]]

Attachment: Part 3 (v2): Make CSS autocompletion tests

No trailing whitespace.

Comment 28

[Tobias Schneider [:tobytailor]]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c44c9aa5131d

Comment 29

[Tobias Schneider [:tobytailor]]

Actual try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b6c70c49a6e

Comment 30

[Carsten Book [:Tomcat]]

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 ?

Comment 31

[Tobias Schneider [:tobytailor]]

Attachment: Part 3 (v3): Make CSS autocompletion tests

Carsten: Sorry, should work now.

Comment 32

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6e2e6b0c7d8e
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb8b3ab41c82
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c6650c7e237

Comment 33

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 34

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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.

Comment 35

[Tobias Schneider [:tobytailor]]

Pretty sure just a race condition. https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ae25e2ec0f0 looks fine to me.

Comment 36

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 37

[Tobias Schneider [:tobytailor]]

You're right. I rebased and re-pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=266e8960f020.

Comment 38

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/073149564592
https://hg.mozilla.org/integration/mozilla-inbound/rev/231e5d92a129
https://hg.mozilla.org/integration/mozilla-inbound/rev/afbddd5fb626

Comment 39

[Carsten Book [:Tomcat]]

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')

Comment 40

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a587c254021
https://hg.mozilla.org/integration/mozilla-inbound/rev/2be0bf743c62
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b0efb1e937b

Comment 41

[Tobias Schneider [:tobytailor]]

Attachment: Part 3 (v4): Make CSS autocompletion tests

Rebased to m-c.

Comment 42

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if 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? :(

Comment 43

[Tobias Schneider [:tobytailor]]

Attachment: Part 3 (v5): Make CSS autocompletion tests

Rebased to m-i.

Comment 44

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d5a9ff0ae110
https://hg.mozilla.org/integration/mozilla-inbound/rev/11f8ba2d1e85
https://hg.mozilla.org/integration/mozilla-inbound/rev/90312ff603de

Comment 45

[Carsten Book [:Tomcat]]

sorry tobias, had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=24952492&repo=mozilla-inbound

Comment 46

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2eac8f080d52
https://hg.mozilla.org/integration/mozilla-inbound/rev/238bfb42ed6b
https://hg.mozilla.org/integration/mozilla-inbound/rev/c50d5221bef5

Comment 47

[Tobias Schneider [:tobytailor]]

Attachment: Part 3 (v6): Make CSS autocompletion tests pass

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

Comment 48

[Tobias Schneider [:tobytailor]]

And an actually useful try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e42f9ef5643b :)

Comment 49

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d20c3ae26cad
https://hg.mozilla.org/integration/mozilla-inbound/rev/99cfe49eaf64
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c7548ca0a5c

Comment 50

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d20c3ae26cad
https://hg.mozilla.org/mozilla-central/rev/99cfe49eaf64
https://hg.mozilla.org/mozilla-central/rev/1c7548ca0a5c

Comment 51

[dovlet84]

Hi guys it looks like the feature has a bug described here: https://bugzilla.mozilla.org/show_bug.cgi?id=1436758

Comment 52

[Eric Shepherd [:sheppy]]

Removing dev-doc-needed keyword since there's a separate bug for documenting this; see bug 1456323.