Closed
Bug 728614
Opened 12 years ago
Closed 12 years ago
Fix the zoom level on rotation on Maple
Categories
(Firefox for Android Graveyard :: General, defect, P1)
Tracking
(firefox14 verified, firefox15 verified, blocking-fennec1.0 beta+)
VERIFIED
FIXED
Firefox 14
People
(Reporter: jpr, Assigned: ehsan.akhgari)
References
Details
(Whiteboard: MAPLE mwc-demo)
Attachments
(9 files, 6 obsolete files)
7.63 KB,
patch
|
dougt
:
review+
|
Details | Diff | Splinter Review |
71.41 KB,
patch
|
Details | Diff | Splinter Review | |
1.56 KB,
patch
|
dougt
:
review+
|
Details | Diff | Splinter Review |
12.63 KB,
patch
|
dougt
:
review+
|
Details | Diff | Splinter Review |
12.13 KB,
patch
|
cwiiis
:
review+
|
Details | Diff | Splinter Review |
2.43 KB,
patch
|
dougt
:
review+
|
Details | Diff | Splinter Review |
3.37 KB,
patch
|
kats
:
review+
|
Details | Diff | Splinter Review |
3.96 KB,
patch
|
kats
:
review+
|
Details | Diff | Splinter Review |
1.71 KB,
patch
|
kats
:
review-
|
Details | Diff | Splinter Review |
Refactor viewport implementation to remove js parts of the implementation, this will clean up the code to make it easier to fix related issues.
Comment 1•12 years ago
|
||
Attachment #598585 -
Flags: review?(doug.turner)
Comment 2•12 years ago
|
||
Attachment #598585 -
Attachment is obsolete: true
Attachment #598586 -
Flags: review?(doug.turner)
Attachment #598585 -
Flags: review?(doug.turner)
Comment 3•12 years ago
|
||
Comment on attachment 598586 [details] [diff] [review] patch Review of attachment 598586 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/GeckoApp.java @@ +1305,5 @@ > + > + GeckoAppShell.sendEventToGecko( > + GeckoEvent.createMetaViewportQueryEvent(tab.getId(), > + new GeckoEvent.Callback() { > + public void callback(GeckoEvent ev, String data) { This callback does nothing. I don't see the point. ::: mobile/android/base/GeckoEvent.java @@ +390,4 @@ > return event; > } > > + public static GeckoEvent createMetaViewportQueryEvent(int tabId, Callback callback) { semicolon isn't spaced correctly. ::: widget/android/AndroidJavaWrappers.cpp @@ -616,4 @@ > break; > } > > - case SCREENSHOT: { This was removed, but: private static final int SCREENSHOT = 25; was not removed. @@ +673,5 @@ > +AndroidGeckoEvent::DoCallback(const nsAString& data) { > + JNIEnv* env = AndroidBridge::GetJNIEnv(); > + if (!env) > + return; > + jstring jData = env->NewString(nsPromiseFlatString(data).get(), data.Length()); You want to have a jni frame here: AndroidBridge::AutoLocalJNIFrame(env, 1); ::: widget/android/nsAppShell.cpp @@ +461,5 @@ > + if (!domWindow) > + break; > + nsCOMPtr<nsIDOMDocument> domDocument; > + domWindow->GetDocument(getter_AddRefs(domDocument)); > + nsCOMPtr<nsIDocument> doc = do_QueryInterface(domDocument); test for null |doc|
Attachment #598586 -
Flags: review?(doug.turner) → review-
Comment 4•12 years ago
|
||
Attachment #598586 -
Attachment is obsolete: true
Attachment #598597 -
Flags: review?(doug.turner)
Updated•12 years ago
|
Attachment #598597 -
Flags: review?(doug.turner) → review+
Assignee | ||
Comment 5•12 years ago
|
||
This is my work in progress patch to move viewport handling to the Java UI side, and eliminate it from the browser.js side. This patch is still early in its infancy, and should be treated with care. Specifically, it should not be expected to work correctly, and much less so to link successfully! I'd be curious to see high-level comments about it, but please don't spend a lot of time reviewing the details as they will probably change.
Assignee | ||
Comment 6•12 years ago
|
||
Still completely untested.
Attachment #598612 -
Attachment is obsolete: true
Comment 7•12 years ago
|
||
Just to add a comment to reflect what I've said on IRC; I think this is a sound and good idea, but I think we should base this patch on top of a clean-up of what's already there first. I think some of the bugs we're seeing could be worked out without taking drastic measures and there's an awful lot of unused code left over from ripping out the compositor. A summary of things that need cleaning up: - viewportExcess in browser.js is unused - this needs to be removed and scrollToFocusedInput possibly rewritten - updateTransform in browser.js is unused - this can just be removed - Viewport offset needs to be removed. This is represented by ViewportMetrics.mViewportOffset in Java, and by offsetX and offsetY in the viewport array in browser.js. This is being ignored when setting the displayport in browser.js, and is causing touch events to have incorrect positions. - The viewport array in browser.js can be removed. All of its values except zoom are now unused or stored elsewhere anyway. This can be replaced with just a zoom variable, I think I think once this has been done, the code will become a bit more readable again and we can reassess from there.
Assignee | ||
Comment 8•12 years ago
|
||
Attachment #598947 -
Flags: review?(chrislord.net)
Updated•12 years ago
|
Attachment #598947 -
Flags: review?(chrislord.net) → review+
Assignee | ||
Comment 9•12 years ago
|
||
Comment on attachment 598597 [details] [diff] [review] patch http://hg.mozilla.org/projects/maple/rev/75d48305a2ef
Assignee | ||
Comment 10•12 years ago
|
||
Comment on attachment 598947 [details] [diff] [review] Part 1 - Remove updateTransform() http://hg.mozilla.org/projects/maple/rev/96c19bab97ce
Assignee | ||
Comment 11•12 years ago
|
||
Attachment #598953 -
Flags: review?(doug.turner)
Attachment #598953 -
Flags: review?(chrislord.net)
Updated•12 years ago
|
Attachment #598953 -
Flags: review?(doug.turner) → review+
Assignee | ||
Comment 12•12 years ago
|
||
https://hg.mozilla.org/projects/maple/rev/c0acd48ef78b
Assignee | ||
Comment 13•12 years ago
|
||
So one thing to note is that our current implementation of ScrollToFocusedInput is broken. You can try this by typing test in the address bar, and focusing the text box at the very end of the google search results page. The content gets scrolled to somewhere in the middle of the page, which does not include the search box.
Assignee | ||
Comment 14•12 years ago
|
||
In fact I measured the values we measure in scrollToFocusedInput for the excessViewport object. I see values similar to this: {x:0, y:0.1500244140625}. So given the fact that the current viewportExcess handling no longer works correctly, and even if it did the values would not be large enough to actually scroll the focused input into view, I will rip this code out until somebody fixes this for realz.
Comment 15•12 years ago
|
||
(In reply to Ehsan Akhgari [:ehsan] from comment #14) > In fact I measured the values we measure in scrollToFocusedInput for the > excessViewport object. I see values similar to this: {x:0, > y:0.1500244140625}. > > So given the fact that the current viewportExcess handling no longer works > correctly, and even if it did the values would not be large enough to > actually scroll the focused input into view, I will rip this code out until > somebody fixes this for realz. Please file a bug too
Assignee | ||
Comment 16•12 years ago
|
||
(In reply to Mark Finkle (:mfinkle) from comment #15) > (In reply to Ehsan Akhgari [:ehsan] from comment #14) > > In fact I measured the values we measure in scrollToFocusedInput for the > > excessViewport object. I see values similar to this: {x:0, > > y:0.1500244140625}. > > > > So given the fact that the current viewportExcess handling no longer works > > correctly, and even if it did the values would not be large enough to > > actually scroll the focused input into view, I will rip this code out until > > somebody fixes this for realz. > > Please file a bug too Removed viewportExcess: https://hg.mozilla.org/projects/maple/rev/ab2c10f183e4 and filed bug 728978.
Assignee | ||
Comment 17•12 years ago
|
||
Attachment #599230 -
Flags: review?(chrislord.net)
Comment 18•12 years ago
|
||
Comment on attachment 598953 [details] [diff] [review] Part 2: Remove viewport offsets Review of attachment 598953 [details] [diff] [review]: ----------------------------------------------------------------- r+ with the comment addressed. ::: mobile/android/base/gfx/ViewportMetrics.java @@ +131,5 @@ > } > > public PointF getDisplayportOrigin() { > + return new PointF(mViewportRect.left, > + mViewportRect.top); This isn't right - Looking at browser.js, the viewport is always in the centre of the displayport now. I'm not sure if this matters anymore, but if it doesn't, then this function (and probably getOptimumViewportOffset) should be removed entirely.
Comment 19•12 years ago
|
||
Comment on attachment 599230 [details] [diff] [review] Part 4: Remove the viewport struct from browser.js Review of attachment 599230 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me.
Attachment #599230 -
Flags: review?(chrislord.net) → review+
Assignee | ||
Comment 20•12 years ago
|
||
(In reply to Chris Lord [:cwiiis] from comment #18) > Comment on attachment 598953 [details] [diff] [review] > Part 2: Remove viewport offsets > > Review of attachment 598953 [details] [diff] [review]: > ----------------------------------------------------------------- > > r+ with the comment addressed. > > ::: mobile/android/base/gfx/ViewportMetrics.java > @@ +131,5 @@ > > } > > > > public PointF getDisplayportOrigin() { > > + return new PointF(mViewportRect.left, > > + mViewportRect.top); > > This isn't right - Looking at browser.js, the viewport is always in the > centre of the displayport now. I'm not sure if this matters anymore, but if > it doesn't, then this function (and probably getOptimumViewportOffset) > should be removed entirely. Filed bug 729183.
Assignee | ||
Comment 21•12 years ago
|
||
(In reply to Chris Lord [:cwiiis] from comment #19) > Comment on attachment 599230 [details] [diff] [review] > Part 4: Remove the viewport struct from browser.js > > Review of attachment 599230 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks good to me. https://hg.mozilla.org/projects/maple/rev/75fdc3a367da
Comment 22•12 years ago
|
||
Attachment #599318 -
Flags: review?(doug.turner)
Comment 23•12 years ago
|
||
this patch only add refs the wrapped objects in AndroidGeckoEvents. Other subclasses of WrappedJavaObject already do their own add ref'ing so the previous patch was interfering with it.
Attachment #599318 -
Attachment is obsolete: true
Attachment #599467 -
Flags: review?(doug.turner)
Attachment #599318 -
Flags: review?(doug.turner)
Comment 24•12 years ago
|
||
Comment on attachment 599467 [details] [diff] [review] patch to add ref wrapped java objects in events (and stub out xul) lets see a follow up to remove the Dispose method on the AndroidLayerRendererFrame class.
Attachment #599467 -
Flags: review?(doug.turner) → review+
Comment 25•12 years ago
|
||
pushed attachment 598597 [details] [diff] [review] and attachment 599467 [details] [diff] [review] as https://hg.mozilla.org/projects/maple/rev/b520f34d78f1
Assignee | ||
Comment 26•12 years ago
|
||
I backed out parts of Brad's patch as I think I won't need them here. If I do, I will reland it: https://hg.mozilla.org/projects/maple/rev/580381e2805c
Summary: Refactor viewport implementation to remove js parts of the implementation → Fix the zoom level on rotation on Maple
Assignee | ||
Comment 27•12 years ago
|
||
This patch fixes the real issue. The abortAnimation call doesn't need to do anything if nothing is happening, and setting the viewport size multiple times just confuses the browser. There are still painting issues with this patch, and I will work on fixing them next.
Attachment #599639 -
Flags: review?
Assignee | ||
Updated•12 years ago
|
Attachment #599639 -
Flags: review?(chrislord.net)
Attachment #599639 -
Flags: review?(bugmail.mozilla)
Attachment #599639 -
Flags: review?
Comment 29•12 years ago
|
||
Comment on attachment 599639 [details] [diff] [review] Part 6: fix the zoom level Review of attachment 599639 [details] [diff] [review]: ----------------------------------------------------------------- r+ from me, but I'm dubious about the change below - hopefully kats will have more insight. ::: mobile/android/base/ui/PanZoomController.java @@ -230,5 @@ > case NOTHING: > // Don't do animations here; they're distracting and can cause flashes on page > // transitions. > - mController.setViewportMetrics(getValidViewportMetrics()); > - mController.notifyLayerClientOfGeometryChange(); I'm a little wary of this, but I don't know/can't remember enough to know whether removing this is dangerous or not.
Attachment #599639 -
Flags: review?(chrislord.net) → review+
Comment 30•12 years ago
|
||
(In reply to Chris Lord [:cwiiis] from comment #29) > Comment on attachment 599639 [details] [diff] [review] > Part 6: fix the zoom level > > Review of attachment 599639 [details] [diff] [review]: > ----------------------------------------------------------------- > > r+ from me, but I'm dubious about the change below - hopefully kats will > have more insight. > > ::: mobile/android/base/ui/PanZoomController.java > @@ -230,5 @@ > > case NOTHING: > > // Don't do animations here; they're distracting and can cause flashes on page > > // transitions. > > - mController.setViewportMetrics(getValidViewportMetrics()); > > - mController.notifyLayerClientOfGeometryChange(); > > I'm a little wary of this, but I don't know/can't remember enough to know > whether removing this is dangerous or not. Just to note btw, the reason I'm wary of this is that all the other cases fall into the NOTHING case - I'd be less wary if this code were moved into the case above and a break added.
Comment 31•12 years ago
|
||
Comment on attachment 599639 [details] [diff] [review] Part 6: fix the zoom level >+++ b/mobile/android/base/ui/PanZoomController.java >@@ -225,18 +225,16 @@ public class PanZoomController > // fall through > case ANIMATED_ZOOM: > // the zoom that's in progress likely makes no sense any more (such as if > // the screen orientation changed) so abort it > // fall through > case NOTHING: > // Don't do animations here; they're distracting and can cause flashes on page > // transitions. >- mController.setViewportMetrics(getValidViewportMetrics()); >- mController.notifyLayerClientOfGeometryChange(); > break; A couple of things: one is that this *might* be ok for the NOTHING case, but the other cases above also fall through to this one, and I'm pretty sure it's not good to remove this for the other cases. So you'd have to move this code up so that it still gets run for the non-NOTHING cases of the switch. The other is that if you remove this code and rotate while at the bottom-right corner of a page, does that leave the page in an overscroll position? This code was mostly intended to ensure that after unexpected viewport size changes (such as on a rotation) we don't end up in overscroll and leave it that way.
Attachment #599639 -
Flags: review?(bugmail.mozilla) → review-
Assignee | ||
Comment 32•12 years ago
|
||
Attachment #599639 -
Attachment is obsolete: true
Attachment #599671 -
Flags: review?(bugmail.mozilla)
Comment 33•12 years ago
|
||
Comment on attachment 599671 [details] [diff] [review] Part 6: fix the zoom level > >- PointF newFocus = new PointF(size.width / 2.0f, size.height / 2.0f); >+ // For rotations, we want the focus point to be at the top left. >+ boolean rotation = (size.width > oldWidth && size.height < oldHeight) || >+ (size.width < oldWidth && size.height > oldHeight); Are there any phones with screen resolutions where this might fail? The screen would have to be near-square, like the Motorola Flipout. Otherwise seems ok based on the testing we did offline.
Attachment #599671 -
Flags: review?(bugmail.mozilla) → review+
Assignee | ||
Comment 34•12 years ago
|
||
This seems to match the behavior of the stock browser. Basically, painting anything with the wrong viewport when the rotation happens makes us look bad.
Attachment #599730 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 35•12 years ago
|
||
Attachment #599731 -
Flags: review?(bugmail.mozilla)
Comment 36•12 years ago
|
||
Comment on attachment 599730 [details] [diff] [review] Part 7: pause painting when rotation happens Review of attachment 599730 [details] [diff] [review]: ----------------------------------------------------------------- Update commit message to say "viewport size" instead of "orientation" ::: mobile/android/base/gfx/GeckoLayerClient.java @@ +134,5 @@ > + // If the viewport has changed but we still don't have the latest viewport > + // from Gecko, ignore the viewport passed to us from Gecko since that is going > + // to be wrong. > + if (!mFirstPaint && mIgnorePaintsPendingViewportSizeChange && !mUpdateViewportOnEndDraw) { > + return null; The condition here doesn't need the "&& !mUpdateViewportOnEndDraw" clause, I think. It only matters if a second viewportSizeChanged gets called before the first one is completely finished processing though.
Attachment #599730 -
Flags: review?(bugmail.mozilla) → review+
Comment 37•12 years ago
|
||
Comment on attachment 599731 [details] [diff] [review] Part 8: Remove updateLayerAfterDraw Review of attachment 599731 [details] [diff] [review]: ----------------------------------------------------------------- I just landed two patches I had pending from before I left last week; one of them does this, along with a bunch of other cleanup.
Attachment #599731 -
Flags: review?(bugmail.mozilla) → review-
Updated•12 years ago
|
Keywords: fennecnative-betablocker
Assignee | ||
Comment 38•12 years ago
|
||
https://hg.mozilla.org/projects/maple/rev/e48d0ef50574 https://hg.mozilla.org/projects/maple/rev/e9930e885ef7
Assignee | ||
Comment 39•12 years ago
|
||
... and this to address the review comment: https://hg.mozilla.org/projects/maple/rev/6b4f7dee6fe8
Reporter | ||
Updated•12 years ago
|
Priority: -- → P1
Assignee | ||
Comment 40•12 years ago
|
||
Attachment #599731 -
Attachment is obsolete: true
Attachment #599779 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Updated•12 years ago
|
Attachment #598953 -
Flags: review?(chrislord.net)
Comment 41•12 years ago
|
||
Comment on attachment 599779 [details] [diff] [review] Part 8: Simplify getViewTransform Review of attachment 599779 [details] [diff] [review]: ----------------------------------------------------------------- I'm ok with taking out the useless getOrigin() call, but I'm not sure about moving that stuff outside the synchronized block. The ViewportMetrics object that you get from the layer controller can be concurrently modified by other threads (e.g. the java UI thread) and so the synchronization seems like a good idea to avoid getting inconsistent values out of it.
Attachment #599779 -
Flags: review?(bugmail.mozilla) → review-
Assignee | ||
Comment 42•12 years ago
|
||
https://hg.mozilla.org/projects/maple/rev/e66077845005
Assignee | ||
Comment 44•12 years ago
|
||
Everything that needed to be done here should be finished. The rest is bug 725091, which is visible on Galaxy Nexus, but not other phones that I have tried.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
blocking-fennec1.0: --- → beta+
Updated•12 years ago
|
Target Milestone: --- → Firefox 14
Comment 46•12 years ago
|
||
Closing bug as verified fixed on: Firefox 15.0a1 (2012-05-29) Firefox 14.0a2 (2012-05-29) Device: Galaxy Nexus OS: Android 4.0.2
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•