Closed Bug 1237101 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox45 --- unaffected
firefox46 --- fixed

People

(Reporter: cpeterson, Assigned: dholbert)

References

Details

(Keywords: regression)

Attachments

(2 files)

[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
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.
Flags: needinfo?(miket)
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).
Attached image screenshot.png
Sorry. I forgot to attach the screenshot! :)
Attached image 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. :)
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.
Summary: Google Docs' toolbar button images are broken after enabling -webkit- CSS prefixes → Google Docs' toolbar button images are broken after enabling -webkit- CSS prefixes, on HiDPI displays
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? :/
Flags: needinfo?(miket)
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...
Yep, agreed.
Here's a simple test case of the bug, https://miketaylr.com/bzla/1237101.html
(that testcase is only valid on HiDPI machines, of course. It renders as green on my machine, because my machine fails the DPI test.)
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.
(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).
FWIW implementing content on elements is bug 215083.  I agree it's not an easy fix to implement that.
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).
Karl, can you do outreach to Google about this please?
Flags: needinfo?(kdubost)
(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.
Flags: needinfo?(cam)
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 &nbsp; so we could get away without it here.

Daniel, WDYT about this if outreach fails?
Flags: needinfo?(cam) → needinfo?(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).
Flags: needinfo?(dholbert)
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.
Component: CSS Parsing and Computation → Desktop
Product: Core → Tech Evangelism
Summary: Google Docs' toolbar button images are broken after enabling -webkit- CSS prefixes, on HiDPI displays → Google Docs' toolbar high-res icons hidden behind a a webkit-specific media query
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
(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.
Flags: needinfo?(kdubost)
(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.
Status: NEW → RESOLVED
Closed: 4 years ago
Component: Desktop → CSS Parsing and Computation
Product: Tech Evangelism → Core
Resolution: --- → FIXED
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).
[restoring the old bug-summary, since the tech evang issue w/ Google Docs CSS is tracked in bug 1239922]
Summary: Google Docs' toolbar high-res icons hidden behind a a webkit-specific media query → Google Docs' toolbar button images are broken after enabling -webkit CSS prefixes, on HiDPI displays
Assignee: nobody → dholbert
Target Milestone: --- → mozilla46
Version: unspecified → Trunk
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.
Flags: needinfo?(miket)
Flags: needinfo?(miket) → webcompat?
Flags: needinfo?(miket)
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?
Flags: needinfo?(miket) → needinfo?(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.
Flags: needinfo?(dholbert)
Awesome, thanks Daniel.
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).
You need to log in before you can comment on or make changes to this bug.