1237101 - Google Docs' toolbar button images are broken after enabling -webkit CSS prefixes, on HiDPI displays

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Chris Peterson [:cpeterson]]

[Tracking Requested - why for this release]:

Google Docs is broken!

STR:
1. Open a Google Doc, such as https://docs.google.com/document/d/1kBx98ulj5V5M9Zdeamy7v6ofZXX3yPziAf0V27A64Mo/ or https://docs.google.com/document/d/11UiFaZT2K-CObCZZCpHQ717H0Dx7eGZItpgUgn2W1LA/
2. Look at the Google Doc's tool bar buttons (or look at the screenshot attached to this bug).

RESULT:
The button images are garbled!

This is a regression from WebKit CSS prefix bug 1213126. I bisected the regression to this pushlog:

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=31edd1840c5f651b5dbf182fdb7f04fe98c88d86&tochange=9fbf850dc78d7197132a298f9ec0270c7de16a13

Comment 1

[Mike Taylor [:miketaylr]]

It seems like this is related to us understanding -webkit-min-device-pixel-ratio and getting some new CSS nested in a media query. Will be able to take a look tomorrow more closely, if nobody beats me to it.

Comment 2

[Daniel Holbert [:dholbert]]

Chris, could you post your screenshot? I can't reproduce any weirdness here (perhaps because I don't have a HiDPI screen, given comment 1).

Comment 3

[Chris Peterson [:cpeterson]]

Attachment: screenshot.png

Sorry. I forgot to attach the screenshot! :)

Comment 4

[Sören Hentzschel]

Attachment: screenshot

