989403 - transform: scale(X) not being repainted when assigned to

Status: VERIFIED FIXED

(Core :: Panning and Zooming, defect)

Description

[(not reading bugmail) Nick Desaulniers [:\n]]

Attachment: application.zip

See attached calculator app.  The offending line is line 24:

this.display.style.transform = 'scale(' + scaleFactor + ')';

With this line commented out, the display is repainted correctly.  With this line commented in, the display is not repainted.

I can only reproduce on FxOS 1.3+ devices with a resolution other than 320x480px, such as a nexus 4 or ZTE Open C.

Comment 1

[(not reading bugmail) Nick Desaulniers [:\n]]

Attachment: example video

Comment 2

[Jason Smith [:jsmith]]

QA Wanted to see if we can reproduce with the attached Calculator app on a Helix device running 1.1 HD.

Comment 3

[Karen Ward [:kward]]

Late discovery of this issue, but it impacts pre-load app.

Comment 4

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

Is this present on trunk, or only on 1.3?

Comment 5

[(not reading bugmail) Nick Desaulniers [:\n]]

I can reproduce with 1.5 on GP Peak git commit info 2014-03-28.

Comment 6

[joseenrique.alvarezfernandez]

Hi all

Commenting the line that Nick pointed out (line 24: this.display.style.transform = 'scale(' + scaleFactor + ')'; ) I can confirm that the app works fine on the OPENC v1.3 (build 20140325120432)

Comment 7

[Matthew Vaughan]

