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

Status: VERIFIED FIXED

(Core :: Layout, defect, P1)

Description

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Attachment: 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)]

Noming for release blocker

Comment 2

[JP Rosevear [:jpr]]

Please retest now that the device pixel resolution tracking has landed.

Comment 3

[Adrian Tamas (:AdrianT)]

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-4, no longer working on Mozilla)]

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)]

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-4, no longer working on Mozilla)]

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 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

A try build that I think should fix this is:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-31774d2885e2/try-android/fennec-15.0a1.en-US.android-arm.apk

It's based on:
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/d87bc5f40a15/remove-duplicate-null-check
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/d87bc5f40a15/track-device-width-change
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/d87bc5f40a15/reflow-for-screen-size

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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?)]

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-4, no longer working on Mozilla)]

OK, I need to compare how well the following two builds fix this:
https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-0a8efc633d4b/try-android/fennec-15.0a1.en-US.android-arm.apk
https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-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-4, no longer working on Mozilla)]

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-4, no longer working on Mozilla)]

Attachment: Remove duplicate null check of presShell. (, patch 1)

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: 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-4, no longer working on Mozilla)]

Attachment: Expose DocumentViewerImpl::CallChildren with an API that fills an array. (, patch 3)

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: Reflow for screen size change when font.size.inflation.minTwips is set. (, patch 4)

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a3bbfbeab7c9
https://hg.mozilla.org/mozilla-central/rev/9bc8a2a2a460
https://hg.mozilla.org/mozilla-central/rev/95cce812c1b7
https://hg.mozilla.org/mozilla-central/rev/fa94b7958cb4

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

That was via a mozilla-inbound push that I forgot to note here:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3bbfbeab7c9
https://hg.mozilla.org/integration/mozilla-inbound/rev/9bc8a2a2a460
https://hg.mozilla.org/integration/mozilla-inbound/rev/95cce812c1b7
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa94b7958cb4

Comment 19

[Paul Feher]

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-4, no longer working on Mozilla)]

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 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0ae2d9f14d4d
https://hg.mozilla.org/releases/mozilla-aurora/rev/c29b05f31ac7
https://hg.mozilla.org/releases/mozilla-aurora/rev/42c25701174e
https://hg.mozilla.org/releases/mozilla-aurora/rev/1b3adfb8004d

Comment 23

[Kevin Brosnan [Ex-Mozilla]]

Verified 2012-05-17 Nightly, Aurora and 14 beta 2.

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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-4, no longer working on Mozilla)]

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)]

I still see this working on the 06-08-2012 nightly build.  removing qawanted, regressionwindow-wanted.