Closed Bug 1267928 Opened 8 years ago Closed 8 years ago

navigation menu for openstreetmap is cutoff

Categories

(Firefox for Android Graveyard :: Toolbar, defect)

All
Android
defect
Not set
normal

Tracking

(firefox46 unaffected, firefox47 unaffected, firefox48+ fixed, firefox49+ verified, fennec48+)

RESOLVED FIXED
Firefox 49
Tracking Status
firefox46 --- unaffected
firefox47 --- unaffected
firefox48 + fixed
firefox49 + verified
fennec 48+ ---

People

(Reporter: Usul, Assigned: rbarker)

References

()

Details

(Keywords: regression)

Attachments

(3 files, 2 obsolete files)

Open openstreetmap.org and see that the map navigating meny (the vertical rectangle containing + - etc) on the right og the screen is cut off.
It looks ok on release.
I don't have 47 installed on my phone at this moment but 46 is unaffected. This is a recent regression and should understand it before shipping 47.
tracking-fennec: --- → ?
Tracking for 49. Need info on Ioana to see whether 47 and 48 is affected.
Flags: needinfo?(ioana.chiorean)
This looks ok on my Nexus 6 with 47.0b2. I can see and use the + - zoom menu. 
Tracking for 48 and 49.
Assignee: nobody → esawin
tracking-fennec: ? → 48+
Hi all, 
I had a look at this issue and I can see that for some reasons it loads ok at the first time - for a few seconds- and after that it pans and the menu and zooming buttons look cut. After the first load or first panning going again to the page, loading it, will display the buttons correctly. 
Indeed only Nightly(49) and Aurora(48) are affected. 

Checking on Chrome, it goes ok from first time. I think it is more like a webcompat issue. 

Please see the video: https://youtu.be/SmSDJCRN-pU

I will look tomorrow morning for a Regression.
Flags: needinfo?(ioana.chiorean)
If this is only 48/49 then it feels like an APZ issue. -> Randall.
Assignee: esawin → rbarker
I think the real issue is that Fennec seems to think there is an input field in focus and the keyboard comes up (this doesn't happen in Chrome although they may just be ignoring it?). Both JPZ and APZ have issues, the reason it presents differently, I believe, is because the zoom-to-focused-field works in APZ and is broken in JPZ. I'll see if I can figure out what field it is zooming to and why we are panning past the left margin of the page (my guess is the boundaries haven't been propagated to APZ when the zoom-to is issued). I'll look and see if that is the case.
This will fix the auto zoom on focus when the bounds are empty.
So the issue appears to be that an <input> field which has autofocus="autofocus" (MDN says it should be a boolean?) When the input gets focus, we try zoom to the input field. However it has an empty bounds (not calculated yet?) which becomes (x=-15,y=0,width=30,height=0). Since APZ doesn't know the page dimensions yet, it pans the -15 and shifts the right side of the page off the screen by 15 pixels.
Attachment #8751912 - Flags: review?(botond)
Also, neither Chrome nor Mobile Safari seem to be autofocusing the input element as neither one brings up a key board on page load.
Explanation for second patch from irc#apz: AsyncPanZoomController::ZoomToRect has an issue when aRect is empty yet has a value in one axis i.e. (-15,0)30x0 . The easiest fix is to bail out at the top if aRect is empty. However if we want to be able to zoom in on a point i.e.  (123,456)0x0 then that will be a little more involved. I went with bail out on empty aRect at the top where we check if aRect is finite.
Attachment #8752250 - Flags: review?(botond) → review+
Comment on attachment 8752251 [details] [diff] [review]
0002-Bug-1267928-Part-2-Prevent-AsyncPanZoomController-ZoomToRect-from-processing-empty-rect-r-16051309-a11e108.patch

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

APZC::ZoomToRect() is used for two purposes:

  1) To zoom to focused input. The rect to zoom to is calculated in
     nsDOMWindowUtils::ZoomToFocusedInput().

  2) To handle double-tap-to-zoom. The rect to zoom to is calculated
     in CalculateRectToZoomTo(), declared in DoubleTapToZoom.h.