(In reply to Jason Smith [:jsmith] from comment #2)
> QA Wanted to see if we can reproduce with the attached Calculator app on a
> Helix device running 1.1 HD.

This issue does not reproduce for me on the 03/31/14 1.1 HD on a Helix using the attached Calculator app.

Device: Helix v1.1 HD MOZ RIL
BuildID: 20140331042200
Gaia: bbee1297b952830cb08c3bee48d281ab4123db3d
Gecko: 6c6ee476132d
Version: 18.0
Firmware Version: 20131217_ENG

Comment 8

[Preeti Raghunath(:Preeti)]

Jet

Please help reassign

Comment 9

[Jet Villegas (inactive)]

OK, I still need that regression range so I can get a suitable assignee.

Comment 10

[Matthew Vaughan]

This issue looks to be APZ related.

I first found that it started reproducing as soon as APZ was enabled by default on 01/10/14 1.3 build. I then went back to the first build that APZ was available (but not enabled by default), which was the 10/25/13 1.3 build. With APZ disabled, the issue did *not* reproduce. However once I enabled it, the issue did reproduce.

I didn't think an actual window would be helpful here, but please let me know if it or anything else is needed.

- First build APZ appeared in -
Device: Helix v1.3 MOZ RIL
BuildID: 20131025100746
Gaia: afbf45f26a73b7cd5e0a831bea48087331975286
Gecko: 2f2a45f04e7c
Version: 27.0a1
Firmware Version: 20131217_ENG

Comment 11

[Jason Smith [:jsmith]]

Redirecting needinfo to Milan since this is an APZC regression.

Comment 12

[Kartikaya Gupta (email:[e-mail])]

Are there any downsides of just commenting out the scale transform property? If not then that might be a viable low-risk fix for 1.3.

Comment 13

[Kartikaya Gupta (email:[e-mail])]

Actually I guess commenting that out will prevent the display area from shrinking to fit, so in the cases where that actually does anything you probably want to keep it.

I installed the app on my hamachi running master but was unable to reproduce the problem from the video, even though I'm able to trigger the scaling code by typing in a bunch of digits.

Comment 14

[(not reading bugmail) Nick Desaulniers [:\n]]

Kats, I'm working on a workaround.  I suspect this only affects devices with a resolution greater than 320x480px.  Such has been my experience.  Please try a device other than the hamachi/buri, inri, or unagi.

Comment 15

[Kartikaya Gupta (email:[e-mail])]

Do you know why this would only affect devices with a higher resolution? As far as I know that shouldn't be a consideration at all in this case, as long as the scaling is triggered.

Comment 16

[(not reading bugmail) Nick Desaulniers [:\n]]

I assumed everything for earlier versions of FxOS was hard coded for 320x480px which is why we have lovely distinctions such as 1.1 and 1.1HD, :P  I jest.  I don't know anything about graphics code, just telling you my experience based on what devices I have and have not been able to reproduce on.

Comment 17

[Ivan Tsay (:ITsay)]

(In reply to Nick Desaulniers [:\n] from comment #14)
> Kats, I'm working on a workaround.  I suspect this only affects devices with
> a resolution greater than 320x480px.  Such has been my experience.  Please
> try a device other than the hamachi/buri, inri, or unagi.

Hi Nick,

Mind if you can take this bug and provide your workaround.

Comment 18

[Jason Smith [:jsmith]]

Clearing blocking flag here - I think a workaround here on the app side & not place a dependency on gecko itself, even though there's a legitimate bug here.

Comment 19

[Jason Smith [:jsmith]]

(In reply to Jason Smith [:jsmith] from comment #18)
> Clearing blocking flag here - I think a workaround here on the app side &
> not place a dependency on gecko itself, even though there's a legitimate bug
> here.

Clarification - I think we should do workaround on the app side & not depend on the gecko fix here. This is a bug, but I think we need the lowest risk solution here, which is fixing this in the app, not gecko.

Comment 20

[Beatriz Rodríguez [:brg]]

We do agree here with the lowest risk fix.
However we do not know how many other third party apps can be affected by this bug in platform side...
We would like to know how easy or difficult is to fix this bug in the platform side first to make the best decission.
This is a blocker for certification of a new device with WVGA.
Renominating till we get the feedback from the dev side.

Comment 21

[Jason Smith [:jsmith]]

(In reply to Beatriz Rodríguez [:brg] from comment #20)
> We would like to know how easy or difficult is to fix this bug in the
> platform side first to make the best decission.

The graphics team is already overloaded as is - we really need a compelling argument showing that isn't possible to workaround this problem.

> This is a blocker for certification of a new device with WVGA.

The calculator issue you can block on for resolving this on the app side, but I don't think there's a compelling argument here to block on the gecko side. There needs to be proof on this bug that there's no way to workaround the bug. If that proof exists, then we can block on this. But without that information, this stays as a non-blocker.

Comment 22

[Beatriz Rodríguez [:brg]]

We cannot launch a device with a gecko bug affecting the development of third party apps ecosystem in devices with other resolution than HVGA unless you can say that the bug will only appear in calculator app.
Can anyone confirm how many apps published in the marketplace are affected by this bug? (Ni Karen)

There are more devices with different resolutions and based in 1.3 to be launch this year that will need this fix as well.

Comment 23

[Jason Smith [:jsmith]]

(In reply to Beatriz Rodríguez [:brg] from comment #22)
> We cannot launch a device with a gecko bug affecting the development of
> third party apps ecosystem in devices with other resolution than HVGA unless
> you can say that the bug will only appear in calculator app.

Well then we need that proof here. We can't just point at one bug and claim it's possible to happen everywhere.

> There are more devices with different resolutions and based in 1.3 to be
> launch this year that will need this fix as well.

All I need here is proof of impact that is outside of Mozilla's control or can't be worked around within Mozilla's control.

Comment 24

[(not reading bugmail) Nick Desaulniers [:\n]]

I have a work around in the review queue.

Comment 25

[Lisa Brewster [:adora]]

Verified fix on Peak v1.3, new version is approved and live on Marketplace.

Comment 26

[Karen Ward [:kward]]

The work around for the app is in place but the underlying problem still needs to be fixed. Is it targeted for v1.4

Comment 27

[Jason Smith [:jsmith]]

(In reply to Karen Ward [:kward] from comment #26)
> The work around for the app is in place but the underlying problem still
> needs to be fixed. Is it targeted for v1.4

Nope.

Comment 28

[joseenrique.alvarezfernandez]

Hi Jason

I understand that the solution for the bug is not targeted for 1.4, am I right? If so, when is it going to be solved?

Best

Comment 29

[David Palomino [:dpalomino]]

(In reply to joseenrique.alvarezfernandez from comment #28)
> Hi Jason
> 
> I understand that the solution for the bug is not targeted for 1.4, am I
> right? If so, when is it going to be solved?
> 
> Best

Hi, 

ni Jason. 

Thanks, 
David

Comment 30

[Jason Smith [:jsmith]]

Redirecting to Milan.

Comment 31

[Karen Ward [:kward]]

Jason - we need to escalate this for V1.4.  What is the issue with resolving it for V1.4?

Comment 32

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

We don't know what's involved, if it's a bug or a missing feature or a larger architectural issue.  Investigating that will take time, and so will fixing it.  If the product wants to prioritize this over some 2.0 features, not a problem. We can spend time and let you know when we have more details then. I don't know that it will make the 4/21 date though, regardless of what we do.

Comment 33

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

Having an offline conversation, will report on resolution.

Comment 34

[Karen Ward [:kward]]

(In reply to Milan Sreckovic [:milan] from comment #33)
> Having an offline conversation, will report on resolution.

Hi Milan.  Is there an update on this bug?

Comment 35

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

(In reply to Karen Ward [:kward] from comment #34)
> (In reply to Milan Sreckovic [:milan] from comment #33)
> > Having an offline conversation, will report on resolution.
> 
> Hi Milan.  Is there an update on this bug?

You're part of the "offline conversation" :)  We have put this on our roadmap for an upcoming release. Depending on the progress of the homescreen work (bug 989848), we may be able to get this for 2.0.

Comment 36

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

Botond will be working on this.

Comment 37

[Kartikaya Gupta (email:[e-mail])]

Attachment: Standalone test case

The thing from application.zip as a standalone HTML page that can be loaded in the B2G browser. I verified the bug still exists on a Flame device running recent 2.1/master code.

Comment 38

[troysun]

A similar issue on fennec 28:
https://bugzilla.mozilla.org/show_bug.cgi?id=999885

Comment 39

[Botond Ballo [:botond]]

My initial investigation is pointing towards an invalidation problem.

Comment 40

[Botond Ballo [:botond]]

Attachment: bug989403.patch

This indeed appears to be an invalidation bug related to transforms.

Here is my understanding of the issue, after debugging it with Timothy's help:

When a transform is set on an element (in this case, a transform:scale(1) on the calculator's output field), an nsDisplayTransform display item is created to wrap the display items corresponding to the transformed content.

The content inside the nsDisplayTransform is layerized, to allow efficient rendering when the transform is changing. When the transform is not changing - as is the case here - the layer containing the transformed content is "inactive", which means it gets flattened into its parent layer on the content side before being sent over to the compositor.

When content inside the nsDisplayTransform changes, two invalidations need to occur for the new content to be repainted correctly:

  1. The region in the inactive layer that is occupied by the
     content needs to be invalidated, so that the content is
     repainted onto that layer.

  2. The region in the parent layer that is inactive layer's
     contents are flattened onto, needs to be invalidated,
     so that the inactive layer is re-flattened onto the
     parent layer.

The problem is with (2). When computing the region of the parent layer to invalidate, we intersect the region occupied by the inactive layer, with the parent layer's clip rect.

The problem is that when the intersection is performed, the regions being intersected are represented in different coordinate systems (but interpreted as being in the same coordinate system). As a result, the intersection is empty, no region of the parent layer is invalidated, and the updated contents of the inactive layer is not re-flattened onto the parent layer.

The difference in coordinate systems arises due to there being a resolution on the parent layer. The clip rect has the resolution applied, but the invalid region does not. (The resolution is then applied to the invalid region *after* the intersection, but by then it's too late.)

The attached patch fixes this problem by applying the parent layer's resolution to the invalid region before intersecting it with the clip rect.

Comment 41

[Botond Ballo [:botond]]

Try push: https://tbpl.mozilla.org/?tree=Try&rev=ec835f5b12ed

Comment 42

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8462939 [details] [diff] [review]
bug989403.patch

Review of attachment 8462939 [details] [diff] [review]:
-----------------------------------------------------------------

Good catch.

You could probably write a reftest for this using MozReftestInvalidate.

Comment 43

[Timothy Nikkel (:tnikkel)]

Looks like the bug was introduced in http://hg.mozilla.org/mozilla-central/rev/c67ff8ae77e2 bug 770001.

Comment 44

[Timothy Nikkel (:tnikkel)]

Now that we know the cause of this bug it appears that bug 993525 is not related.

Comment 45

[Botond Ballo [:botond]]

To round out the explanation: the reason this bug does not reproduce with APZ disabled (comment 10) is that the page does not have a meta-viewport tag, and the default viewport set is differently with APZ enabled than with APZ disabled. In particular, the page resolution is 1 with APZ disabled, and so the difference in coordinate systems that gives rise to the bug ends up not making a difference.

Comment 46

[Botond Ballo [:botond]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #42)
> You could probably write a reftest for this using MozReftestInvalidate.

I have been trying to write a reftest for this, with Timothy's help, but I've had no success.

I'm getting completely different numbers on b2g-emulator, where my reftest fails even with my patch, than on my Flame device, where a page very similar to my test, just with the MozReftestInvalidate event replaced by a mousedown event, renders correctly with my patch and incorrectly without.

I will attach the reftest I'm trying, and the resulting images. If anyone has any ideas as to why it's failing, please let me know.

Comment 47

[Botond Ballo [:botond]]

Attachment: Reftest attempt - test page

Comment 48

[Botond Ballo [:botond]]

Attachment: Reftest attempt - reference page

Comment 49

[Botond Ballo [:botond]]

Attachment: Reftest attempt - result for test page

Comment 50

[Botond Ballo [:botond]]

Attachment: Reftest attempt - result for reference page

Comment 51

[Botond Ballo [:botond]]

The page I'm using to test on my Flame device is http://people.mozilla.org/~bballo/calc2.html.

Comment 52

[Botond Ballo [:botond]]

Let's land the fix for now. If anyone has any suggestions about why my reftest attempt is failing, I can go back to trying to get it to work.

One of the problems preventing me from debugging the reftest effectively is that I can't run the emulator normally (i.e. not for running reftests). I filed bug 1046325 for this.

Comment 53

[Botond Ballo [:botond]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6973d3354d63

Comment 54

[Timothy Nikkel (:tnikkel)]

I think the reftest that isn't working on emulator is worthy of a bug too.

Comment 55

[Botond Ballo [:botond]]

(In reply to Timothy Nikkel (:tn) from comment #54)
> I think the reftest that isn't working on emulator is worthy of a bug too.

Filed bug 1046440.

Comment 56

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6973d3354d63

Comment 57

[No-Jun Park [:njpark]]

Testing Perspective:
This bug happens when:
- a page does not have the meta-viewport tag, forcing to use the default viewport width of 980px
- transform-scale value is not 1 (but occasionally, it can happen with value = 1)
- non-animating content within the div that has above transform-scale applied

When all of the above conditions are met, the invalidation issue occurs.  Since most of the mobile app has meta viewport tag specified to accommodate the small screen, this may not commonly occur.  Because of the specific nature of the bug, I think it is sufficient to test this with existing Calculator app, with both the original code and the modified code, where the line 

var scaleFactor = Math.min(1, (screenWidth - 16) / valWidth);

is change to 

var scaleFactor = Math.min(1.1, (screenWidth - 16) / valWidth);

to force non-1 scale value.  The debugger shows that in most cases the scaleFactor var is set to 1 in most instances on Flame device.

Will post the link to moztrap test case shortly.

Comment 58

[No-Jun Park [:njpark]]

mozTrap test case location:
https://moztrap.mozilla.org/manage/case/14106/

Note: Graphics regression test will be used to detect possible side issues arising from this patch.