Closed
Bug 989403
Opened 11 years ago
Closed 10 years ago
transform: scale(X) not being repainted when assigned to
Categories
(Core :: Panning and Zooming, defect)
Tracking
()
People
(Reporter: nick, Assigned: botond)
References
Details
(Keywords: regression, Whiteboard: [ucid:Graphic11,ft:graphic][2.1-feature-qa+])
Attachments
(8 files)
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.
Reporter | ||
Updated•11 years ago
|
blocking-b2g: --- → 1.3?
Reporter | ||
Comment 1•11 years ago
|
||
Comment 2•11 years ago
|
||
QA Wanted to see if we can reproduce with the attached Calculator app on a Helix device running 1.1 HD.
Keywords: qawanted
Comment 3•11 years ago
|
||
Late discovery of this issue, but it impacts pre-load app.
blocking-b2g: 1.3? → 1.3+
Comment 4•11 years ago
|
||
Is this present on trunk, or only on 1.3?
Component: CSS Parsing and Computation → Layout: View Rendering
Reporter | ||
Comment 5•11 years ago
|
||
I can reproduce with 1.5 on GP Peak git commit info 2014-03-28.
Comment 6•11 years ago
|
||
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)
Updated•11 years ago
|
QA Contact: mvaughan
Comment 7•11 years ago
|
||
(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
Updated•11 years ago
|
Keywords: regression,
regressionwindow-wanted
Comment 9•11 years ago
|
||
OK, I still need that regression range so I can get a suitable assignee.
Flags: needinfo?(mvaughan)
Comment 10•11 years ago
|
||
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)
Keywords: regressionwindow-wanted
Comment 11•11 years ago
|
||
Redirecting needinfo to Milan since this is an APZC regression.
Component: Layout: View Rendering → Panning and Zooming
Flags: needinfo?(bugs) → needinfo?(milan)
Comment 12•11 years ago
|
||
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•11 years ago
|
||
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.
Reporter | ||
Comment 14•11 years ago
|
||
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•11 years ago
|
||
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.
Reporter | ||
Comment 16•11 years ago
|
||
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•11 years ago
|
||
(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)
Comment 18•11 years ago
|
||
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)
Comment 19•11 years ago
|
||
(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•11 years ago
|
||
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)
Updated•11 years ago
|
blocking-b2g: --- → 1.3?
Comment 21•11 years ago
|
||
(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
Comment 22•11 years ago
|
||
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)
Comment 23•11 years ago
|
||
(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.
Reporter | ||
Updated•11 years ago
|
Flags: needinfo?(nick)
Reporter | ||
Comment 24•11 years ago
|
||
I have a work around in the review queue.
Updated•11 years ago
|
Updated•11 years ago
|
Flags: needinfo?(milan)
Comment 25•11 years ago
|
||
Verified fix on Peak v1.3, new version is approved and live on Marketplace.
Comment 26•11 years ago
|
||
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)
Updated•11 years ago
|
Depends on: apz-css-transforms
Comment 27•11 years ago
|
||
(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)
Comment 28•11 years ago
|
||
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•11 years ago
|
||
(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)
Comment 31•11 years ago
|
||
Jason - we need to escalate this for V1.4. What is the issue with resolving it for V1.4?
Flags: needinfo?(kward)
Comment 32•11 years ago
|
||
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)
Comment 33•11 years ago
|
||
Having an offline conversation, will report on resolution.
Comment 34•11 years ago
|
||
(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)
Comment 35•11 years ago
|
||
(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)
Updated•11 years ago
|
QA Whiteboard: [priority]
Updated•11 years ago
|
blocking-b2g: backlog → ---
feature-b2g: --- → 2.1
Comment 37•10 years ago
|
||
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•10 years ago
|
||
A similar issue on fennec 28:
https://bugzilla.mozilla.org/show_bug.cgi?id=999885
Assignee | ||
Comment 39•10 years ago
|
||
My initial investigation is pointing towards an invalidation problem.
Assignee | ||
Comment 40•10 years ago
|
||
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)
Assignee | ||
Comment 41•10 years ago
|
||
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+
Comment 43•10 years ago
|
||
Looks like the bug was introduced in http://hg.mozilla.org/mozilla-central/rev/c67ff8ae77e2 bug 770001.
Blocks: 770001
Comment 44•10 years ago
|
||
Now that we know the cause of this bug it appears that bug 993525 is not related.
No longer depends on: apz-css-transforms
Assignee | ||
Comment 45•10 years ago
|
||
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.
Assignee | ||
Comment 46•10 years ago
|
||
(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.
Assignee | ||
Comment 47•10 years ago
|
||
Assignee | ||
Comment 48•10 years ago
|
||
Assignee | ||
Comment 49•10 years ago
|
||
Assignee | ||
Comment 50•10 years ago
|
||
Assignee | ||
Comment 51•10 years ago
|
||
The page I'm using to test on my Flame device is http://people.mozilla.org/~bballo/calc2.html.
Assignee | ||
Comment 52•10 years ago
|
||
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
Assignee | ||
Comment 53•10 years ago
|
||
Keywords: checkin-needed
Comment 54•10 years ago
|
||
I think the reftest that isn't working on emulator is worthy of a bug too.
Assignee | ||
Comment 55•10 years ago
|
||
(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•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Updated•10 years ago
|
QA Whiteboard: [priority] → [priority][2.1-feature-qa+]
Updated•10 years ago
|
Flags: in-moztrap?(npark)
Updated•10 years ago
|
QA Contact: mvaughan → npark
Updated•10 years ago
|
QA Whiteboard: [priority][2.1-feature-qa+] → [priority]
Whiteboard: [ucid:Graphic11,ft:graphic] → [ucid:Graphic11,ft:graphic][2.1-feature-qa+]
Comment 57•10 years ago
|
||
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•10 years ago
|
||
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+
Updated•10 years ago
|
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•