Closed
Bug 1267928
Opened 8 years ago
Closed 8 years ago
navigation menu for openstreetmap is cutoff
Categories
(Firefox for Android Graveyard :: Toolbar, defect)
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)
1.08 MB,
image/png
|
Details | |
994 bytes,
patch
|
botond
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
1.36 KB,
patch
|
botond
:
review+
lizzard
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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.
Comment 1•8 years ago
|
||
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:
--- → ?
Updated•8 years ago
|
Flags: needinfo?(ioana.chiorean)
Comment 3•8 years ago
|
||
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+
Comment 4•8 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•8 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•8 years ago
|
||
This will fix the auto zoom on focus when the bounds are empty.
Assignee | ||
Comment 8•8 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•8 years ago
|
Attachment #8751912 -
Flags: review?(botond)
Assignee | ||
Comment 9•8 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•8 years ago
|
||
Attachment #8751912 -
Attachment is obsolete: true
Attachment #8751912 -
Flags: review?(botond)
Attachment #8752250 -
Flags: review?(botond)
Assignee | ||
Comment 11•8 years ago
|
||
Attachment #8752251 -
Flags: review?(botond)
Assignee | ||
Comment 12•8 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•8 years ago
|
Attachment #8752250 -
Flags: review?(botond) → review+
Comment 13•8 years ago
|
||
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•8 years ago
|
||
Updated to address review comments.
Attachment #8752251 -
Attachment is obsolete: true
Attachment #8752333 -
Flags: review?(botond)
Comment 15•8 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 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 16•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a2ef7d4ebdd4 https://hg.mozilla.org/integration/mozilla-inbound/rev/0d2fe6baefe3
Comment 17•8 years ago
|
||
bugherder |
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
Comment 18•8 years ago
|
||
Hi Florin, can we have some QA's help to verify this?
Flags: needinfo?(florin.mezei)
Comment 19•8 years ago
|
||
Moving needinfo to Ioana who leads the Mobile QA team.
Flags: needinfo?(florin.mezei) → needinfo?(ioana.chiorean)
Comment 20•8 years ago
|
||
I am not able to reproduce this on Nightly ( 05/18). Setting the flag accordingly.
Assignee | ||
Comment 21•8 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•8 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•8 years ago
|
||
setting back to normal ( was changed by mistake)
Severity: critical → normal
Updated•8 years ago
|
Component: General → Graphics, Panning and Zooming
OS: Unspecified → Android
Hardware: Unspecified → All
Comment 24•8 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 Fix for recent regression, verified on nightly, let's take it for 48 aurora.
Attachment #8752333 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 25•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/95ddeda3ebef https://hg.mozilla.org/releases/mozilla-aurora/rev/59a8c40a2a17
Updated•8 years ago
|
Attachment #8752250 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
Comment 26•1 year ago
|
||
Removing regressionwindow-wanted
keyword because this bug has been resolved.
Keywords: regressionwindow-wanted
Comment 27•1 year ago
|
||
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.
Description
•