Closed Bug 1180509 Opened 4 years ago Closed 4 years ago

[OMTA] Icons and text-strings in Australis doorhanger menu have some minor movement at the end of opacity fade-in animation

Categories

(Core :: Layout, defect, major)

41 Branch
x86_64
Windows 7
defect
Not set
major

Tracking

()

VERIFIED FIXED
mozilla43
Tracking Status
firefox39 --- unaffected
firefox40 --- unaffected
firefox41 + verified
firefox42 + verified
firefox43 + verified

People

(Reporter: Virtual, Assigned: jwatt)

References

Details

(Keywords: nightly-community, regression)

Attachments

(3 files)

Icons and text-strings in Australis doorhanger menu have some minor movement at the end of opacity fade-in animation

Caused by:
Bug #980770
Bug #1122526



[Tracking Requested - why for this release]: Regression
OS: Unspecified → Windows 7
Hardware: Unspecified → x86_64
(In reply to David Baron [:dbaron] ⏰UTC-7 from comment #3)
> This may be related to bug 1163817.

On the other hand, it may not be, since bug 1163817 only started happening when we started rendering the animated layer at an unusual scale (slightly greater than 1.0 on one axis), which I don't think should be happening here.  This animated layer should be being rendered at a 1.0 scale -- although I suppose the problem could be that something's causing that not to happen.

(In reply to Virtual_ManPL [:Virtual] from comment #0)
> Icons and text-strings in Australis doorhanger menu have some minor movement
> at the end of opacity fade-in animation
> 
> Caused by:
> Bug #980770
> Bug #1122526

I don't think there's evidence that this was actually caused by bug 1122526.  You just couldn't see it beforehand.

> [Tracking Requested - why for this release]: Regression

I don't think this is serious enough to block the release, or to block shipping OMT animations.


I'd also note that this isn't reproducable on Linux even when I enable the doorhanger animations there.
I wonder if this is related to DPI.  Are the people who are seeing this running with the "large fonts" setting or whatever it is that causes us to use a ratio of CSS pixels to display pixels that's larger than 1?
Tracking in 42, is this regression currently affecting 40,41?
Flags: needinfo?(bernesb)
It will be affecting 41 - per Bug #1122526 Comment 67, Bug #1122526 Comment 68, Bug #1122526 Comment 69, Bug #1122526 Comment 70
and it may be affecting 40 - depends on uplift approval of patches from Bug #1122526.
Flags: needinfo?(bernesb)
(In reply to Virtual_ManPL [:Virtual] from comment #7)
> and it may be affecting 40 - depends on uplift approval of patches from Bug
> #1122526.


No, it won't, since OMTA is disabled for beta/release on 40.
(In reply to David Baron [:dbaron] (busy until July 20) from comment #8)
> (In reply to Virtual_ManPL [:Virtual] from comment #7)
> > and it may be affecting 40 - depends on uplift approval of patches from Bug
> > #1122526.
> 
> 
> No, it won't, since OMTA is disabled for beta/release on 40.

Ah, that's why.
Thank you very much for detailed explanation here and in Bug #1122526 Comment 73.
So I'm marking Firefox 40 as unaffected
and Firefox 41 affected as patches from Bug #1122526 will be landing there soon.
Thank you for the explanations in Comment 7, 8, and setting the correct status flags.
Based on bug 1122526, marking this as fixed. 

Virtual, if you have time to verify this works now, since you definitely saw it to begin with, that would be awesome.
Flags: needinfo?(bernesb)
(In reply to Liz Henry (:lizzard) from comment #11)
> Based on bug 1122526, marking this as fixed. 

Based in what way?  This is a regression from bug 1122526.

Undoing that marking because I don't think it makes sense.



(I'm still interested in a answer to comment 5.)
Also -- one other possibility is that the movement is a shift between grayscale antialiasing and subpixel antialiasing.
(In reply to David Baron [:dbaron] ⏰UTC-7 (busy Aug. 8-Aug. 30) from comment #5)
> I wonder if this is related to DPI.  Are the people who are seeing this
> running with the "large fonts" setting or whatever it is that causes us to
> use a ratio of CSS pixels to display pixels that's larger than 1?

I have normal default DPI, which is 96 DPI and 100% font size, nothing changed.
I just wonder what Elbart has, sent him a PM on forum and email.
(In reply to David Baron [:dbaron] ⏰UTC-7 (busy Aug. 8-Aug. 30) from comment #12)

> Based in what way?  This is a regression from bug 1122526.
> 
> Undoing that marking because I don't think it makes sense.


OK, my mistake. When Virtual said earlier, "Firefox 41 affected as patches from Bug #1122526 will be landing there soon", I looked at that bug and it was marked as fixed for 41. So, I thought that the regression was fixed in that bug soon after your comment 3 weeks ago. Since I didn't know for sure, I asked for verification. Thanks for catching this.
There does seem to be a switch from grayscale antialiasing to subpixel antialiasing at the end of the animation, which is probably expected since at the end of the opacity animation we switch from a partially transparent background to an opaque background. I don't think that's the main issue though. There is also a jump in the position of both text and icons, and including a vertical jump at that, which suggests a snapping or sizing issue.

One thing I have noticed is that in ChooseScaleAndSetTransform the Size returned by nsLayoutUtils::ComputeSuitableScaleForAnimation is not exactly {1,1} on Windows whereas it is on OS X. In my case on Windows it's more like {0.986...,0.996...}.
(In reply to Jonathan Watt [:jwatt] from comment #16)
> One thing I have noticed is that in ChooseScaleAndSetTransform the Size
> returned by nsLayoutUtils::ComputeSuitableScaleForAnimation is not exactly
> {1,1} on Windows whereas it is on OS X. In my case on Windows it's more like
> {0.986...,0.996...}.

Yeah, definitely sounds worth figuring out why that's the case.  (Shouldn't be hard to figure out with a few printfs in the function.)
(In reply to Virtual_ManPL [:Virtual] from comment #14)
> I have normal default DPI, which is 96 DPI and 100% font size, nothing
> changed.
> I just wonder what Elbart has, sent him a PM on forum and email.

That's no longer interesting if yours is normal, and also because of Jonathan's work.
Flags: needinfo?(elbart)
When under for the width scale calculation:

  GetSuitableScale
  nsLayoutUtils::ComputeSuitableScaleForAnimation
  mozilla::ChooseScaleAndSetTransform
  mozilla::FrameLayerBuilder::BuildContainerLayerFor
  nsDisplayTransform::BuildLayer
  mozilla::ContainerState::ProcessDisplayItems
  mozilla::FrameLayerBuilder::BuildContainerLayerFor
  nsDisplayList::PaintRoot
  nsLayoutUtils::PaintFrame

we have:

  float displayVisibleRatio = float(aDisplayDimension) /
                              float(aVisibleDimension);
  // We want to rasterize based on the largest scale used during the
  // transform animation, unless that would make us rasterize something
  // larger than the screen.  But we never want to go smaller than the
  // minimum scale over the animation.
  return std::max(std::min(aMaxScale, displayVisibleRatio), aMinScale);

For all platforms aMaxScale comes from an examination of the CSS animation and is 1.

In the case of Windows, up in mozilla::ChooseScaleAndSetTransform, aVisibleRect is {-120,0,18552,30480} and displaySize is {18300,30360}. As a result, in GetSuitableScale, displayVisibleRatio is slightly less than 1 (0.996062994) and therefore the std::min call returns 0.996062994.

In the case of OS X, up in mozilla::ChooseScaleAndSetTransform, aVisibleRect is {0,0,16071,24420} and displaySize is {16080,24420}. As a result, in GetSuitableScale, displayVisibleRatio is slightly greater than 1 (1.00056005) and therefore the std::min call returns exactly 1.0.

A similar thing happens for height.
The "visible rect" comes from calling nsDisplayList::GetBounds on the nsDisplayTransform's children nsDisplayList.

The "display" size is the result of calling GetClientSize() on the nearest nsIWidget of the transformed nsMenuPopupFrame.

So as long as the contents of the nsMenuPopupFrame are larger than its nsIWidget then the scale is going to end up being less than the 1.0f that we need (in order to provide a pre-rendered texture at a factor of 1 to give a smooth end to the animation).
So to be clear it's patch 4 in bug 1122526 that has caused this regression.

  https://bugzilla.mozilla.org/attachment.cgi?id=8626764&action=diff

We used to use to just set displaySize to presContext->GetVisibleArea().Size(), which for me in this case is {0,0,115200,62880}. That would make displaySize bigger than aVisibleRect which avoids the less-than-1.0 result from GetSuitableScale.
Attached patch patchSplinter Review
This fixes the issue and seems like a good thing to do anyway.
Assignee: nobody → jwatt
Comment on attachment 8649761 [details] [diff] [review]
patch

Maybe you can review this while David is on vacation, Robert?
Attachment #8649761 - Flags: review?(roc)
https://hg.mozilla.org/mozilla-central/rev/739b247fd7b7
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Requesting an uplift to 41 and 42, as these versions are also affected.
Flags: needinfo?(jwatt)
Comment on attachment 8649761 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]: 1122526
[User impact if declined]: Ugliness in Australis doorhanger menu
[Describe test coverage new/current, TreeHerder]: Lots of tests test this code
[Risks and why]: Reasonably low risk, just a small tweak
[String/UUID change made/needed]: None
Flags: needinfo?(roc)
Attachment #8649761 - Flags: approval-mozilla-beta?
Attachment #8649761 - Flags: approval-mozilla-aurora?
Flags: needinfo?(jwatt)
Virtual_ManPL, could you please verify this is fixed in the latest Nightly build? Thanks.
Flags: needinfo?(bernesb)
Comment on attachment 8649761 [details] [diff] [review]
patch

Patch looks simple and has been in Nightly for a week. Let's uplift to Aurora42 and Beta41.
Attachment #8649761 - Flags: approval-mozilla-beta?
Attachment #8649761 - Flags: approval-mozilla-beta+
Attachment #8649761 - Flags: approval-mozilla-aurora?
Attachment #8649761 - Flags: approval-mozilla-aurora+
(In reply to Ritu Kothari (:ritu) from comment #28)
> Virtual_ManPL, could you please verify this is fixed in the latest Nightly
> build? Thanks.

Yes, I verified it and it's fixed (or looks so) in latest Nightly 43.0a1 (2015-08-26)
Flags: needinfo?(bernesb)
Status: RESOLVED → VERIFIED
Virtual_ManPL --- thanks for giving the heads-up on this!
You need to log in before you can comment on or make changes to this bug.