navigation menu for openstreetmap is cutoff

RESOLVED FIXED in Firefox 48

Status

()

Firefox for Android
Toolbar
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Usul, Assigned: rbarker)

Tracking

({regression, regressionwindow-wanted})

Trunk
Firefox 49
All
Android
regression, regressionwindow-wanted
Points:
---

Firefox Tracking Flags

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

Details

(URL)

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

2 years ago
Created attachment 8745871 [details]
Screenshot_2016-04-27-06-41-40.png

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: --- → ?
status-firefox46: --- → unaffected
status-firefox47: --- → ?
status-firefox48: --- → affected
tracking-firefox47: --- → ?
tracking-firefox48: --- → ?
tracking-firefox49: --- → ?
Tracking for 49. Need info on Ioana to see whether 47 and 48 is affected.
tracking-firefox49: ? → +
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.
status-firefox47: ? → unaffected
tracking-firefox47: ? → ---
tracking-firefox48: ? → +
Assignee: nobody → esawin
tracking-fennec: ? → 48+

Comment 4

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

Comment 6

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

Comment 7

2 years ago
Created attachment 8751912 [details] [diff] [review]
0001-Bug-1267928-Do-not-auto-zoom-on-focused-content-that-has-an-empty-bounds-r-16051213-cba84b6.patch

This will fix the auto zoom on focus when the bounds are empty.
(Assignee)

Comment 8

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

Updated

2 years ago
Attachment #8751912 - Flags: review?(botond)
(Assignee)

Comment 9

2 years ago
Also, neither Chrome nor Mobile Safari seem to be autofocusing the input element as neither one brings up a key board on page load.
(Assignee)

Comment 10

2 years ago
Created 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
Attachment #8751912 - Attachment is obsolete: true
Attachment #8751912 - Flags: review?(botond)
Attachment #8752250 - Flags: review?(botond)
(Assignee)

Comment 11

2 years ago
Created attachment 8752251 [details] [diff] [review]
0002-Bug-1267928-Part-2-Prevent-AsyncPanZoomController-ZoomToRect-from-processing-empty-rect-r-16051309-a11e108.patch
Attachment #8752251 - Flags: review?(botond)
(Assignee)

Comment 12

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

Updated

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

Comment 14

2 years ago
Created attachment 8752333 [details] [diff] [review]
0002-Bug-1267928-Part-2-Prevent-AsyncPanZoomController-ZoomToRect-from-processing-empty-rect-r-16051313-ee8f2fa.patch

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+

Comment 17

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a2ef7d4ebdd4
https://hg.mozilla.org/mozilla-central/rev/0d2fe6baefe3
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox49: affected → fixed
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)

Comment 20

2 years ago
I am not able to reproduce this on Nightly ( 05/18). Setting the flag accordingly.
Severity: normal → critical
status-firefox49: fixed → verified
Flags: needinfo?(ioana.chiorean)
(Assignee)

Comment 21

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

Comment 22

2 years ago
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?

Comment 23

2 years ago
setting back to normal ( was changed by mistake)
Severity: critical → normal

Updated

2 years ago
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+
You need to log in before you can comment on or make changes to this bug.