The latter explicitly returns |CSSRect()| to indicate that we should zoom out instead, and APZC::ZoomToRect() interpret it as such. This patch would break that.

Can you explain the motivation for this change in more detail? You said:

(In reply to Randall Barker [:rbarker] from comment #12)
> AsyncPanZoomController::ZoomToRect has an issue when aRect is empty yet has
> a value in one axis i.e. (-15,0)30x0.

What issue is that?
Attachment #8752251 - Flags: review?(botond) → review-
Updated to address review comments.
Attachment #8752251 - Attachment is obsolete: true
Attachment #8752333 - Flags: review?(botond)
Comment on attachment 8752333 [details] [diff] [review]
0002-Bug-1267928-Part-2-Prevent-AsyncPanZoomController-ZoomToRect-from-processing-empty-rect-r-16051313-ee8f2fa.patch

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

Thanks!

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +3623,5 @@
>  void AsyncPanZoomController::ZoomToRect(CSSRect aRect, const uint32_t aFlags) {
>    if (!aRect.IsFinite()) {
>      NS_WARNING("ZoomToRect got called with a non-finite rect; ignoring...");
>      return;
> +  } else if (aRect.IsEmpty() && (aFlags & DISABLE_ZOOM_OUT)) {

// Double-tap-to-zooming uses an empty rect to mean "zoom out".
// If zooming out is disabled, an empty rect is nonsensical
// and will produce undesirable scrolling.
Attachment #8752333 - Flags: review?(botond) → review+
https://hg.mozilla.org/mozilla-central/rev/a2ef7d4ebdd4
https://hg.mozilla.org/mozilla-central/rev/0d2fe6baefe3
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Hi Florin, can we have some QA's help to verify this?
Flags: needinfo?(florin.mezei)
Moving needinfo to Ioana who leads the Mobile QA team.
Flags: needinfo?(florin.mezei) → needinfo?(ioana.chiorean)
I am not able to reproduce this on Nightly ( 05/18). Setting the flag accordingly.
Severity: normal → critical
Flags: needinfo?(ioana.chiorean)
Comment on attachment 8752250 [details] [diff] [review]
0001-Bug-1267928-Part-1-Do-not-auto-zoom-on-focused-content-that-has-an-empty-bounds-r-16051309-12cd15f.patch

Approval Request Comment
[Feature/regressing bug #]:Bug 1267928
[User impact if declined]:Open Street Maps will open -15 pixels off the margins in Fennec causing some of the UI to be clipped.
[Describe test coverage new/current, TreeHerder]: none
[Risks and why]: Very low risk, just added bounds checking to auto zoom code.
[String/UUID change made/needed]: none.
Attachment #8752250 - Flags: approval-mozilla-aurora?
Comment on attachment 8752333 [details] [diff] [review]
0002-Bug-1267928-Part-2-Prevent-AsyncPanZoomController-ZoomToRect-from-processing-empty-rect-r-16051313-ee8f2fa.patch

Approval Request Comment
[Feature/regressing bug #]:Bug 1267928
[User impact if declined]:Open Street Maps will open -15 pixels off the margins in Fennec causing some of the UI to be clipped.
[Describe test coverage new/current, TreeHerder]: none
[Risks and why]: Very low risk, just added bounds checking to auto zoom code.
[String/UUID change made/needed]: none.
Attachment #8752333 - Flags: approval-mozilla-aurora?
setting back to normal ( was changed by mistake)
Severity: critical → normal
Component: General → Graphics, Panning and Zooming
OS: Unspecified → Android
Hardware: Unspecified → All
Comment on attachment 8752333 [details] [diff] [review]
0002-Bug-1267928-Part-2-Prevent-AsyncPanZoomController-ZoomToRect-from-processing-empty-rect-r-16051313-ee8f2fa.patch

Fix for recent regression, verified on nightly, let's take it for 48 aurora.
Attachment #8752333 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8752250 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Product: Firefox for Android → Firefox for Android Graveyard

Removing regressionwindow-wanted keyword because this bug has been resolved.

Removing regressionwindow-wanted keyword because this bug has been resolved.

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