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

RESOLVED FIXED in Firefox 52

Status

()

defect
P3
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: alice0775, Assigned: xidorn)

Tracking

({regression})

51 Branch
mozilla53
x86
Windows 10
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(platform-rel +, firefox50 unaffected, firefox51 wontfix, firefox52 fixed, firefox53 fixed)

Details

(Whiteboard: [sitewait] [css] [platform-rel-Imgur], )

Attachments

(5 attachments)

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
Posted image screenshot
I can reproduce this on Windows with Firefox 51. Probably having a too small perspective reveals some graphics issue.
Posted file semi-reduced html
STR:
1. Click [click]
   observe graphics
2. Repeat Step1

Actual Results:
Graphic glitch
Though the graphics glitch is quite varied.
Keywords: regression
Flags: needinfo?(milan)
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.
Status: NEW → RESOLVED
Closed: 3 years ago
Flags: needinfo?(milan)
Resolution: --- → WONTFIX
Version: 51 Branch → 49 Branch
Let me reopen this while we're figuring if we should just back out bug 1274158, or if this is a tech-evangelism issue.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Version: 49 Branch → 51 Branch
(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.
Component: Graphics → Layout
(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.
Component: Layout → Graphics
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.
Component: Graphics → Layout
(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.
s/after scrolling the page/after scrolling the page, resizing browser or switching tabs/
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.
Let's turn this into a Tech Evangelism and open a new bug for the glitch leftover issue.
Component: Layout → Desktop
Product: Core → Tech Evangelism
Version: 51 Branch → Firefox 51
See Also: → 1316764
(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.
Whiteboard: [needscontact]
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?
> 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).
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.
Adam, do we have contacts at imgur?
Flags: needinfo?(astevenson)
Karl reached out 16 days ago to imgur in another bug, but no response yet. We can find more contacts to reach out to.
Flags: needinfo?(astevenson)
Would be good to look for more contacts, it seems.
Contacted @briankassouf by email.
Seems like the imgur person from https://github.com/webcompat/web-bugs/issues/3632#issuecomment-268102580 might be another good option.
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.
Whiteboard: [needscontact] → [sitewait] [css]
Depends on: 1316764
Priority: -- → P3
I can't repro the glitch anymore, would you mind verifying Alice0775?
Flags: needinfo?(alice0775)
(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
Flags: needinfo?(alice0775)
OK, thanks. 

(Interesting that it didn't repro for me in Win/53.)
Dees, do you have any contacts here?  We're having trouble reaching them.
Flags: needinfo?(dchinniah)
According to Comment #24, Karl was in touch with imgur and they filed an internal ticket for this.
(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...
platform-rel: --- → ?
Flags: needinfo?(dchinniah)
Whiteboard: [sitewait] [css] → [sitewait] [css] [platform-rel-Imgur]
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. :)
The other option is to treat perspective(0) as an identity transform rather than being perspective(small-number).
... 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).
Component: Desktop → Layout: Web Painting
Flags: needinfo?(xidorn+moz)
Product: Tech Evangelism → Core
Version: Firefox 51 → 51 Branch
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 :/
Component: Layout: Web Painting → CSS Parsing and Computation
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 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.
Attachment #8827273 - Flags: review?(xidorn+moz) → review-
OK, I'll take a look at this tomorrow. I may have an idea about how to fix it elegantly.
Assignee: nobody → xidorn+moz
Flags: needinfo?(xidorn+moz)
(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 :)
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).
(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 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.
Attachment #8828618 - Flags: review?(dholbert) → review+
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3856294790fe
Treat perspective(0) as perspective(infinity). r=dholbert
https://hg.mozilla.org/mozilla-central/rev/3856294790fe
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
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
Attachment #8828618 - Flags: approval-mozilla-beta?
Attachment #8828618 - Flags: approval-mozilla-aurora?
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.
Flags: needinfo?(xidorn+moz)
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b7eeafd700d
followup - Move green.html into reference subdirectory.
^ Moved the reference file.
Flags: needinfo?(xidorn+moz)
Comment on attachment 8828618 [details]
Bug 1316236 - Treat perspective(0) as perspective(infinity).

fix visual glitch on animations, beta52+
Attachment #8828618 - Flags: approval-mozilla-beta?
Attachment #8828618 - Flags: approval-mozilla-beta+
Attachment #8828618 - Flags: approval-mozilla-aurora?
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #50)
> ^ Moved the reference file.

Thanks; that fixed it.
Duplicate of this bug: 1335357
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.
Depends on: 1342229
You need to log in before you can comment on or make changes to this bug.