Closed Bug 747231 Opened 8 years ago Closed 8 years ago

layout is different (overlap / extra space) when loading from landscape and switching to portrait versus loading from portrait and switching to landscape on various websites

Categories

(Core :: Layout, defect, P1)

14 Branch
ARM
Android
defect

Tracking

()

VERIFIED FIXED
mozilla15
Tracking Status
firefox14 --- verified
firefox15 --- verified
blocking-fennec1.0 --- +

People

(Reporter: nhirata, Assigned: dbaron)

References

()

Details

(Whiteboard: readability)

Attachments

(5 files)

Attached image screenshot
1. go to http://people.mozilla.com/~nhirata/html_tp/dashboard.html in landscape
2. switch to portrait and scroll to the bottom 
3. hit the home button and then go back to fennec
4. reload in portrait 
5. switch to landscape and scroll to the bottom
6. hit the home button and then go back to fennec
5. compare step 4 versus step 6

Expected: they look about the same and readable
Actual: Going from portrait to landscape will space things out more; going from landscape to portrait will cause typing to be on top of each other.

Note:
1. also occurs on crash-stats : http://bit.ly/J6Fxnp
2. Galaxy S II, Nightly 4/19/2012
Noming for release blocker
blocking-fennec1.0: --- → ?
Please retest now that the device pixel resolution tracking has landed.
Keywords: qawanted
I am able to reproduce the issue following this scenario:
1. Open a page in landscape mode (used the crash report provided in the bug).
2. Scroll to the bottom of the page.
3. Tap the home button to minimize the app.
4. Turn the device in portrait mode.
5. Open Fennec

Actual result: Some of the text is overlapped - similar to the screenshot.

Following the steps described in the bug I was unable to reproduce the issue.

Tested on Nightly/14.0a1 2012-04-24 on HTC Desire (Android 2.2).
Keywords: qawanted
blocking-fennec1.0: ? → beta+
This looks like the result of not reflowing things that we should have reflowed, or not marking things dirty in a strong enough way.  Not sure what it has to do with kerning.
Without font inflation, the kerning does not change when switching orientation.
With font inflation, this issue still occurs.

Tested Nightly 4/26/2012 LG Revolution
Assignee: nobody → dbaron
Component: General → Layout
Priority: -- → P1
Product: Fennec Native → Core
QA Contact: general → layout
Target Milestone: --- → mozilla15
Version: Firefox 14 → 11 Branch
Version: 11 Branch → 14 Branch
Summary: kerning is different when loading from landscape and switching to portrait versus loading from portrait and switching to landscape on various websites → layout is different (overlap / extra space) when loading from landscape and switching to portrait versus loading from portrait and switching to landscape on various websites
So I think the basic problem here is that I missed handling one of the dynamic change cases when implementing font inflation.

In particular, the function MinimumFontSizeFor in nsLayoutUtils.cpp depends on the result of dx->GetClientRect(): this result is used only when font.size.inflation.minTwips has a nonzero value.  When we switch between landscape and viewport, this value changes, but we don't have any notification of the change.  When font inflation is enabled, we need to reflow in this case (probably by marking the root frame dirty with a call to PresShell::FrameNeedsReflow).

I think the weirdness of when the bug does or doesn't show up is likely related to text run expiration.


I need to figure out what needs to get hooked up to get a notification for this case.
So it does appear to fix this, except:
 * there's a noticeable reflow after switching (which maybe argues that the patch should flush reflows?)
 * even after that reflow, there's sometimes visible appearance of the old size when panning, followed by a repaint



Also, now that I know what the problem is, I'm less worried about this bug than before, and I'm not as worried about having it in beta.
blocking-fennec1.0: beta+ → ?
We'd really like to get this fixed as soon as possible, but it doesn't need to block beta.
blocking-fennec1.0: ? → +
OK, I need to compare how well the following two builds fix this:
https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/dbaron@mozilla.com-0a8efc633d4b/try-android/fennec-15.0a1.en-US.android-arm.apk
https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/dbaron@mozilla.com-c56a007fdac6/try-android/fennec-15.0a1.en-US.android-arm.apk

The difference between them is that the second one has a layout flush:
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/rev/927a42ccc700

In other words, the first build is based on:
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/f59b565f5c41/remove-duplicate-null-check
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/f59b565f5c41/track-device-width-change
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/f59b565f5c41/expose-call-children
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/f59b565f5c41/reflow-for-screen-size

and the second is based on
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/927a42ccc700/remove-duplicate-null-check
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/927a42ccc700/track-device-width-change
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/927a42ccc700/expose-call-children
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/927a42ccc700/reflow-for-screen-size

other than deciding whether I want that layout flush or not, I think these patches are ready.


(I was waiting for email from try server all day.  Turns out it just didn't send the emails.)
I don't think the layout flush helps with any of the weirdness (which may or may not be related to font inflation at all).
Comment on attachment 620946 [details] [diff] [review]
Reflow for screen size change when font.size.inflation.minTwips is set.  (, patch 4)

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

::: dom/base/nsDOMWindowUtils.cpp
@@ +343,5 @@
> +      // hook in the needed updates here rather than adding a
> +      // separate notification just for this change.
> +      if (presContext &&
> +          nsLayoutUtils::FontSizeInflationEnabled(presContext) &&
> +          nsLayoutUtils::FontSizeInflationMinTwips() != 0) {

Move this entire block to a helper function maybe?
Attachment #620946 - Flags: review?(roc) → review+
Depends on: 752428
This is reproducible on:
Firefox Native 14.0b1 build2 (2012-05-09)
Device: HTC Desire Z
OS: Android 2.3.3
Comment on attachment 620943 [details] [diff] [review]
Remove duplicate null check of presShell.  (, patch 1)

Note:  this comment applies to the set of patches.

[Approval Request Comment]
Regression caused by (bug #): bug 627842
User impact if declined: incorrect display (layout vs. rendering using different font size inflation) when switching between landscape and portrait
Testing completed (on m-c, etc.): on mozilla-central
Risk to taking this patch (and alternatives if risky): given the testing (and also approval of the null-check crash fix), relatively low, though it is introducing a new codepath that runs in this case
String changes made by this patch: none
Attachment #620943 - Flags: approval-mozilla-aurora?
Attachment #620944 - Flags: approval-mozilla-aurora?
Attachment #620945 - Flags: approval-mozilla-aurora?
Attachment #620946 - Flags: approval-mozilla-aurora?
Duplicate of this bug: 754706
Attachment #620943 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #620944 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #620945 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #620946 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified 2012-05-17 Nightly, Aurora and 14 beta 2.
Status: RESOLVED → VERIFIED
In my testing, this has regressed, at least on nightly.  Any chance I could get somebody to find a regression window for when it regressed?
Status: VERIFIED → RESOLVED
Closed: 8 years ago8 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
2012-05-17-03-05-23-mozilla-central-android works
2012-05-29-03-05-18-mozilla-central-android works
2012-06-08-03-05-25-mozilla-central-android works

Maybe it's just something with my try server build...
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
I still see this working on the 06-08-2012 nightly build.  removing qawanted, regressionwindow-wanted.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.