(In reply to Daniel Holbert [:dholbert] from comment #2)
> Chris, could you post your screenshot? I can't reproduce any weirdness here
> (perhaps because I don't have a HiDPI screen, given comment 1).

I am not Chris, but I have a screenshot. :)

Comment 5

[Chris Peterson [:cpeterson]]

This is a HiDPI problem. If I drag the Google Docs window from my MacBook Pro's builtin HiDPI display to a non-HiDPI external monitor, the button images look correct. If I drag the window back to the builtin display, then the button images are garbled again.

Comment 6

[Mike Taylor [:miketaylr]]

Bah, us getting access to the HiDPI assets in the media query uncovers another non-standard WebKit-ism (at least I think, maybe it's in CSS4 or something): allowing generated content for non-pseudo elements.

You can reproduce the same bug in Chrome if you disable the content property on the HiDPI .docs-icon-img style: https://cloudup.com/cyWlXkFfbMB

Here's the current situation for us, for a single icon:

<div class="docs-icon-img-container docs-icon-img docs-icon-enter-compact" aria-hidden="true">&nbsp;</div>

.docs-icon-img:before {
  content: url(//ssl.gstatic.com/docs/common/jfk_sprite138.png);
}

.docs-icon-enter-compact {
    left: 0;
    top: -2418px;
}

@media screen and (-webkit-min-device-pixel-ratio:2) {
  .docs-icon-img {
    content: url(//ssl.gstatic.com/docs/common/jfk_sprite_hdpi76.png);
  }

  .docs-icon-enter-compact {
    left: -21px;
    top: -921px;
  }
}

We ignore content on non-pseudo elements, so the top/left inside the MQ are just offsetting the lodpi sprite.

So the million dollar question is, what's the easiest fix and what are the odds of getting Google to update their CSS? :/

Comment 7

[Daniel Holbert [:dholbert]]

One short-term fix might be to just drop support for "-webkit-device-pixel-ratio" (bug 1176968), given that Google Docs likely has higher traffic than the sites referenced in the webcompat issues there...

Comment 8

[Mike Taylor [:miketaylr]]

Yep, agreed.

Comment 9

[Mike Taylor [:miketaylr]]

Here's a simple test case of the bug, https://miketaylr.com/bzla/1237101.html

Comment 10

[Daniel Holbert [:dholbert]]

(that testcase is only valid on HiDPI machines, of course. It renders as green on my machine, because my machine fails the DPI test.)

Comment 11

[Karl Dubost💡 :karlcow]

just a note about `-webkit-device-pixel-ratio` from bug 1176968 are happening in scenario such as:

> @media only screen and (max-width:640px) and (-webkit-min-device-pixel-ratio: 1.5) { }

Maybe indeed the weight of Google docs is more important. Though it might be worth to try to convince Google to fix it. It's beneficial for them. And Google is not one but many teams. With some of the teams they fix.

Comment 12

[Mike Taylor [:miketaylr]]

(In reply to Daniel Holbert [:dholbert] from comment #10)
> (that testcase is only valid on HiDPI machines, of course. It renders as
> green on my machine, because my machine fails the DPI test.)

Yeah, thanks for pointing that out. (I'm not even sure what's passing or failing, given this is all happening in the context of non-standard prefix twilight...).


(In reply to Karl Dubost :karlcow from comment #11)
> Maybe indeed the weight of Google docs is more important. Though it might be
> worth to try to convince Google to fix it. It's beneficial for them. And
> Google is not one but many teams. With some of the teams they fix.

Yes, I think we should reach out to Google and ask them to fix. We should also file a bug on Blink and tag it as a hotlist-interop issue (if there isn't one out there already).

Comment 13

[Cameron McCormack (:heycam)]

FWIW implementing content on elements is bug 215083.  I agree it's not an easy fix to implement that.

Comment 14

[Daniel Holbert [:dholbert]]

Even in the absence of Firefox having webkit-prefix support, this bug still represents something that we'd like Google to fix -- they're hiding their high-res icons behind a a webkit-specific media query, and ideally they shouldn't do that.

IMO the best way to improve things would be for them to add a "@media screen and (min-resolution: 2dppx)" media-query  right after their webkit-specific one, with interoperable code to choose the high-res icon. (interoperable = probably using ".docs-icon-img:before" instead of just ".docs-icon-img").

If they did that, then I expect they wouldn't need to bother changing their -webkit-min-device-pixel-ratio media-query block, if they don't want to, since the standards-based fallback would come afterwards and would override it for us (even with webkit prefix support turned on).

Comment 15

[Mike Taylor [:miketaylr]]

Karl, can you do outreach to Google about this please?

Comment 16

[Mike Taylor [:miketaylr]]

(In reply to Cameron McCormack (:heycam) from comment #13)
> FWIW implementing content on elements is bug 215083.  I agree it's not an
> easy fix to implement that.

(possibly wrong bug to ask, but...) Would it be less complex to only implement in the same way that Chrome and Safari do, i.e., only support div {content: url(<image>)}? 

I seem to recall Opera had a ton of compat issues due to random styles using div {content: ''; background-image: url(<image>)}, so I'm not sure that's long-tail web compatible anyways -- would have to dig up my old opera email archive to look.

Comment 17

[Cameron McCormack (:heycam)]

Maybe, for a particularly hacky solution.

We need to have the generated image influence the size of the box, which means we can't just map it to a background-image internally.  The real solution is to use the same generated content mechanism that ::before and ::after use, but that's pretty tied in with the way those pseudos work currently.  The hacky way would be to indeed map it to background-image internally, and to change nsBlockFrame's reflow code to look at the size of the synthesized background-image.  We probably should suppress box generation for the element's children too, although in this test the content is just   so we could get away without it here.

Daniel, WDYT about this if outreach fails?

Comment 18

[Daniel Holbert [:dholbert]]

I'm definitely comfortable with hacks & "works well enough for our purposes" for explicitly vendor-prefixed stuff.

I'm less comfortable with it for unprefixed stuff like "content" -- that would kind of violate our web API exposure guidelines (https://wiki.mozilla.org/WebAPI/ExposureGuidelines#Suggested_implementation_process ) .  The CSS3 content spec explicitly says it's not ready to be implemented; so in a perfect world, any work that we do on it at this stage would be preffed off, and we'd only pref it on when the spec & the feature were both ready enough to be exposed to the web.

For now, I'm leaning towards bug 1237720 as a short-term fix (i.e. turn off -webkit-min-device-pixel-ratio support).  AFAIK, that won't break too many sites (and it'll only break them in a "lower quality icons" kind of way).  Hopefully we'll be successful with google-docs outreach; but if we aren't, we can have a discussion about whether implementing a halfway solution to bug 215083 would be worth it (and perhaps see if we can get more concrete data about how much of the web depends on the "content" property & how much our halfway solution would help/hurt).

Comment 19

[Mike Taylor [:miketaylr]]

This is fixed now in Nightly (thanks to Daniel) -- I'm going to move to Tech Evangelism and we can use this bug to track the HiDPI images aspect.

Comment 20

[Liz Henry (:lizzard) (relman/hg->git project)]

As I understand it the icons are no longer super garbled for this site but may be lower resolution for Google Docs and possibly other sites. 

Tracking for 46 since that still sounds like a regression from enabling layout.css.prefixes.webkit by default, though we want other sites to fix how they use webkit

Comment 21

[Daniel Holbert [:dholbert]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #20)
> As I understand it the icons are no longer super garbled for this site but
> may be lower resolution for Google Docs and possibly other sites.

Correct-ish -- when we enabled webkit prefix support, that made us start seeing a section of Google Docs CSS which provides high-res icons (yay!) but also some additional stuff that breaks the icons (boo!).  So, in bug 1237720, we reverted the specific webkit prefixing feature that was getting us this section of CSS and breaking Google docs.  So now we're back to the same lower-res icons that we were receiving before we enabled prefix support.

> Tracking for 46 since that still sounds like a regression from enabling layout.css.prefixes.webkit

Nope, not a regression at this point (we're back to the same state we were in before enabling that pref).

Mike, to avoid further confusion, I'm going to revert your morph-to-tech-evang in comment 19 -- I spun off bug 1239922 for the tech evang aspect here, as a nice clean starting point.  And then, I'll resolve this bug here as FIXED by bug 1237720.

Comment 22

[Daniel Holbert [:dholbert]]

(Also reverting tracking+, since I think it was set based on an incorrect assumption, per second quote in comment 21.)

Resolving as FIXED by bug 1237720.

Comment 23

[Mike Taylor [:miketaylr]]

Thanks, sorry for the confusion. 

Nothing has changed here, we've always been served low-res icons (we only just discovered as a result of layout.css.prefixes.webkit).

Comment 24

[Daniel Holbert [:dholbert]]

[restoring the old bug-summary, since the tech evang issue w/ Google Docs CSS is tracked in bug 1239922]

Comment 25

[Karl Dubost💡 :karlcow]

To the best of my knowledge, they mostly fixed it!

→ grep -hi 'webkit-min-device-pixel-ratio' ~/Downloads/google1.css ~/Downloads/google2.css 

@media (-webkit-min-device-pixel-ratio:2.0), (min-resolution:192dpi) {
@media (min-resolution:144dpi), (-webkit-min-device-pixel-ratio:1.5) {
@media (min-resolution:1.25dppx), (-webkit-min-device-pixel-ratio:1.25), (min-device-pixel-ratio:1.25) {
@media (min-resolution:1.25dppx), (-webkit-min-device-pixel-ratio:1.25), (min-device-pixel-ratio:1.25) {
@media (min-resolution:1.25dppx), (-webkit-min-device-pixel-ratio:1.25), (min-device-pixel-ratio:1.25) {
@media (min-resolution:1.25dppx), (-o-min-device-pixel-ratio:5/4), (-webkit-min-device-pixel-ratio:1.25), (min-device-pixel-ratio:1.25) {
@media (min-resolution:1.25dppx), (-webkit-min-device-pixel-ratio:1.25), (min-device-pixel-ratio:1.25) {
@media (min-resolution:1.25dppx), (-webkit-min-device-pixel-ratio:1.25), (min-device-pixel-ratio:1.25) {
@media (min-resolution:1.25dppx), (-webkit-min-device-pixel-ratio:1.25), (min-device-pixel-ratio:1.25) {
@media (min-resolution:1.25dppx), (-o-min-device-pixel-ratio:5/4), (-webkit-min-device-pixel-ratio:1.25), (min-device-pixel-ratio:1.25) {
@media (min-resolution:1.25dppx), (-o-min-device-pixel-ratio:5/4), (-webkit-min-device-pixel-ratio:1.25), (min-device-pixel-ratio:1.25) {


# This one to fix
@media screen and (-webkit-min-device-pixel-ratio:2) { }
related to the icons, but this is still working.

I set:

layout.css.dpi;3
layout.css.prefixes.device-pixel-ratio-webkit;true


And open a Google doc with a toolbar and had no issues.

Could someone else double check and if indeed it's not an issue anymore we could reactivate it, because it creates issues on other site such as Etsy and Quora.

Comment 26

[Mike Taylor [:miketaylr]]

Indeed, it does seem fixed for me with layout.css.prefixes.device-pixel-ratio-webkit=true. Daniel, do you think we should experiment with turning that back on, perhaps Nightly only for a release or two?

Comment 27

[Daniel Holbert [:dholbert]]

That's great news!  It sounds like maybe we can close out the Tech Evang bug then? (bug 1239922)

I filed bug 1444139 on re-enabling this feature.

Comment 28

[Mike Taylor [:miketaylr]]

Awesome, thanks Daniel.

Comment 29

[Daniel Holbert [:dholbert]]

Following up on comment 26 - comment 28: apparently Google Docs findbar is still broken if we enable this feature (see bug 1239922 comment 13), so I backed out the pref-flip in bug 1444139 (and reopened both of those bugs).

Anyway -- further discussion should probably go in bug 1239922 (about GDocs itself) or bug 1444139 (about the feasability/timing of the pref flip).