Last Comment Bug 747231 - 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
: layout is different (overlap / extra space) when loading from landscape and s...
Status: VERIFIED FIXED
readability
:
Product: Core
Classification: Components
Component: Layout (show other bugs)
: 14 Branch
: ARM Android
: P1 normal (vote)
: mozilla15
Assigned To: David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch)
:
Mentors:
http://people.mozilla.com/~nhirata/ht...
: 754706 (view as bug list)
Depends on: 752428
Blocks: font-inflation
  Show dependency treegraph
 
Reported: 2012-04-19 16:00 PDT by Naoki Hirata :nhirata (please use needinfo instead of cc)
Modified: 2012-06-08 14:24 PDT (History)
14 users (show)
ryanvm: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
verified
verified
+


Attachments
screenshot (86.18 KB, image/png)
2012-04-19 16:00 PDT, Naoki Hirata :nhirata (please use needinfo instead of cc)
no flags Details
Remove duplicate null check of presShell. (, patch 1) (3.46 KB, patch)
2012-05-03 20:30 PDT, David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch)
roc: review+
mark.finkle: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
Cache the last device width we used for font inflation on the pres context so that we can track when it changes. (, patch 2) (5.43 KB, patch)
2012-05-03 20:30 PDT, David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch)
roc: review+
mark.finkle: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
Expose DocumentViewerImpl::CallChildren with an API that fills an array. (, patch 3) (3.17 KB, patch)
2012-05-03 20:31 PDT, David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch)
roc: review+
mark.finkle: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
Reflow for screen size change when font.size.inflation.minTwips is set. (, patch 4) (7.71 KB, patch)
2012-05-03 20:31 PDT, David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch)
roc: review+
mark.finkle: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Naoki Hirata :nhirata (please use needinfo instead of cc) 2012-04-19 16:00:49 PDT
Created attachment 616784 [details]
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
Comment 1 Naoki Hirata :nhirata (please use needinfo instead of cc) 2012-04-20 15:23:49 PDT
Noming for release blocker
Comment 2 JP Rosevear [:jpr] 2012-04-23 12:15:48 PDT
Please retest now that the device pixel resolution tracking has landed.
Comment 3 Adrian Tamas (:AdrianT) 2012-04-24 04:36:43 PDT
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).
Comment 4 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2012-04-26 11:23:48 PDT
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.
Comment 5 Naoki Hirata :nhirata (please use needinfo instead of cc) 2012-04-26 11:27:02 PDT
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
Comment 6 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2012-04-29 08:11:49 PDT
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.
Comment 8 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2012-05-02 11:05:28 PDT
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.
Comment 9 Mark Finkle (:mfinkle) (use needinfo?) 2012-05-02 11:47:32 PDT
We'd really like to get this fixed as soon as possible, but it doesn't need to block beta.
Comment 10 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2012-05-03 20:05:22 PDT
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.)
Comment 11 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2012-05-03 20:27:35 PDT
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 12 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2012-05-03 20:30:44 PDT
Created attachment 620943 [details] [diff] [review]
Remove duplicate null check of presShell.  (, patch 1)
Comment 13 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2012-05-03 20:30:56 PDT
Created attachment 620944 [details] [diff] [review]
Cache the last device width we used for font inflation on the pres context so that we can track when it changes.  (, patch 2)
Comment 14 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2012-05-03 20:31:08 PDT
Created attachment 620945 [details] [diff] [review]
Expose DocumentViewerImpl::CallChildren with an API that fills an array.  (, patch 3)
Comment 15 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2012-05-03 20:31:22 PDT
Created attachment 620946 [details] [diff] [review]
Reflow for screen size change when font.size.inflation.minTwips is set.  (, patch 4)
Comment 16 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-03 20:50:24 PDT
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?
Comment 19 Paul Feher 2012-05-10 01:35:17 PDT
This is reproducible on:
Firefox Native 14.0b1 build2 (2012-05-09)
Device: HTC Desire Z
OS: Android 2.3.3
Comment 20 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2012-05-14 01:35:58 PDT
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
Comment 21 Naoki Hirata :nhirata (please use needinfo instead of cc) 2012-05-14 11:38:44 PDT
*** Bug 754706 has been marked as a duplicate of this bug. ***
Comment 23 Kevin Brosnan [:kbrosnan] 2012-05-17 19:07:19 PDT
Verified 2012-05-17 Nightly, Aurora and 14 beta 2.
Comment 24 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2012-06-08 12:10:14 PDT
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?
Comment 25 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2012-06-08 13:25:03 PDT
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...
Comment 26 Naoki Hirata :nhirata (please use needinfo instead of cc) 2012-06-08 14:24:29 PDT
I still see this working on the 06-08-2012 nightly build.  removing qawanted, regressionwindow-wanted.

Note You need to log in before you can comment on or make changes to this bug.