1316236 - graphic glitch on http://imgur.com after upload image

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P3)

Description

[Alice0775 White]

Build Identifier:
https://hg.mozilla.org/releases/mozilla-aurora/rev/d9cfe58247e85c05ad98a4e60045bbdd62e0ec2b
Mozilla/5.0 (Windows NT 10.0; WOW64; rv:51.0) Gecko/20100101 Firefox/51.0 ID:20161108004019

Affected to Aurora51.0a2 and Nightly52.0a1.
Not affected to 50.0rc2.

Reproducible: 80%

Steps To Reproduce:
1. open http://imgur.com (no need login)
2. Click [New post] button at the top of page so that drop target will be shown
3. Drag and drop a image file from Explorer or desktop to the drop target
   or
   Click [Browse] button and chose a image file and then click [Open] in file picker
4. Observe graphic

Actual Results:
graphic glitch.
See screencast https://www.youtube.com/watch?v=oDpeg98dgJ4

Comment 1

[Alice0775 White]

Attachment: screenshot

Comment 2

[Alice0775 White]

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=4f30550d3edb5c60f07cea7259652722deccc094&tochange=adb44576fbe4936fca68eaa91b30c25858b68c00

Triggered by: Bug 1274158

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

I can reproduce this on Windows with Firefox 51. Probably having a too small perspective reveals some graphics issue.

Comment 4

[Alice0775 White]

Attachment: semi-reduced html

STR:
1. Click [click]
   observe graphics
2. Repeat Step1

Actual Results:
Graphic glitch

Comment 5

[Alice0775 White]

Attachment: screenshot against the semi-reduced html

Though the graphics glitch is quite varied.

Comment 6

[Milan Sreckovic [:milan] (needinfo for best results)]

From the point of view of bug 1274158, we have succeeded in making zero perspective behave as almost-but-not-quite-zero.  For example, modify the reduced testcase to use 1px instead of 0 for the perspective value, and the same thing happens in all browsers.  Chrome et al. behave differently at perspective set to 0, but behave the same at perspective set to 1px.  If we accept the premise of bug 1274158 in that we want zero perspective to be used, not ignored, then this is the consistent behaviour.

I'm going to close this as WONTFIX - if we want to go back to behaviour pre bug 1274158, we should probably reopen the discussion in that bug - it has the right audience.  I will put a note in there about this bug.

Comment 7

[Milan Sreckovic [:milan] (needinfo for best results)]

Let me reopen this while we're figuring if we should just back out bug 1274158, or if this is a tech-evangelism issue.

Comment 8

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #3)
> I can reproduce this on Windows with Firefox 51. Probably having a too small
> perspective reveals some graphics issue.

I haven't looked at why these show up at values < 5px, but they do on all browsers.  This is really a question of backing out bug 1274158 or tech-evangelism.

Comment 9

[Alice0775 White]

