1027851 - [B2G][E-mail]When composing an email, everything typed after pressing the spacebar when typing in a recipient's email address is blurry

Status: VERIFIED FIXED

(Core :: Graphics, defect)

Description

[Andrew Stole [:astole]]

If, when typing in any of the recipient sections of an email, the spacebar is tapped on, every letter and character typed afterwards is blurred.

Repro Steps:
1) Update a Flame to BuildID: 20140619040201
2) Open the email app and sign into a vaild email account
3) Compose a new email
4) In any of the recipient sections, start typing an email address
5) Press the spacebar
6) Type more letters

Actual:
The letters typed after pressing the spacebar are blurry

Expected:
No letters are blurry

2.1 Environmental Variables:
Device: Flame 2.1
BuildID: 20140619040201
Gaia: 82e957160ca017087bd374cd051339e55b641e68
Gecko: f78e532e8a10
Version: 33.0a1
Firmware Version: v121-2

User Agent: Mozilla/5.0 (Mobile; rv:33.0) Gecko/33.0 Firefox/33.0

Repro frequency: 100%
See attached: video, logcat

Comment 1

[Andrew Stole [:astole]]

Attaching video and adding QA Wanted to test branches and devices.

Comment 2

[Eric Kramer [:Kramer]]

This issue DOES reproduce on Flame 2.0

Flame 2.0
BuildID: 20140627000201
Gaia: 8df02268fcd7e80c5fab8c1ec099772e37f8659d
Gecko: 731a5e8831e6
Version: 32.0a2 (2.0) 
Firmware Version: v122

Actual Results:
The letters typed after pressing the spacebar are blurry

=============================================================
This issue does NOT repro on Flame 1.4, Buri 2.1, Buri 1.4 Open_C 2.1

Flame 1.4
Build ID: 20140625000201
Gaia: c9416de14acf9e94ab006619cd2418c768422fcb
Gecko: cddf88f78632
Version: 30.0 (1.4) 
Firmware Version: v122

Buri Master (2.1)
Build ID: 20140627093529
Gaia: b8f36518696f3191a56e4f33447ee9d6ec820da1
Gecko: c90b38c47a1d
Version: 33.0a1 (Master) 
Firmware Version: v1.2device.cfg

Open_C Master (2.1)
Build ID: 20140627093529
Gaia: b8f36518696f3191a56e4f33447ee9d6ec820da1
Gecko: c90b38c47a1d
Version: 33.0a1 (Master) 
Firmware Version: P821A10V1.0.0B06_LOG_DL

Actual Results: 
The text appears normal with no signs of blur.

Comment 3

[Jason Smith [:jsmith]]

This is a low res tiling regression. Given the priority of visual design in 2.0, this probably needs to get fixed.

Comment 4

[Sandip Kamat]

This needs to be fixed.

Comment 5

[Sandip Kamat]

This needs to be fixed. Flagging Milan.

Comment 6

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

