WIP: Add CSS property to control printing of backgrounds for individual elements.
Created attachment 100931 [details] Patch
Is this feature on standards track? Please also see <http://www.webkit.org/coding/adding-features.html>.
This is just proof-of-concept for me to play with right now, not sure when it will land, or if it will be in this form. Just using this bug to track my changes. If you're interested in the CSS WG discussion of background printing, see: http://lists.w3.org/Archives/Public/www-style/2011Feb/0626.html
Created attachment 101127 [details] Patch
Further, the CSSWG resolved two weeks ago to pursue a proposal for controlling the printing of backgrounds (and potentially other visual effects): <http://lists.w3.org/Archives/Public/www-style/2011Aug/0645.html>
Created attachment 106819 [details] Done except body background printing.
I can't see why backgrounds set on the body of the document and on iframe bodies are never printed. I notice that on Safari right now these backgrounds are never printed, even if the print backgrounds checkbox is ticked on the print dialog. Does anyone know where the code for suppressing backgrounds in that case is located?
Created attachment 108258 [details] Patch
Comment on attachment 108258 [details] Patch This patch isn't ready for review without either feature guards or information about what standards this implements, how this relates to other browsers, etc. The implementation seems reasonable. We don't commit commented out code. I'm not sure what else you'd like commented on.
(In reply to comment #9) > (From update of attachment 108258 [details]) > This patch isn't ready for review without either feature guards or information about what standards this implements, how this relates to other browsers, etc. The implementation seems reasonable. We don't commit commented out code. I'm not sure what else you'd like commented on. The CSSWG resolved to include this property (undecided on final name) here: <http://lists.w3.org/Archives/Public/www-style/2011Aug/0645.html>. It will go into the Paged Media spec.
Eric, can you explain what you feel next steps would be to get this landed? This has caused pain for web developers for years, and is one of the top issues for Google Docs right now (they effectively implement highlighting as a span with a bg color, and lack of this support means that highlighting doesn't print in their docs). Yes, you can save from within docs to a PDF as a workaround, but this is a giant hack, not a great user experience, and negates a lot of the work being done around paged media to give people more control natively over printing without having to resort to generating a PDF. As Tab mentions, I first brought this up on www-style months ago. After much back and forth discussion not really leading anywhere, the WG finally agreed that it would be good to move forward and get implementation experience, which is what we're attempting to do here. What do you feel needs to happen to let this patch land? Is it just the commented out code in CSSStyleSelector.cpp or is there something else that holds this up?
I think there has been some minor misunderstanding here. I should have phrased my comment in the positive, instead of the negative. This patch is very simple and presumably pretty non-controvertial. There are technical difficulties with this patch. The idea is probably sound, just needs justification in the ChagneLog. I will attempt to enumerate the troubles with landing this patch as-is in hopes to give Luke a list of action items to move forward with.
Comment on attachment 108258 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=108258&action=review This concept seems fine. But this patch as-is cannot land without another iteration. It's most important that you include tests (or justification why testing is impossible) as well as comments in the ChangeLog explaining the rational for this change. Those two factors (along with the obvious technical nits) will make a second review simple. > Source/WebCore/ChangeLog:8 > + No new tests. (OOPS!) This patch cannot be landed with this OOPS (the pre-commit hook will refuse it). In general all patches require tests. So this patch will need tests before it can be landed. The ChangeLog should also make it clear to the reviewer why we're adding this functionality, who else implements it, what spec it follows, or if there is no spec yet, link to some working-group discussion about how this is a good idea. Or some justification as to why we're adding it. That doesn't mean to assume that adding it is bad! Just that having a paper trail for any feature makes it much easier for others to undersatnd the reasoning during review or at a later time. > Source/WebCore/css/CSSStyleSelector.cpp:993 > +// documentStyle->setForceBackgroundsToWhite(document->body() && document->body()->renderStyle() && document->body()->renderStyle()->forceBackgroundsToWhite()); > +// if (document->printing() && !settings->shouldPrintBackgrounds()) > +// documentStyle->setForceBackgroundsToWhite(true); As project policy, we do not commit commented out code. I assume you just meant to remove this. > Source/WebCore/css/CSSValueKeywords.in:770 > +economy > +exact I believe it's OK to have these be non-prefixed since the CSS property itself has a -webkit- prefix. > Source/WebCore/rendering/style/RenderStyle.h:205 > - bool _force_backgrounds_to_white : 1; > + ColorAdjust m_colorAdjust : ColorAdjustBits; This scares me. How do we know that we're not overflowing 64-bits when we adjust the ColorAdjustBits size? Do we have compile-time size guards for this object (like we do for some other objects?) If we overflow to the next word, we could end up causing a large memory regression. > Source/WebCore/rendering/style/RenderStyleConstants.h:39 > +static const size_t ColorAdjustBits = 1; Is this the new style to have a constant for this bit size? I agree that it reads nicer, but without compile time object size guards I think we could get ourselves in trouble.
What spec is this from? Can you point me to a draft? If this isn't even in a draft yet, then I don't want it in WebKit yet.
Comment on attachment 108258 [details] Patch "economy" and "exact" and the property name are all terrible. I don't know if this is from a spec or who came up with the terminology, but none of these terms mean anything. An author would have no idea what this property is even supposed to do from these terms.
(In reply to comment #14) > What spec is this from? Can you point me to a draft? If this isn't even in a draft yet, then I don't want it in WebKit yet. As I pointed out in comment #10, the CSSWG resolved to pursue this idea and requests implementation experience. This patch aligns with the preferred solution hashed out over a lot of painful arguing in the WG. The names are WIP. This will go into Paged Media, but Hakon is blocking at the moment. An implementation, as requested by the WG, will help with this.
See thread starting at http://lists.w3.org/Archives/Public/www-style/2011Aug/0436.html
Created attachment 110316 [details] Patch
Comment on attachment 110316 [details] Patch Attachment 110316 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10007854 New failing tests: svg/css/getComputedStyle-basic.xhtml fast/css/getComputedStyle/computed-style.html fast/css/getComputedStyle/computed-style-without-renderer.html
(In reply to comment #16) > (In reply to comment #14) > > What spec is this from? Can you point me to a draft? If this isn't even in a draft yet, then I don't want it in WebKit yet. > > As I pointed out in comment #10, the CSSWG resolved to pursue this idea and requests implementation experience. This patch aligns with the preferred solution hashed out over a lot of painful arguing in the WG. The names are WIP. > I strongly disagree with introducing this property into WebKit and largely agree with Hakon. Hint properties just don't work well. As sleazy Web sites discover the property, they'll start setting it on elements to force the backgrounds to print, and then the UI default will just have to shift from "honor the property" to "never honor the property" again. Then users will have to find and toggle some obscure switch in Preferences for the property to even be useful. > This will go into Paged Media, but Hakon is blocking at the moment. An implementation, as requested by the WG, will help with this. What data are you trying to collect? Are you wanting to ship it with Chrome to see what happens? Are you wanting to produce nightlies with it turned on? I guess I'm not clear on how an implementation helps much when there's a fundamental disagreement regarding property abuse. One final note: please be careful about making posts with language like "WebKit is interested in..." In this case, you really meant "Google is interested in..." and there's no reason not to be honest about it. I say "Apple wants..." or "Apple is interested in..." all the time when talking about properties that we're working on, and don't try to speak for all of WebKit unless I know there's consensus.
(In reply to comment #20) > As sleazy Web sites discover the property, they'll start setting it on elements to force the backgrounds to print, and then the UI default will just have to shift from "honor the property" to "never honor the property" again. Then users will have to find and toggle some obscure switch in Preferences for the property to even be useful. Sleazy web-sites, for some definition of "sleazy", can already do this. Google Maps, for example, achieves striped backgrounds on its printed directions by positioning an element behind alternating steps, and setting a large border of the chosen color on it. Since borders are printed, it looks like a background. You can get images the same way by using the <img> tag. This is obviously pretty ridiculous. Since there are good use-cases for this functionality, it would be cool to expose it. It's possible that it will end up becoming just as polluted as 'background' already is. If it does, shrug, we tried, and we'll never try again. However, the initial pollution was basically certain, while this is not - it's got a better chance of succeeding. > > This will go into Paged Media, but Hakon is blocking at the moment. An implementation, as requested by the WG, will help with this. > > What data are you trying to collect? Are you wanting to ship it with Chrome to see what happens? Are you wanting to produce nightlies with it turned on? I guess I'm not clear on how an implementation helps much when there's a fundamental disagreement regarding property abuse. The data we're looking for is "how to get Hakon to stop blocking". A decent fraction of the WG wants this, and another fraction at least doesn't hate it. This was the compromise from a pretty annoying argument - get an impl first, then it'll go into a spec. > One final note: please be careful about making posts with language like "WebKit is interested in..." In this case, you really meant "Google is interested in..." and there's no reason not to be honest about it. I say "Apple wants..." or "Apple is interested in..." all the time when talking about properties that we're working on, and don't try to speak for all of WebKit unless I know there's consensus. Sorry, force of habit. I usually consider myself working on Webkit, with the Chrome affiliation a mere formality.
I'd just like to take a moment to advocate for the usefulness of this property. There are, like it or not, an exceptional number of places that background is used to convey important information to the user. There are also many places that backgrounds are used in a purely aesthetic way, and are therefore not required to understand the meaning of the page. Both of these are frequently and necessarily used within a single page. For example, even on this page, a green background on style or red background on cr-linux convey important information, while the grey background on add an attachment adds litte information. Simply toggling the backgrounds on and off for the entire page does not sufficiently capture this nuanced use of backgrounds to convey information to the user. Therefore there is a real and important use case for adding a property that encapsulates the knowledge the web author has about which backgrounds convey information, and which are purely aesthetic. > As sleazy Web sites discover the property, they'll start setting it on elements to force the backgrounds to print, and then the UI default will just have to shift from "honor the property" to "never honor the property" again. Then users will have to find and toggle some obscure switch in Preferences for the property to even be useful. I don't concede this adversarial view of web authors, particularly for web pages where the user has already decided that they find the information useful and require a hard copy. Perhaps naively, I don't think we should prevent web authors from improving the usefulness and accessibility of their content by allowing offline consumption on the basis that a "sleazy Web site" wants to waste the user's ink. I do concede that in order to maintain precisely the same functionality that it has right now, Safari would need to use a tri-state of "print all backgrounds", "print no backgrounds" and "print only important backgrounds", however I anticipate that ultimately "print only important backgrounds" will become vastly more likely and useful than the other options - to the point that in Chrome we intend to make it the only option.
For what it's worth, we've been trying to solve this problem in Chrome since before the February email to css-style (http://lists.w3.org/Archives/Public/www-style/2011Feb/0626.html). We've done a lot of work in Docs to fix up printing from HTML so that we don't have to generate a PDF and get people to print the PDF (which is a giant hack). The same problem presents itself in other Google apps. People don't connect "printing background colors and images" with "I have a word in this sentence highlighted and it's not printing out that way, wtf?!?!". From our perspective, we can't expect users to make that connection and figure out they need to check the box. I suspect very few users could even find the box, much less understand what it means. In Chrome, a group of people wanted to solve this by just flipping the pref to "always print background colors and images." I personally felt this would be a bad solution as we would inevitably get random crap printing that the site author never intended to print (many people either don't consider what happens when you print, or they assume that all browsers by default don't print backgrounds.) I argued for getting some better way to let site authors express their intent -- that they have considered printing and that something really ought to be printed. This seemed a better way forward. Yes, malicious / crap sites can still do the "wrong" thing, but they can cause bad print output in tons of other ways simpler than this. This is the best approach we could come up with to resolve long standing issues around printing, and we would really like to be able to move forward with the approach and see what happens. It solves a very real need that our users and users of many other websites are having, without side effects to rendering of existing content.
How do we distinguish between outputting to a printer (and thus preferring to omit backgrounds), and printing to a PDF, which may never be printed?
(In reply to comment #24) > How do we distinguish between outputting to a printer (and thus preferring to omit backgrounds), and printing to a PDF, which may never be printed? You can toggle setShouldPrintBackgrounds on the Settings object if you want to force them on.
Thanks for the examples of where you might need this property. I admit that those are compelling use cases. Those use cases have convinced me that it is ok to proceed with experimentation here, although I would like us to pick a syntax that is a bit less bizarre. color-adjust: economy | exact is - in my opinion - needlessly obfuscating the feature. We know this is about saving ink and is print-specific, so can't we get "printer" or "print" into the property name to make that clear? Even just printer-color-adjust would be a big improvement. Are we avoiding using the term "background" explicitly because we might want to avoid printing other things like box-shadow? I don't think we necessarily have to fear the term "background" and think printer-background-adjust would be even better.
(In reply to comment #26) > Thanks for the examples of where you might need this property. I admit that those are compelling use cases. > > Those use cases have convinced me that it is ok to proceed with experimentation here, although I would like us to pick a syntax that is a bit less bizarre. Awesome, thanks Hyatt! > color-adjust: economy | exact > > is - in my opinion - needlessly obfuscating the feature. We know this is about saving ink and is print-specific, so can't we get "printer" or "print" into the property name to make that clear? > > Even just printer-color-adjust would be a big improvement. > > Are we avoiding using the term "background" explicitly because we might want to avoid printing other things like box-shadow? I don't think we necessarily have to fear the term "background" and think printer-background-adjust would be even better. We actually already know for a *fact* that this will be used for more things than just backgrounds - IE currently suppresses box-shadow on printing, and has indicated that it should toggle with this property as well. Other properties such as border-image have a strong chance of making their way onto the "blocked unless important" list as well. So, I don't think that 'background' should appear in the property's name. I advocated for a somewhat further generic name on the assumption that platforms other than printing, with their own feature economy that differs from typical screens, may want to use this. For example, an AMOLED screen. I wouldn't be horribly sad if this line of argument got rejected, though.
Created attachment 111884 [details] Patch
Comment on attachment 111884 [details] Patch Attachment 111884 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10182684 New failing tests: svg/css/getComputedStyle-basic.xhtml fast/css/getComputedStyle/computed-style.html fast/css/getComputedStyle/computed-style-without-renderer.html
(In reply to comment #29) > (From update of attachment 111884 [details]) > Attachment 111884 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/10182684 > > New failing tests: > svg/css/getComputedStyle-basic.xhtml > fast/css/getComputedStyle/computed-style.html > fast/css/getComputedStyle/computed-style-without-renderer.html I think the chromium queue has failed to patch for some reason I don't understand. I don't think there is anything wrong with the patch though?
> I think the chromium queue has failed to patch for some reason I don't understand. I don't think there is anything wrong with the patch though? Have you tried running those tests? It seems very likely that your patch will cause them to fail.
Created attachment 112131 [details] Patch
Comment on attachment 112131 [details] Patch Lacking any further objections from others, this change looks reasonable to me.
Created attachment 113258 [details] Patch for landing
Comment on attachment 113258 [details] Patch for landing Clearing flags on attachment: 113258 Committed r99022: <http://trac.webkit.org/changeset/99022>
All reviewed patches have been landed. Closing bug.