Closed Bug 989403 Opened 6 years ago Closed 5 years ago

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

Categories

(Core :: Panning and Zooming, defect)

28 Branch
ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla34
feature-b2g 2.1

People

(Reporter: nick, Assigned: botond)

References

Details

(Keywords: regression, Whiteboard: [ucid:Graphic11,ft:graphic][2.1-feature-qa+])

Attachments

(8 files)

Attached file 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.
blocking-b2g: --- → 1.3?
QA Wanted to see if we can reproduce with the attached Calculator app on a Helix device running 1.1 HD.
Keywords: qawanted
Late discovery of this issue, but it impacts pre-load app.
blocking-b2g: 1.3? → 1.3+
Is this present on trunk, or only on 1.3?
Component: CSS Parsing and Computation → Layout: View Rendering
I can reproduce with 1.5 on GP Peak git commit info 2014-03-28.
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)
QA Contact: mvaughan
(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
Keywords: qawanted
Jet

Please help reassign
Flags: needinfo?(bugs)
OK, I still need that regression range so I can get a suitable assignee.
Flags: needinfo?(mvaughan)
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
Flags: needinfo?(mvaughan)
Blocks: gaia-apzc
Redirecting needinfo to Milan since this is an APZC regression.
Component: Layout: View Rendering → Panning and Zooming
Flags: needinfo?(bugs) → needinfo?(milan)
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.
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.
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.
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.
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.
(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.
Flags: needinfo?(nick)
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.
blocking-b2g: 1.3+ → ---
Flags: needinfo?(nick)
(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.
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.
Flags: needinfo?(nick)
blocking-b2g: --- → 1.3?
(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.
blocking-b2g: 1.3? → backlog
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.
Flags: needinfo?(kward)
(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.
Flags: needinfo?(nick)
I have a work around in the review queue.
Blocks: gaia-apzc-2
No longer blocks: gaia-apzc
Flags: needinfo?(milan)
Verified fix on Peak v1.3, new version is approved and live on Marketplace.
The work around for the app is in place but the underlying problem still needs to be fixed. Is it targeted for v1.4
Flags: needinfo?(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.
Flags: needinfo?(jsmith)
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
(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
Flags: needinfo?(jsmith)
Redirecting to Milan.
Flags: needinfo?(jsmith) → needinfo?(milan)
Jason - we need to escalate this for V1.4.  What is the issue with resolving it for V1.4?
Flags: needinfo?(kward)
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.
Flags: needinfo?(milan)
Having an offline conversation, will report on resolution.
(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?
Flags: needinfo?(milan)
(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.
Flags: needinfo?(milan)
QA Whiteboard: [priority]
blocking-b2g: backlog → ---
feature-b2g: --- → 2.1
Blocks: 1016796
Whiteboard: [ucid:Graphic11,ft:graphic]
Botond will be working on this.
Assignee: nobody → botond
Attached file 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.
A similar issue on fennec 28:
https://bugzilla.mozilla.org/show_bug.cgi?id=999885
My initial investigation is pointing towards an invalidation problem.
Attached patch bug989403.patchSplinter Review
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.
Attachment #8462939 - Flags: review?(roc)
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.
Attachment #8462939 - Flags: review?(roc) → review+
Now that we know the cause of this bug it appears that bug 993525 is not related.
No longer depends on: apz-css-transforms
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.
(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.
The page I'm using to test on my Flame device is http://people.mozilla.org/~bballo/calc2.html.
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.
Keywords: checkin-needed
I think the reftest that isn't working on emulator is worthy of a bug too.
(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.
https://hg.mozilla.org/mozilla-central/rev/6973d3354d63
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
QA Whiteboard: [priority] → [priority][2.1-feature-qa+]
Flags: in-moztrap?(npark)
QA Contact: mvaughan → npark
QA Whiteboard: [priority][2.1-feature-qa+] → [priority]
Whiteboard: [ucid:Graphic11,ft:graphic] → [ucid:Graphic11,ft:graphic][2.1-feature-qa+]
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.
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.
Flags: in-moztrap?(npark) → in-moztrap+
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.