Closed Bug 993004 Opened 6 years ago Closed 6 years ago

[B2G][Dialer][Callbar]The callbar is invisible when viewed in landscape orientation

Categories

(Core :: Graphics: Layers, defect)

30 Branch
ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31
blocking-b2g 1.4+
Tracking Status
firefox29 --- wontfix
firefox30 --- fixed
firefox31 --- fixed
b2g-v1.3 --- unaffected
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: JMercado, Assigned: nical)

References

Details

(Keywords: regression)

Attachments

(3 files, 1 obsolete file)

Description:
When viewed in landscape orientation, the callbar is invisible.  This does not prevent the user from interacting with it to return to the dialer.

Repro Steps:
1) Update a buri to BuildID: 20140407000203
2) Make a call
3) Open an application that can be viewed in landscape orientation (such as the browser)
4) View the callbar in landscape orientation

Actual:
The callbar is invisible.

Expected:
The callbar can be seen and provides information about the call in progress.

1.4 Environmental Variables:
Device: buri 1.4 MOZ
BuildID: 20140407000203
Gaia: 86de7fcce674ef6196d68e7e23552d219a3d72db
Gecko: 6e028297be14
Version: 30.0a2
Firmware Version: v1.2-device.cfg


Notes:

Repro frequency: 100%
See attached: screenshot, logcat
This issue does not occur on the 1.3 Buri MOZ RIL

1.3 Environmental Variables:
Device: buri 1.3 MOZ
BuildID: 20140401164001
Gaia: c5cd3a11e91339163b351d50769eaca0067da860
Gecko: 5045a67b47ed
Version: 28.0
Firmware Version: v1.2-device.cfg

The callbar appears when viewed in landscape mode.
blocking-b2g: --- → 1.4?
Component: Gaia::Dialer → Gaia::System::Window Mgmt
QA Contact: jzimbrick
Mozilla-Inbound Regression Window:

Last Working Environmental Variables: 
Device: Buri
BuildID: 20140311145711
Gaia: 3005269d4dcabcc7d27eaf72bda44a969873af8c
Gecko: aab0b6f3e345
Version: 30.0a1
Base Image: V1.2-device.cfg

First Broken Environmental Variables:
Device: Buri
BuildID: 20140311152010
Gaia: 3005269d4dcabcc7d27eaf72bda44a969873af8c
Gecko: 499a25915134
Version: 30.0a1
Base Image: V1.2-device.cfg

The Gaia ID is the same on both builds, indicating a Gecko issue.

Gecko Pushlog: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=aab0b6f3e345&tochange=499a25915134
Tiling regression.
Blocks: 981794
Component: Gaia::System::Window Mgmt → Graphics: Layers
Product: Firefox OS → Core
Version: unspecified → 30 Branch
regression from 1.3.   blocking1.4+
blocking-b2g: 1.4? → 1.4+
Hi, Milan, can you help to find someone to check this issue? Thank you very much.
Flags: needinfo?(milan)
This happens on both landscape orientations?
:nical, see my question in https://bugzilla.mozilla.org/show_bug.cgi?id=981794#c7
Assignee: nobody → nical.bugzilla
Flags: needinfo?(milan) → needinfo?(nical.bugzilla)
The comment Milan mentioned is:

> I have a feeling this isn't quite right, and that either 90 or 270 case should 
> be (aClip.y, aClip.width-aClip.x, aClip.height, aClip.width)