I investigated this (actually I investigated the issue at https://bugzilla.mozilla.org/show_bug.cgi?id=1033383#c0 because the STR are easier). What seems to happen is that the AboutToCheckerboard call in the tiling code is returning false in some cases for short scrollable elements. The reason this happens is that the actual critical displayport is a multiple of the composition bounds size, but the danger zone is a fixed amount. For small elements the danger zone is larger than the critical displayport size being used, and so it's perpetually in an "about to checkerboard" state.

Two other factors come into play here: one is the critical displayport margin values are also dependent on the zoom. The other is that we expand the critical displayport to the nearest tile boundary. This means that at different zoom levels and with different positioning of the element the bug will manifest slightly differently. There might be some more factors as well, since I don't think I've fully accounted for all the numbers I'm seeing in my log.

I'll probably get to the bottom of this tomorrow and see if I can come up with a patch.

Comment 7

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

More investigation shows that there is a second bug that is also affecting this. When I hit enter twice on the contenteditable field, it becomes scrollable and hits the code at http://mxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils.cpp?rev=97220529a789#2671 but the composition bounds computed there is wrong. This results in the displayport getting set incorrectly, which results in problems later because the displayport is never updated and it doesn't properly cover the area it should.

Comment 8

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

I'm spun off comment 7 into bug 1034179 because I don't know that code as well and the fix isn't obvious to me.

Comment 10

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

Attachment: Part 1 - Add some more logging to TiledContentClient

Comment 11

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

Attachment: Part 2 - Ensure the displayport includes the danger zone

These two patches, combined with the patch on bug 1034179, should have fixed the issue. However it still happens because there's more rounding error that we need to deal with somehow. Specifically, the composition bounds are stored as a ParentLayerIntRect, which involves rounding. When converting this rounded value back to CSS pixels in CalculatePendingDisplayPort, this results in a slightly different value, which still results in a slightly negative bottom margin for these STR

Comment 12

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

Comment on attachment 8450453 [details] [diff] [review]
Part 1 - Add some more logging to TiledContentClient

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

Might as well get these reviewed. I'll file a follow-up bug for the rounding problem.

Comment 13

[Chris Lord [:cwiiis]]

Comment on attachment 8450453 [details] [diff] [review]
Part 1 - Add some more logging to TiledContentClient

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

LGTM, couple of nits.

::: gfx/layers/client/TiledContentClient.cpp
@@ +201,5 @@
>  
>    // Always abort updates if the resolution has changed. There's no use
>    // in drawing at the incorrect resolution.
>    if (!FuzzyEquals(compositorMetrics.GetZoom().scale, contentMetrics.GetZoom().scale)) {
> +    TILING_PRLOG(("TILING: Aborting because resolution changed %f %f\n",

s/changed %f %f/changed from %.2f to %.2f/ ? (and switch the parameters accordingly)

@@ +241,5 @@
>  
>    // Abort drawing stale low-precision content if there's a more recent
>    // display-port in the pipeline.
>    if (aLowPrecision && !aHasPendingNewThebesContent) {
> +    TILING_PRLOG(("TILING: Aborting low-precision because have new pending content\n"));

s/have/there is/ or s/have/of/ ?

Comment 14

[Chris Lord [:cwiiis]]

Comment on attachment 8450459 [details] [diff] [review]
Part 2 - Ensure the displayport includes the danger zone

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

LGTM, one comment to consider and one optional nit.

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +1842,5 @@
>    CSSPoint scrollOffset = aFrameMetrics.GetScrollOffset();
>    CSSRect scrollableRect = aFrameMetrics.GetExpandedScrollableRect();
>  
>    // Calculate the displayport size based on how fast we're moving along each axis.
>    CSSSize displayPortSize = CalculateDisplayPortSize(compositionSize, velocity);

I wonder if it would be better to make this change in CalculateDisplayPortSize rather than outside of it, in case it gets used elsewhere? Your call.

@@ +1843,5 @@
>    CSSRect scrollableRect = aFrameMetrics.GetExpandedScrollableRect();
>  
>    // Calculate the displayport size based on how fast we're moving along each axis.
>    CSSSize displayPortSize = CalculateDisplayPortSize(compositionSize, velocity);
> +  // Ensure that it is at least as large as the visible area inflated by the

nit, new-line before this line?

Comment 15

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

(In reply to Chris Lord [:cwiiis] from comment #13)
> > +    TILING_PRLOG(("TILING: Aborting because resolution changed %f %f\n",
> 
> s/changed %f %f/changed from %.2f to %.2f/ ? (and switch the parameters
> accordingly)

Done, but I left it as %f because it's a FuzzyEquals with epsilon as 1e-5 so I think it's better if we print the full value. If it's off by 1e-4 or something the FuzzyEquals will fail but the printed values will be identical with %.2f which would be confusing.

> 
> s/have/there is/ or s/have/of/ ?

Done.

(In reply to Chris Lord [:cwiiis] from comment #14)
> I wonder if it would be better to make this change in
> CalculateDisplayPortSize rather than outside of it, in case it gets used
> elsewhere? Your call.

Good point, I made that change.

> >    // Calculate the displayport size based on how fast we're moving along each axis.
> >    CSSSize displayPortSize = CalculateDisplayPortSize(compositionSize, velocity);
> > +  // Ensure that it is at least as large as the visible area inflated by the
> 
> nit, new-line before this line?

Moved this into the CalculateDisplayPortSize function and spaced things appropriately.

Comment 16

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/1ed6254f6ef3
https://hg.mozilla.org/integration/mozilla-inbound/rev/954509139d29

Comment 17

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

Also this should probably stay open until the dependencies are fixed.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1ed6254f6ef3
https://hg.mozilla.org/mozilla-central/rev/954509139d29

Comment 19

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

Dependencies fixed.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

NO_UPLIFT until sufficiently baked on trunk per discussion with kats.

Comment 21

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

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/cfd8acd59ca2
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/38be883fd6a6

Comment 22

[Duane Dixon [:ddixon]]

Issue is verified fixed in Flame 2.0, 2.1 builds (Full Flash, nightly).

Actual Results: Letters typed in any recipient field remain clear.  

Device: Flame 2.0
BuildID: 20141028000202
Gaia: 5e532a84e762b1bb6231756182cf1465671a061e
Gecko: 124f0bed1700
Gonk: 6e51d9216901d39d192d9e6dd86a5e15b0641a89
Version: 32.0 (2.0)
Firmware: V188
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Device: Flame 2.1
BuildID: 20141030001201
Gaia: 3e585d8be5e2dffc376f83313299c9b6d53c3db4
Gecko: b643d78a23c6
Gonk: 48835395daa6a49b281db62c50805bd6ca24077e
Version: 34.0 (2.1)
Firmware: V188
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0