(In reply to Milan Sreckovic [:milan] from comment #6)
> From the point of view of bug 1274158, we have succeeded in making zero
> perspective behave as almost-but-not-quite-zero.  For example, modify the
> reduced testcase to use 1px instead of 0 for the perspective value, and the
> same thing happens in all browsers.  Chrome et al. behave differently at
> perspective set to 0, but behave the same at perspective set to 1px.

After finish animation, The glitch remains on Firefox until scrolling page/switching tabs, but not on the other browser.
So, at least, graphic problem is actual bug on Firefox.

Comment 10

[Milan Sreckovic [:milan] (needinfo for best results)]

I believe you're talking about imgur scenario, right?  Yes, since that one is using perspective(0), it will behave differently in Firefox from everywhere else.  We designed to behave differently, as of bug 1274158.  If we want to match what other browsers do, with perspective(0), we have to undo bug 1274158, or at least change what we're doing.

The comment 8 actually talks about replacing perspective(0) with perspective(1px) or perspective(2px)  in the "semi-reduced.html" example.  In Firefox, the behaviour is the same (close).  In Chrome, the modified file then behaves the same as Firefox.

So, we have to choose - we either want perspective(0) to behave like perspective("close to zero") - which is the change that bug 1274158 introduced, or we want it to behave like Chrome.  Right now, we have the consistency of "zero behaves like it's close to zero"; Chrome does not.  That may not be a good enough reason to actually keep the modified behaviour, but it comes down to the CSS handling changes in bug 1274158.

Comment 11

[Alice0775 White]

(In reply to Milan Sreckovic [:milan] from comment #10)
> I believe you're talking about imgur scenario, right? 
> behaviour, but it comes down to the CSS handling changes in bug 1274158.

No, Both imgur scenario and semi-reduced html.

Tested with semi-reduced html(but perspective(0) change 0 to 1px):
During animation: The glitch is on all browser 
After completion animation: The glitch remains only Firefox(after scrolling the page, the glitch is gone).  The other browser is no glitch.

Comment 12

[Alice0775 White]

s/after scrolling the page/after scrolling the page, resizing browser or switching tabs/

Comment 13

[Xidorn Quan [:xidorn] UTC+10]

So I think there are actually two issues:
1. the glitch during animation, which is caused by bug 1274158;
2. the glitch after the animation finishes.

I think the first issue is a interoperability issue which is probably related to the spec. The second is an actual pre-existing graphics issue.

What I don't really understand is, what does imgur actually expect from using perspective(0) here.

Comment 14

[Xidorn Quan [:xidorn] UTC+10]

Let's turn this into a Tech Evangelism and open a new bug for the glitch leftover issue.

Comment 15

[Mike Taylor [:miketaylr]]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #3)
> I can reproduce this on Windows with Firefox 51. Probably having a too small
> perspective reveals some graphics issue.

Note: this doesn't reproduce on OSX. 

But here's a demo w/ perspective(0) that glitches out on my mac as well: http://codepen.io/miketaylr/pen/LbGvYJ

in global.css imgur has a bunch of animations (removed the prefixed versions):

@keyframes flipin {
  from {
    transform:perspective(500px) translate(-50%,0) rotateX(-90deg) rotateY(0);
    opacity:.5
  }
  to {
    transform:perspective(0) translate(-50%,115%) rotateX(0) rotateY(0);
    opacity:1
  }
}

@keyframes bounceup {
  from {
    transform:perspective(0) translate(-50%,115%) rotateX(0) rotateY(0);
    opacity:1
  }
  to {
    transform:perspective(0) translate(-50%,98%) rotateX(0) rotateY(0);
    opacity:1
  }
}

@keyframes stayput {
  from,to {
    transform:perspective(0) translate(-50%,98%) rotateX(0) rotateY(0);
    opacity:1
  }
}

@keyframes bouncedown {
  from {
    transform:perspective(0) translate(-50%,98%) rotateX(0) rotateY(0);
    opacity:1
  }
  to {
    transform:perspective(0) translate(-50%,115%) rotateX(0) rotateY(0);
    opacity:1
  }
}

@keyframes flipout {
  from {
    transform:perspective(0) translate(-50%,115%) rotateX(0) rotateY(0);
    opacity:1
  }
  to {
    transform:perspective(500px) translate(-50%,0) rotateX(-90deg) rotateY(0);
    opacity:5px
  }
}


I would guess they're trying to "reset" perspective w/ the 0 value. But they can just remove all the instances of "perspective(0)" to get the same result -- I've tested locally.

Comment 16

[Milan Sreckovic [:milan] (needinfo for best results)]

Yes, in Firefox perspective(0) behaves like perspective(1px).  The "can't reproduce on OS X" is the "keeps looking wrong after animation finishes and we start scrolling".
Or are you seeing that problem on OS X?

Comment 17

[Mike Taylor [:miketaylr]]

> Or are you seeing that problem on OS X?

Nah, not as described in the original STR. I can see the glitch occasionally on OSX w/ this demo however: http://codepen.io/miketaylr/pen/LbGvYJ (might have to hover on or off a few times).

Comment 18

[Milan Sreckovic [:milan] (needinfo for best results)]

By the way, I'm guessing the effect they want is perspective(infinity) type of thing.  At least, in the semi-reduced case, that's what gives me a good looking result (slides in from the left), and it matches in all browsers.

Comment 19

[Mike Taylor [:miketaylr]]

Adam, do we have contacts at imgur?

Comment 20

[Adam Stevenson [:adamopenweb]]

Karl reached out 16 days ago to imgur in another bug, but no response yet. We can find more contacts to reach out to.

Comment 21

[Mike Taylor [:miketaylr]]

Would be good to look for more contacts, it seems.

Comment 22

[Karl Dubost💡 :karlcow]

Contacted @briankassouf by email.

Comment 23

[Mike Taylor [:miketaylr]]

Seems like the imgur person from https://github.com/webcompat/web-bugs/issues/3632#issuecomment-268102580 might be another good option.

Comment 24

[Karl Dubost💡 :karlcow]

Note that they recorded the issue for this bug. 

> As for the more recently noted one, I opened a ticket for us to investigate (in FF 50.1.0 in OS X 10.12.2 I don't see the tooltip at all...) but can't really comment on any timeline to resolution.

Comment 25

[Mike Taylor [:miketaylr]]

I can't repro the glitch anymore, would you mind verifying Alice0775?

Comment 26

[Alice0775 White]

(In reply to Mike Taylor [:miketaylr] from comment #25)
> I can't repro the glitch anymore, would you mind verifying Alice0775?

I can still reproduce the problem with STR comment#0 and attachment 8808923 [details] on Latest Nightly53.0a1[1].

[1]
https://hg.mozilla.org/mozilla-central/rev/57ac9f63fc6953f4efeb0cc84a60192d3721251f
Mozilla/5.0 (Windows NT 10.0; WOW64; rv:53.0) Gecko/20100101 Firefox/53.0 ID:20170104030214

Comment 27

[Mike Taylor [:miketaylr]]

OK, thanks. 

(Interesting that it didn't repro for me in Win/53.)

Comment 28

[Milan Sreckovic [:milan] (needinfo for best results)]

Dees, do you have any contacts here?  We're having trouble reaching them.

Comment 29

[Mike Taylor [:miketaylr]]

According to Comment #24, Karl was in touch with imgur and they filed an internal ticket for this.

Comment 30

[Desigan Chinniah [:cyberdees] [:dees] [London - GMT]]

(In reply to Milan Sreckovic [:milan] from comment #28)
> Dees, do you have any contacts here?  We're having trouble reaching them.

I don't. However if :karlcow's contact doesn't move it forward — I could try other avenues. Let me know once you have feedback...

Comment 31

[Karl Dubost💡 :karlcow]

Note that it was __only__ 3 weeks ago.

And the contact said explicitly

> … but can't really comment on any timeline to resolution.
— https://github.com/webcompat/web-bugs/issues/3632#issuecomment-268102580

It is recorded. They know about it. 
I can try to send a gentle reminder if you want, but I would not want to be annoying too. Companies have priorities too. :)

Comment 32

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

The other option is to treat perspective(0) as an identity transform rather than being perspective(small-number).

Comment 33

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

... which is what the CSSWG just agreed to do, given that it seems like the most compatible thing.  (i.e., treating perspective(0) as perspective(infinity), including for animation, where perspective is currently animated as a matrix but should be animated by interpolating the *reciprocal* of the argument).

Comment 34

[Xidorn Quan [:xidorn] UTC+10]

Given that CSSWG decided to treat perspective(0) different from perspective(calc(0)), I think it is actually not a Web Painting issue, but a style system issue, that we need to handle them differently.

To achieve that, we probably need to change ProcessPerspective (in nsStyleTransformationMatrix.cpp) to handle the zero value specifically in advance. If we also need to do the same thing for the perspective property (which I doubt), we would probably need to add a new flag for SetCoord in nsRuleNode.cpp to handle zero coord value and zero value from calc differently.

Probably not very hard to change... when I have time :/

Comment 35

[Gerry Chang [:gchang]]

Per comment #34, the behavior between perspective(0) & perspective(calc(0)) is different. I doubt that we can fix this in 51. Mark 51 fix-optional and let's try to fix this in 52.

Comment 36

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Bug 1316236: Treat zero perspective as inf perspective.

Review commit: https://reviewboard.mozilla.org/r/104992/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/104992/

Comment 37

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8827273 [details]
Bug 1316236: Treat zero perspective as inf perspective.

https://reviewboard.mozilla.org/r/104992/#review106640

I bet this would not fix this issue. This issue is not really about rendering perspective(0), but about animating between perspective(0) and other perspective value.

Comment 38

[Xidorn Quan [:xidorn] UTC+10]

OK, I'll take a look at this tomorrow. I may have an idea about how to fix it elegantly.

Comment 39

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #37)
> Comment on attachment 8827273 [details]
> Bug 1316236: Treat zero perspective as inf perspective.
> 
> https://reviewboard.mozilla.org/r/104992/#review106640
> 
> I bet this would not fix this issue. This issue is not really about
> rendering perspective(0), but about animating between perspective(0) and
> other perspective value.

OK, except that it does fix it for me, and it does so by treating perspective(0) as perspective(inf) which is what I thought was the proposed solution.  So, I'll take your bet :)

Comment 40

[Xidorn Quan [:xidorn] UTC+10]

OK, I think you are right that this should fix the issue, as the interpolation computation of perspective function all goes to that function eventually.

I was also worrying about handling of perspective(calc(0)) which, according to CSSWG's minutes, should be handled differently from perspective(0). But after digging our code a bit, I think that requirement is hard to implement, and I don't think that actually adds much value. So I guess something like this patch should be enough.

The only thing may need fix on top of your patch is the distance calculation, which doesn't use that function, and currently clamp zero to epsilon.

I'm not sure whether we should check "> 0" or ">= epsilon". The minimum positive value float can represent is ~1.4e-45, whose reciprocal is infinite, while epsilon is ~1.19e-7, whose reciprocal is still finite (~8.39e6).

Comment 41

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1316236 - Treat perspective(0) as perspective(infinity).

Review commit: https://reviewboard.mozilla.org/r/105948/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/105948/

Comment 42

[Daniel Holbert [:dholbert]]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #40)
> I was also worrying about handling of perspective(calc(0)) [...]
> I don't think that actually adds much value.

I was going to ask that you raise this question with the CSSWG, but it looks like you already have here:
 https://github.com/w3c/csswg-drafts/issues/413#issuecomment-273928748
Thanks!

Comment 43

[Daniel Holbert [:dholbert]]

Comment on attachment 8828618 [details]
Bug 1316236 - Treat perspective(0) as perspective(infinity).

https://reviewboard.mozilla.org/r/105948/#review107146

r=me with nits addressed:

::: layout/reftests/w3c-css/submitted/transforms/perspective-zero.html:14
(Diff revision 1)
> +#ref {
> +  background: red;

Minor naming issue (which makes this test a little harder to reason about):  "#ref" feels like a misleading name for this element, especially when paired with #test on the other element.

It *suggests* that this is some sort of reference for the #test element, showing the expected rendering. But it's not at all -- we're actually expecting #ref to be covered up here (which is why it's red) -- and if any of it is visible, then that means something went wrong.

Maybe something like #coverMe or #shouldBeCovered would be a clearer ID for this element?

::: layout/reftests/w3c-css/submitted/transforms/perspective-zero.html:21
(Diff revision 1)
> +  /* If perspective(0) is invalid, #test would not create a stacking
> +   * context, and #ref would be placed on top of #test showing red.
> +   * If perspective(0) is handled as perspective(epsilon) rather than
> +   * perspective(infinity), #test would be invisible. */

This comment describes two possible bad outcomes but doesn't indicate the expected good outcome. Perhaps start with that? (something along the lines of "This transform should [...] so that it covers up [...]"

::: layout/reftests/w3c-css/submitted/transforms/reftest.list:2
(Diff revision 1)
>  == transform-containing-block-dynamic-1b.html containing-block-dynamic-1-ref.html
>  == perspective-containing-block-dynamic-1a.html containing-block-dynamic-1-ref.html
>  == perspective-containing-block-dynamic-1b.html containing-block-dynamic-1-ref.html
> +
> +== perspective-zero.html green.html

Perhaps best to delete the blank line separating this new test from the rest of the file? I don't see why we'd want to visually separate this one test from the others here.

::: layout/style/nsStyleTransformMatrix.h:33
(Diff revision 1)
>   * A helper to generate gfxMatrixes from css transform functions.
>   */
>  namespace nsStyleTransformMatrix {
>  
> +  // Function for applying perspective() transform function. We treat
> +  // any value smaller than epsilon as perspective(infinite), which

s/infinite/infinity/, I think.

("infinite" is an adjective; "infinity" is the noun for the quantity)

::: layout/style/nsStyleTransformMatrix.h:35
(Diff revision 1)
>  namespace nsStyleTransformMatrix {
>  
> +  // Function for applying perspective() transform function. We treat
> +  // any value smaller than epsilon as perspective(infinite), which
> +  // follows CSSWG's resolution on perspective(0). See bug 1316236.
> +  inline void PerspectiveMatrix(mozilla::gfx::Matrix4x4& aMatrix, float aDepth)

Perhaps this should be named "ApplyPerspectiveToMatrix", since that's what it does?

The current name, "PerspectiveMatrix()", sounds more like a function that *returns* a perspective matrix to me, rather than a function that modifies one of its parameters.

Comment 44

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8828618 [details]
Bug 1316236 - Treat perspective(0) as perspective(infinity).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/105948/diff/1-2/

Comment 45

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3856294790fe
Treat perspective(0) as perspective(infinity). r=dholbert

Comment 46

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3856294790fe

Comment 47

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8828618 [details]
Bug 1316236 - Treat perspective(0) as perspective(infinity).

Approval Request Comment
[Feature/Bug causing the regression]: bug 1274158
[User impact if declined]: may see glitch on some websites with CSS transition or animation
[Is this code covered by automated tests?]: a new test is added in this patch
[Has the fix been verified in Nightly?]: just landed.
[Needs manual test from QE? If yes, steps to reproduce]: no.
[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: may not be very risky. shouldn't be riskier than bug 1274158 which is already in beta.
[Why is the change risky/not risky?]: it only covers a specific case. This patch is changing the behavior of exactly the same case as bug 1274158, but to match other browsers' behavior. As far as no more regression shows up with bug 1274158, this patch shouldn't be riskier.
[String changes made/needed]: n/a

Comment 48

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

remote: vendor-imports/mozilla/mozilla-central-reftests/transforms/green.html status changed to 'Needs Work' due to error:
remote: Not linked to a specification.

This presumably either needs to have -ref in the filename or be in a reference (?) subdirectory.

Comment 49

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b7eeafd700d
followup - Move green.html into reference subdirectory.

Comment 50

[Xidorn Quan [:xidorn] UTC+10]

^ Moved the reference file.

Comment 51

[Julien Cristau [:jcristau]]

Comment on attachment 8828618 [details]
Bug 1316236 - Treat perspective(0) as perspective(infinity).

fix visual glitch on animations, beta52+

Comment 52

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a425672d300f

Comment 53

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

https://hg.mozilla.org/mozilla-central/rev/1b7eeafd700d

Comment 54

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

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #50)
> ^ Moved the reference file.

Thanks; that fixed it.

Comment 56

[Bjorn Tipling]

I'm responsible for the code on imgur that led to this issue. My intention was to reset the perspective but did so incorrectly due to ignorance. I'm submitting a fix now that sets the perspective to none, by omitting perspective as advised above.