I wouldn't be surprised, it's worth a shot.
Is this bar supposed to be a tiled layer, though?
Flags: needinfo?(nical.bugzilla)
Not sure - let me play with it a bit, you can catch up tomorrow.
(In reply to Milan Sreckovic [:milan] from comment #10)
> Not sure - let me play with it a bit, you can catch up tomorrow.

Or not. Having trouble finding a phone where both accelerometer work and it will take one of the sims I have.
Milan, is there anything we can help here? Thanks.
I can confirm that this bugs is due to wrong clipping of the tiles here: http://dxr.mozilla.org/mozilla-central/source/gfx/layers/composite/TiledContentHost.cpp#349
Looking into it.
Ok, so I have it working now: the proper computation is:

    case ROTATION_0:
      result = aClip;
      break;
    case ROTATION_90:
      result = gfx::Rect(aClip.y,
                         destSize.width - aClip.x - aClip.width,
                         aClip.height, aClip.width);
      break;
    case ROTATION_270:
      result = gfx::Rect(destSize.height - aClip.y - aClip.height,
                         aClip.x,
                         aClip.height, aClip.width);
      break;
    case ROTATION_180:
      result = gfx::Rect(destSize.width - aClip.x - aClip.width,
                         destSize.height - aClip.y - aClip.height,
                         aClip.width, aClip.height);
      break;

In addition, in order to properly support rendering to intermediate surfaces, we must add the offset to the intermediate surface before doing this computation (currently we are doing it after).

So this also fixes a bug where typing a long string into an input field in landscape mode makes the text disappear.

I originally thought zoom levels would need to be part of the equation but actually they don't since the clip space isn't transformed by panning and zooming.

I am still looking into the proper place to get destSize (which is basically the screen resolution. I originally though that it had to be the size of the destination surface (the screen OR the intermediate surface) but it appears that it always works with the screen size even when rendering into an intermediate surface. Plus we don't currently have access to the size of render targets (they always return (0, 0) and print warnings about us not being supposed to ask for the size.
All compositor implems have an mSize member that I could expose to the base class, but some implementation don't actually seem to keep it up to date (for example in the basic compositor it looks like an unused member).
Also, worth noting, it looks like the compositor receives the screen orientation before the content process repaints content, which causes us to start using the clipping coordinates of the next orientation before we actually display the next orientation (for half a second or so). there must be a better place to update the compositor's mScreenRotation so that it is in sync with the content.
(In reply to Nicolas Silva [:nical] from comment #14)
> Ok, so I have it working now: the proper computation is:
> 
>     case ROTATION_0:
>       result = aClip;
>       break;
>     case ROTATION_90:
>       result = gfx::Rect(aClip.y,
>                          destSize.width - aClip.x - aClip.width,
>                          aClip.height, aClip.width);
>       break;
>     case ROTATION_270:
>       result = gfx::Rect(destSize.height - aClip.y - aClip.height,
>                          aClip.x,
>                          aClip.height, aClip.width);
>       break;
>     case ROTATION_180:
>       result = gfx::Rect(destSize.width - aClip.x - aClip.width,
>                          destSize.height - aClip.y - aClip.height,
>                          aClip.width, aClip.height);
>       break;

Hey, Botond, looks like our whiteboard computation was right :)
woops, the previous patch was empty. Botond will be a better reviewer for this, since he already had to wrap his head around the problem.
Attachment #8411808 - Attachment is obsolete: true
Attachment #8411808 - Flags: review?(bas)
Attachment #8411846 - Flags: review?(botond)
(In reply to Nicolas Silva [:nical] from comment #15)
> Also, worth noting, it looks like the compositor receives the screen
> orientation before the content process repaints content, which causes us to
> start using the clipping coordinates of the next orientation before we
> actually display the next orientation (for half a second or so). there must
> be a better place to update the compositor's mScreenRotation so that it is
> in sync with the content.

I filed bug 1000916 as a followup for this.
I may not have a correct visual of what is going on, and that may mean others don't either, so I would maybe draw some pictures and attach them.  In the meantime, I'll just comment based on my mental image :)

(In reply to Nicolas Silva [:nical] from comment #14)
> Ok, so I have it working now: the proper computation is:
> 
>     case ROTATION_0:
>       result = aClip;
>       break;
>     case ROTATION_90:
>       result = gfx::Rect(aClip.y,
>                          destSize.width - aClip.x - aClip.width,

Double check if this is not:

                           destSize.height - aClip.x - aClip.width

instead?

>                          aClip.height, aClip.width);
>       break;
>     case ROTATION_270:
>       result = gfx::Rect(destSize.height - aClip.y - aClip.height,

Double check if this is not:

                            destSize.width - aClip.y - aClip.height

instead?

>                          aClip.x,
>                          aClip.height, aClip.width);
>       break;
>     case ROTATION_180:
>       result = gfx::Rect(destSize.width - aClip.x - aClip.width,
>                          destSize.height - aClip.y - aClip.height,
>                          aClip.width, aClip.height);
>       break;
(In reply to Milan Sreckovic [:milan] from comment #20)
> I may not have a correct visual of what is going on, and that may mean
> others don't either, so I would maybe draw some pictures and attach them. 
> In the meantime, I'll just comment based on my mental image :)

I'll attach some pictures but in the mean time, I already checked what you suggest. The reason it makes sens that the computation substracts/adds widths to Xs and heights to Ys is that we are going from clip-space to layer space. in clip-space, the x and y axis correspond to the ones of the destination buffer, which means that width is along X and y is along Y.

If you were going the other way around (from layer space to clip space) you would indeed have to substract/add X axis to heights and Y axis to width
(In reply to Nicolas Silva [:nical] from comment #21)
> destination buffer, which means that width is along X and y is along Y.

...and height is along Y, sorry
Comment on attachment 8411846 [details] [diff] [review]
Fix the clipping transformation when the screen is rotated

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

After some IRC and email discussion with Milan and Nicolas I am convinced that this is correct.
Attachment #8411846 - Flags: review?(botond) → review+
Comment on attachment 8411846 [details] [diff] [review]
Fix the clipping transformation when the screen is rotated

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

It should be noted I suspect you're getting the -client size- of the widget here rather than the Widget size. Which on mobile platforms would usually be the same, but on desktop would not. But that's just a naming issue since for your purposes the client size, I believe, would be the correct one.
Fixed the error caused by silly enum sentinel values not being taken in he switch statement and landed.
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d116167774f
sorry nicolas has to backout this checking since it caused bustages like https://tbpl.mozilla.org/php/getParsedLog.php?id=38471754&tree=Mozilla-Inbound
also missed a #include of gfx2DGlue.h https://hg.mozilla.org/integration/mozilla-inbound/rev/863ba80b665f on Windows
https://hg.mozilla.org/mozilla-central/rev/a14c6bf99d13
https://hg.mozilla.org/mozilla-central/rev/863ba80b665f
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.