The default bug view has changed. See this FAQ.

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

VERIFIED FIXED in Firefox 14

Status

()

Core
Layout
P1
normal
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: nhirata, Assigned: dbaron)

Tracking

14 Branch
mozilla15
ARM
Android
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox14 verified, firefox15 verified, blocking-fennec1.0 +)

Details

(Whiteboard: readability, URL)

Attachments

(5 attachments)

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
Noming for release blocker
blocking-fennec1.0: --- → ?

Comment 2

5 years ago
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

Updated

5 years ago
blocking-fennec1.0: ? → beta+
(Assignee)

Comment 4

5 years ago
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)

Updated

5 years ago
Assignee: nobody → dbaron
Component: General → Layout
Priority: -- → P1
Product: Fennec Native → Core
QA Contact: general → layout
Target Milestone: --- → mozilla15
Version: Firefox 14 → 11 Branch
(Assignee)

Updated

5 years ago
Version: 11 Branch → 14 Branch
(Assignee)

Updated

5 years ago
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
(Assignee)

Comment 6

5 years ago
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.
(Assignee)

Comment 7

5 years ago
A try build that I think should fix this is:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/dbaron@mozilla.com-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
(Assignee)

Comment 8

5 years ago
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.

Updated

5 years ago
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: ? → +
(Assignee)

Comment 10

5 years ago
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.)
(Assignee)

Comment 11

5 years ago
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).
(Assignee)

Comment 12

5 years ago
Created attachment 620943 [details] [diff] [review]
Remove duplicate null check of presShell.  (, patch 1)
Attachment #620943 - Flags: review?(roc)
(Assignee)

Comment 13

5 years ago
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)
Attachment #620944 - Flags: review?(roc)
(Assignee)

Comment 14

5 years ago
Created attachment 620945 [details] [diff] [review]
Expose DocumentViewerImpl::CallChildren with an API that fills an array.  (, patch 3)
Attachment #620945 - Flags: review?(roc)
(Assignee)

Comment 15

5 years ago
Created attachment 620946 [details] [diff] [review]
Reflow for screen size change when font.size.inflation.minTwips is set.  (, patch 4)
Attachment #620946 - Flags: review?(roc)
Attachment #620943 - Flags: review?(roc) → review+
Attachment #620944 - Flags: review?(roc) → review+
Attachment #620945 - Flags: review?(roc) → review+
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+
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
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
(Assignee)

Comment 18

5 years ago
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
(Assignee)

Updated

5 years ago
Depends on: 752428

Comment 19

5 years ago
This is reproducible on:
Firefox Native 14.0b1 build2 (2012-05-09)
Device: HTC Desire Z
OS: Android 2.3.3
status-firefox14: --- → affected
(Assignee)

Updated

5 years ago
Blocks: 627842
(Assignee)

Comment 20

5 years ago
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?
(Assignee)

Updated

5 years ago
Attachment #620944 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

5 years ago
Attachment #620945 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

5 years ago
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+
(Assignee)

Comment 22

5 years ago
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
status-firefox14: affected → fixed
Verified 2012-05-17 Nightly, Aurora and 14 beta 2.
Status: RESOLVED → VERIFIED
status-firefox14: fixed → verified
status-firefox15: --- → verified
(Assignee)

Comment 24

5 years ago
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
Last Resolved: 5 years ago5 years ago
Keywords: qawanted, regressionwindow-wanted
(Assignee)

Updated

5 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 25

5 years ago
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
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
I still see this working on the 06-08-2012 nightly build.  removing qawanted, regressionwindow-wanted.
Keywords: qawanted, regressionwindow-wanted
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.