Closed
Bug 707571
Opened 13 years ago
Closed 13 years ago
user-scalable property of viewport meta tag is ignored
Categories
(Firefox for Android Graveyard :: General, defect, P2)
Tracking
(firefox14 verified, firefox15 verified, blocking-fennec1.0 +, fennec14+)
VERIFIED
FIXED
Firefox 15
People
(Reporter: steffen.wilberg, Assigned: mbrubeck)
References
()
Details
(Keywords: regression, Whiteboard: maple, [viewport])
Attachments
(6 files, 13 obsolete files)
7.29 KB,
patch
|
mbrubeck
:
review+
|
Details | Diff | Splinter Review |
16.62 KB,
patch
|
kats
:
review+
|
Details | Diff | Splinter Review |
6.09 KB,
patch
|
kats
:
review+
|
Details | Diff | Splinter Review |
9.46 KB,
patch
|
kats
:
review+
|
Details | Diff | Splinter Review |
5.36 KB,
patch
|
mfinkle
:
review+
|
Details | Diff | Splinter Review |
31.77 KB,
patch
|
mbrubeck
:
review+
akeybl
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
In about:config, I can zoom in and zoom out. I should not be able to, because config.xhtml has this line: <meta name="viewport" content="width=480; initial-scale=.6667; user-scalable=0" /> user-scalable=0 or "no" means that the user cannot interactively change the zoom factor. Current spec is here: http://dev.w3.org/csswg/css-device-adapt/#viewport-meta This is a regression from Fennec XUL: http://mxr.mozilla.org/projects-central/source/birch/mobile/xul/chrome/content/content.js#773
Assignee | ||
Comment 1•13 years ago
|
||
We had fairly frequent complaints in XUL Fennec from users who wanted to zoom sites but couldn't. We should consider whether the benefits of this feature outweigh the costs. Reasons to support user-scalable=no: * Parity with native apps, which can (and usually do) have non-scalable UI. * Content can respond to clicks faster if we can bypass double-click detection (useful for games). However, touch events provide an alternative way to get this benefit. * Even if we don't support user-scalable=no, when we add multi-touch support we will still need to allow pages to disable zooming by intercepting touch events; this is necessary for games, drawing programs, and other apps that use multi-touch input for their own purposes. Reasons not to support user-scalable=no: * Zooming is an important accessibility feature, for users who have trouble reading or interacting with small elements on screen. * It is commonly used by sites that don't really "need" it (that is, where it doesn't provide any specific benefit to users). In some cases it might be used just to work around limitations or bugs in other browsers, like position:fixed limitations in some WebKit browsers.
Comment 3•13 years ago
|
||
I just noticed this in Native Fennec. I thought this worked before. Anyway, here is a testcase: http://people.mozilla.org/~mwargers/tests/userscalable-no.html
Comment 5•13 years ago
|
||
I agree that there should be a (site specific?) pref that would allow users to zoom in, even though the viewport meta tag of that page would disallow zooming. That being said, the stock browser is disallowing zooming with this meta tag and I guess Safari on the iPhone too. So we're the odd one in this case. Also, about:config is not a web page, it is basically part of the browser UI, so if the intention is to not allow zooming (which seems to be the case here with that meta tag), that should be honored. Also, in the case of web apps, it's probably important that this thing is working.
Updated•13 years ago
|
tracking-fennec: --- → 11+
Updated•13 years ago
|
blocking-fennec1.0: --- → ?
Assignee | ||
Comment 8•13 years ago
|
||
For the first release let's implement this, with a hidden pref to disable it. (Add-ons can provide a nicer UI or site-specific enabling and disabling.)
Assignee: nobody → mbrubeck
blocking-fennec1.0: ? → +
Comment 9•13 years ago
|
||
Matt, hope you don't mind, but I was in the neighborhood working on bug 702907
Assignee: mbrubeck → blassey.bugs
Attachment #604092 -
Flags: review?(mbrubeck)
Assignee | ||
Updated•13 years ago
|
Attachment #604092 -
Flags: review?(mbrubeck) → review+
Comment 10•13 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/89833e1b3016
Whiteboard: [inbound]
Comment 11•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/89833e1b3016
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → Firefox 13
Comment 12•13 years ago
|
||
This doesn't prevent double-tap-to-zoom. Also, changes in maple break this behaviour as implemented; it will probably need to be reverted at least temporarily. In the new viewport design (https://wiki.mozilla.org/Fennec/NativeUI/Viewport) the isZoomAllowed flag should be passed in to java via the compositor pathway for correct behaviour.
Comment 13•13 years ago
|
||
Backed out on maple with https://hg.mozilla.org/projects/maple/rev/70fe7536c40c. Also stealing this bug; I'll reimplement it once maple lands on m-c.
Assignee: blassey.bugs → bugmail.mozilla
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: maple
Comment 14•13 years ago
|
||
Just a braindump here in case somebody else picks up this bug before I get to it: the correct way to fix this would be pass in the "user-scalable: no" flag to Java via the first-paint compositor pathway described in the viewport doc at https://wiki.mozilla.org/Fennec/NativeUI/Viewport This means that whenever the compositor calls setFirstPaintViewport it should pass in this flag for the active document. There are other flags that we will probably also want to pass in at this point, for example the mWaitForTouchListeners flag in LayerController should also be done this way rather than listening for tab change events in java the way it is now. Ideally the implementation would therefore allow some sort of opaque data block to be passed along with the setFirstPaintViewport call and we could pull this out of there.
Updated•13 years ago
|
Whiteboard: maple → maple, [viewport]
Assignee | ||
Updated•13 years ago
|
Assignee: bugmail.mozilla → mbrubeck
Target Milestone: Firefox 13 → ---
Assignee | ||
Comment 15•13 years ago
|
||
(In reply to Kartikaya Gupta (:kats) from comment #14) > Ideally the implementation would therefore allow some sort of opaque data > block to be passed along with the setFirstPaintViewport call and we could > pull this out of there. Did you have any particular mechanism in mind for the "opaque data block"? A simple bit mask would work for this bug, but do you think we'll need something more flexible (JSON?) for other data?
Comment 16•13 years ago
|
||
(In reply to Matt Brubeck (:mbrubeck) from comment #15) > Did you have any particular mechanism in mind for the "opaque data block"? > A simple bit mask would work for this bug, but do you think we'll need > something more flexible (JSON?) for other data? So far there are three pieces of data I would like to put in this block. One is the user-scalable property, another is the mWaitForTouchListeners flag that's in the TouchEventHandler, and the third is the page background colour (see https://bugzilla.mozilla.org/show_bug.cgi?id=744439#c3). I suspect that eventually we'll have more of these things, so I think the flexible JSON (or similar) approach is best here.
Updated•13 years ago
|
Priority: P3 → P2
Assignee | ||
Comment 17•13 years ago
|
||
This ports clampZoom and getPageZoom code from XUL Fennec, with some simplifications. It also removes some unused zoom/displayport related prefs. Instead of loading viewZoomOverlay.js from toolkit to access the zoom.minPercent and zoom.maxPercent prefs (like XUL Fennec), this patch just uses the existing kViewportMinScale/MaxScale constants. The main effect of this patch is to set the default zoom correctly when a page loads that has zoom limits. It also affects the viewportH calculation for some pages that are wider than the viewport, like this one: http://people.mozilla.com/~mbrubeck/test/wide.html (Before this patch, Fennec would make the viewport extremely tall to allow you to zoom out to the full width. But the page disallows zooming, so with this patch we no longer do that.)
Attachment #604092 -
Attachment is obsolete: true
Attachment #624585 -
Flags: review?(mark.finkle)
Comment 18•13 years ago
|
||
Comment on attachment 624585 [details] [diff] [review] part 1: Make JavaScript viewport calculations respect zoom limits Looks OK and clampZoom seems like it shouldn't adversely affect the existing behavior. Is there any zoom clamping happening on the Java side we need to be aware of here?
Attachment #624585 -
Flags: review?(mark.finkle) → review+
Assignee | ||
Comment 19•13 years ago
|
||
(In reply to Mark Finkle (:mfinkle) from comment #18) > Looks OK and clampZoom seems like it shouldn't adversely affect the existing > behavior. Is there any zoom clamping happening on the Java side we need to > be aware of here? The Java code currently enforces a default minimum zoom of 0 and maximum zoom of 4. I reverted the part of this patch that increased the JS minimum zoom to 0.2; it should be fine to leave it at 0. In my next patches I will improve the interaction between the limits set by the page and the hard-coded limits in Java. Aside from reverting the kViewportMinScale change, this version just adds some whitespace fixes. Carrying r=mfinkle.
Attachment #624585 -
Attachment is obsolete: true
Attachment #624760 -
Flags: review+
Assignee | ||
Comment 20•13 years ago
|
||
This patch makes PanZoomController ignore double-tap and pinch gestures when zooming is disabled, and always keep the page at its default zoom level.
Attachment #624761 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 21•13 years ago
|
||
There are two remaining things on my to-do list for this bug: * Disable the double-tap listener on pages with user-scalable=no. This should improve tap responsiveness, and hopefully it will also fix a minor bug where link highlights appear and just stay there if you double-tap on a link user-scalable=no page. * Make PanZoomController obey minimum-scale and maximum-scale values from <meta name="viewport"> elements.
Status: REOPENED → ASSIGNED
Comment 22•13 years ago
|
||
Comment on attachment 624761 [details] [diff] [review] part 2: Disable zooming for pages with user-scalable=no Review of attachment 624761 [details] [diff] [review]: ----------------------------------------------------------------- Looks good overall but you should make sure that dynamic page size changes are handled properly. ::: mobile/android/base/GeckoApp.java @@ +1005,5 @@ > } else if (event.equals("Update:Restart")) { > doRestart("org.mozilla.gecko.restart_update"); > + } else if (event.equals("Tab:ViewportMetadata")) { > + int tabId = message.getInt("tabID"); > + Tab tab = Tabs.getInstance().getTab(tabId); Does tab need a null check here? ::: mobile/android/chrome/content/browser.js @@ +2366,5 @@ > + sendMessageToJava({ gecko: { > + type: "Tab:ViewportMetadata", > + allowZoom: aMetadata.allowZoom, > + defaultZoom: this.getDefaultZoom(), > + tabID: this.id You might need to send one of these when the page size changes too. If the page shrinks after loading, the defaultZoom might be too small, and result in overscroll being shown. Then again the bounce code in PanZoomController might take care of it too.
Attachment #624761 -
Flags: review?(bugmail.mozilla) → review+
Assignee | ||
Comment 23•13 years ago
|
||
(In reply to Kartikaya Gupta (:kats) from comment #22) > > + } else if (event.equals("Tab:ViewportMetadata")) { > > + int tabId = message.getInt("tabID"); > > + Tab tab = Tabs.getInstance().getTab(tabId); > > Does tab need a null check here? Yes, probably a good idea. Added. Several similar event handlers in this file do not check for null tabs; would it be worth a follow-up bug to add more null checks? > You might need to send one of these when the page size changes too. If the > page shrinks after loading, the defaultZoom might be too small, and result > in overscroll being shown. Then again the bounce code in PanZoomController > might take care of it too. Done. Carrying r=kats. This version just fixes these two review comments.
Attachment #624761 -
Attachment is obsolete: true
Attachment #624799 -
Flags: review+
Assignee | ||
Comment 24•13 years ago
|
||
Attachment #624814 -
Flags: review?(bugmail.mozilla)
Comment 25•13 years ago
|
||
(In reply to Matt Brubeck (:mbrubeck) from comment #23) > Several similar event handlers in this file do not check for null tabs; > would it be worth a follow-up bug to add more null checks? > If they're working fine so far then it's probably better to leave it as-is. Adding null checks might mask other breakage, and if we're not getting crashes because of NPEs there then changing it doesn't improve anything.
Updated•13 years ago
|
Attachment #624814 -
Attachment is patch: true
Assignee | ||
Comment 26•13 years ago
|
||
I didn't correctly address the dynamic size change issue. We need to send updated metadata on MozScrolledAreaChanged events, not just on resize and DOMMetaAdded events. This is what you were looking for, right Kats?
Attachment #624799 -
Attachment is obsolete: true
Attachment #624820 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 27•13 years ago
|
||
Comment on attachment 624820 [details] [diff] [review] part 2 (v3): Disable zooming for pages with user-scalable=no Hold off for a moment on this... I thought of some more clean-up that should simplify this a good deal.
Attachment #624820 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 28•13 years ago
|
||
Here is the promised simplification. When the page does not specify a default zoom factor, rather than calculate the minimum zoom in JS and pass it to Java, we can just use the minimum zoom factor that gets calculated on the Java side. Compared to the previous versions of this patch: * We no longer need a getDefaultZoom method in JS. * The "Tab:ViewportMetadata" message does not contain any values that are based on the page or screen size, so we don't need to recalculate and re-send the message on size changes. * The "defaultZoom" value may be NaN, in which case we clamp to minZoomFactor instead.
Attachment #624820 -
Attachment is obsolete: true
Attachment #624855 -
Flags: review?(bugmail.mozilla)
Comment 29•13 years ago
|
||
Comment on attachment 624855 [details] [diff] [review] part 2 (v4): Disable zooming for pages with user-scalable=no Review of attachment 624855 [details] [diff] [review]: ----------------------------------------------------------------- r+ with comments addressed. ::: mobile/android/base/ui/PanZoomController.java @@ +822,5 @@ > + minZoomFactor = maxZoomFactor = mController.getDefaultZoom(); > + if (mController.getDefaultZoom() > 0) > + minZoomFactor = maxZoomFactor = mController.getDefaultZoom(); > + else > + maxZoomFactor = minZoomFactor; This block seems... wrong. The "if" branch is identical to the line just before the if, and the "else" seems redundant. I don't think you need the if/else at all here, maybe it got left in from a previous version? ::: mobile/android/chrome/content/browser.js @@ +2446,5 @@ > + sendViewportMetadata: function sendViewportMetadata() { > + sendMessageToJava({ gecko: { > + type: "Tab:ViewportMetadata", > + allowZoom: this.metadata.allowZoom, > + defaultZoom: this.metadata.defaultZoom, As discussed on IRC, check to make sure that NaN values can be sent safely via JSON.
Attachment #624855 -
Flags: review?(bugmail.mozilla) → review+
Comment 30•13 years ago
|
||
Comment on attachment 624814 [details] [diff] [review] part 3: Disable double-tap on pages with user-scalable=no Review of attachment 624814 [details] [diff] [review]: ----------------------------------------------------------------- r+ with comments addressed. ::: mobile/android/base/gfx/LayerController.java @@ +350,5 @@ > } > > public void setAllowZoom(boolean aValue) { > mAllowZoom = aValue; > + mView.getTouchEventHandler().setDoubleTapEnabled(aValue); This call should be done on the UI thread. ::: mobile/android/base/gfx/TouchEventHandler.java @@ +215,5 @@ > } > mProcessingBalance--; > } > > + public void setDoubleTapEnabled(boolean aValue) { Add a comment that this function should be called on the UI thread.
Attachment #624814 -
Flags: review?(bugmail.mozilla) → review+
Assignee | ||
Comment 31•13 years ago
|
||
(In reply to Kartikaya Gupta (:kats) from comment #29) > > + minZoomFactor = maxZoomFactor = mController.getDefaultZoom(); > > + if (mController.getDefaultZoom() > 0) > > + minZoomFactor = maxZoomFactor = mController.getDefaultZoom(); > > + else > > + maxZoomFactor = minZoomFactor; > > This block seems... wrong. The "if" branch is identical to the line just > before the if, and the "else" seems redundant. I don't think you need the > if/else at all here, maybe it got left in from a previous version? Good catch. Actually it was the first line that was accidentally left over after some editing. The if statement stays, but I cleaned it up a bit. > As discussed on IRC, check to make sure that NaN values can be sent safely > via JSON. I changed the code to send 0 instead of NaN when the value is not set.
Attachment #624855 -
Attachment is obsolete: true
Attachment #624918 -
Flags: review+
Assignee | ||
Comment 32•13 years ago
|
||
(In reply to Kartikaya Gupta (:kats) from comment #30) > This call should be done on the UI thread. > Add a comment that this function should be called on the UI thread. Done and done. However, I found that this patch causes a problem with pinch and pan gestures on user-scalable=no pages. Currently investigating.
Assignee | ||
Updated•13 years ago
|
Attachment #624814 -
Attachment is obsolete: true
Assignee | ||
Comment 33•13 years ago
|
||
Attachment #624922 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 34•13 years ago
|
||
Forgot to qref.
Assignee | ||
Comment 35•13 years ago
|
||
I ended up having to change a little more than I thought previously; this changes getValidViewportMetrics to ensure that min/maxZoomFactor are set correctly on pages that don't specify a default zoom.
Attachment #624918 -
Attachment is obsolete: true
Attachment #624927 -
Attachment is obsolete: true
Attachment #624964 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 36•13 years ago
|
||
This fixes the bug I saw when pinching on pages with user-scalable=no. The problem had to do with a subtle interaction between the two gesture detectors (GestureDetector and SimpleScaleGestureDetector). We send events to the GestureDetector first, then if they are not consumed we send them to the SimpleScaleGestureDetector. Pinching sent DOWN events to both detectors, then an UP events to the GestureDetector, which if it's not waiting for a double-tap, could register a single-tap immediately and consume the event, which prevented the SimpleScaleGestureDetector from seeing it. This left the SimpleScaleGestureDetector thinking that an extra pointer was still on the screen, which caused any further swipes to be treated as pinches instead. The solution here is to make sure that UP and CANCEL events are passed to both detectors.
Attachment #624921 -
Attachment is obsolete: true
Attachment #624968 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Updated•13 years ago
|
Attachment #624968 -
Attachment is patch: true
Assignee | ||
Comment 37•13 years ago
|
||
Rebased on top of the earlier patches.
Attachment #624922 -
Attachment is obsolete: true
Attachment #624922 -
Flags: review?(bugmail.mozilla)
Attachment #624969 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 38•13 years ago
|
||
Note to self: I have one last TODO item, which is to add a hidden pref so that users (and add-ons) can enable zooming on all pages if they want to.
Assignee | ||
Comment 39•13 years ago
|
||
Forgot to qref again; sigh...
Attachment #624969 -
Attachment is obsolete: true
Attachment #624969 -
Flags: review?(bugmail.mozilla)
Attachment #625003 -
Flags: review?(bugmail.mozilla)
Comment 40•13 years ago
|
||
Comment on attachment 624964 [details] [diff] [review] part 2 (v6): Disable zooming for pages with user-scalable=no Review of attachment 624964 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/ui/PanZoomController.java @@ +820,5 @@ > + if (!mController.getAllowZoom()) { > + // If allowZoom is false, clamp to the default zoom level. > + if (mController.getDefaultZoom() > 0) > + minZoomFactor = mController.getDefaultZoom(); > + maxZoomFactor = minZoomFactor; Since getDefaultZoom() is always guaranteed to be >= 0, you technically don't need the inner if condition here and could just do minZoomFactor = maxZoomFactor = mController.getDefaultZoom(). I feel it might be a little clearer since it's not obvious why the > 0 check is there right now just from looking at the code. I leave it to you to decide.
Attachment #624964 -
Flags: review?(bugmail.mozilla) → review+
Comment 41•13 years ago
|
||
Comment on attachment 624968 [details] [diff] [review] part 3 (v3): Disable double-tap on pages with user-scalable=no Review of attachment 624968 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Matt Brubeck (:mbrubeck) from comment #36) > Pinching sent DOWN events to both detectors, then an UP events to the > GestureDetector, which if it's not waiting for a double-tap, could register > a single-tap immediately and consume the event, which prevented the > SimpleScaleGestureDetector from seeing it. Just to clarify, did this happen for all pinches, or just pinches where you move by small amounts? It seems odd to me that a pinch motion (down-move-up) would trigger as a single-tap, I would have thought that the "move" would cause the GestureDetector to cancel the tap detection. > This left the > SimpleScaleGestureDetector thinking that an extra pointer was still on the > screen, which caused any further swipes to be treated as pinches instead. We should probably harden the SimpleScaleGestureDetector against this case in general, by clearing the list of stored touch points before processing an ACTION_DOWN event. > The solution here is to make sure that UP and CANCEL events are passed to > both detectors. I'm not entirely happy with this solution, but I'm not opposed to it either. If you think hardening the SimpleScaleGestureDetector would work and is a better fix, feel free to do that instead. Otherwise just file a follow-up bug for the hardening and land this as-is.
Attachment #624968 -
Flags: review?(bugmail.mozilla) → review+
Comment 42•13 years ago
|
||
Comment on attachment 625003 [details] [diff] [review] part 4: Respect minimum- and maximum-scale values set by web pages Review of attachment 625003 [details] [diff] [review]: ----------------------------------------------------------------- r+ with comment addressed ::: mobile/android/base/ui/PanZoomController.java @@ +819,5 @@ > > + if (mController.getMinZoom() > 0) > + minZoomFactor = mController.getMinZoom(); > + if (mController.getMaxZoom() > 0) > + maxZoomFactor = mController.getMaxZoom(); I was looking at the browser.js code that calculates minScale and maxScale in getViewportMetadata, and it seems like maxScale could be less than minScale. That might mess things up, the browser.js code should probably modify the clamp to ensure that maxScale is always >= minScale.
Attachment #625003 -
Flags: review?(bugmail.mozilla) → review+
Assignee | ||
Comment 43•13 years ago
|
||
When this pref is set, all pages are treated as user scalable regardless of user-scalable=no or minimum/maximum-scale. Here's a restartless add-on that turns this pref on: http://limpet.net/mbrubeck/temp/alwayszoom.xpi
Attachment #625113 -
Flags: review?(mark.finkle)
Updated•13 years ago
|
Attachment #625113 -
Flags: review?(mark.finkle) → review+
Assignee | ||
Comment 44•13 years ago
|
||
(In reply to Kartikaya Gupta (:kats) from comment #40) > > + if (mController.getDefaultZoom() > 0) > > + minZoomFactor = mController.getDefaultZoom(); > > + maxZoomFactor = minZoomFactor; > > Since getDefaultZoom() is always guaranteed to be >= 0, you technically > don't need the inner if condition here and could just do minZoomFactor = > maxZoomFactor = mController.getDefaultZoom(). Oh, good point! Fixed. (The "if" statement was left over from when I thought that getDefaultZoom could return NaN.) (In reply to Kartikaya Gupta (:kats) from comment #41) > Just to clarify, did this happen for all pinches, or just pinches where you > move by small amounts? It seems odd to me that a pinch motion (down-move-up) > would trigger as a single-tap, I would have thought that the "move" would > cause the GestureDetector to cancel the tap detection. It happened in any pinch where *one* of the fingers moved by only a small amount. > We should probably harden the SimpleScaleGestureDetector against this case > in general, by clearing the list of stored touch points before processing an > ACTION_DOWN event. That sounds like a good idea, but I'm going to file a follow-up for it. Some of the other less-hacky things I tried ended up introducing other regressions; this was not the cleanest fix but it's the one I have the most confidence in. (My own idea was to pass events to the scale gesture detector first when appropriate, but I like your idea better.) (In reply to Kartikaya Gupta (:kats) from comment #42) > I was looking at the browser.js code that calculates minScale and maxScale > in getViewportMetadata, and it seems like maxScale could be less than > minScale. That might mess things up, the browser.js code should probably > modify the clamp to ensure that maxScale is always >= minScale. Fixed. https://hg.mozilla.org/integration/mozilla-inbound/rev/93318e2225d6 https://hg.mozilla.org/integration/mozilla-inbound/rev/593363418099 https://hg.mozilla.org/integration/mozilla-inbound/rev/ce227a3c263a https://hg.mozilla.org/integration/mozilla-inbound/rev/cc608831d13b https://hg.mozilla.org/integration/mozilla-inbound/rev/29d9df6f59e9
Assignee | ||
Comment 45•13 years ago
|
||
(This is a roll-up patch just to avoid setting flags on all five attachments. The patches can still land on Aurora as separate changesets.) [Approval Request Comment] User impact if declined: Fennec ignores zoom limits set by web pages. This is a web compatibility regression compared to XUL Fennec and other mobile browsers. Testing completed (on m-c, etc.): Landed on m-i on May 18. Has not had extensive testing yet. There is only minimal automated testing of this code in place. We should wait until this has baked in Nightly for several days before approving it for Aurora. Risk to taking this patch (and alternatives if risky): Mobile-only patch. This is a somewhat large patch and there is moderate risk of regression. It is supposed to change behavior only on pages that declare zoom restrictions, but if there are unexpected bugs then they might cause pages to be zoomed differently on initial load, or web pages' zoom limits might not be interpreted correctly, or tap gestures might not be handled correctly. Since this is a high-priority blocking feature for Fennec 1.0, I recommend testing this on Nightly, then landing it soon for more widespread testing on Aurora and Beta. If regressions are found, we can decide whether to fix them, or to back out the feaure for release. String or UUID changes made by this patch: None.
Attachment #625128 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 46•13 years ago
|
||
I filed follow-up bug 756474 to harden the scale detector (comment 41) and bug 756473 to add more automated tests for this code.
Assignee | ||
Updated•13 years ago
|
Attachment #625128 -
Attachment is obsolete: true
Attachment #625128 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 47•13 years ago
|
||
Backed out on inbound because it caused a large number of reftest failures. :( https://hg.mozilla.org/integration/mozilla-inbound/rev/6628bbca67f7 It looks like every reftest just rendered a completely white page: https://tbpl.mozilla.org/php/getParsedLog.php?id=11866149&tree=Mozilla-Inbound
Target Milestone: Firefox 15 → ---
Assignee | ||
Comment 48•13 years ago
|
||
The reftest failures are intermittent; some of the test runs with the patch are green while others are orange. I downloaded one of the builds with orange tests and did not see any visible problems.
Assignee | ||
Comment 49•13 years ago
|
||
The reftest failures are intermittent but frequent; 9 out of 13 completed test runs had failures. I don't see anything relevant in the reftest logs.
Assignee | ||
Comment 50•13 years ago
|
||
My only theory so far is that maybe some sort of timing or event order during page load has changed, and the reftest harness is now taking its snapshots before we have drawn the page content. Giving this bug back to kats for now because I'll be away next week; I'm happy to take it back when I return.
Assignee: mbrubeck → bugmail.mozilla
Assignee | ||
Comment 51•13 years ago
|
||
To see which patch is responsible, I pushed each intermediate changeset to Try. For example, Try run #3 has only parts 1-3 applied: 1. https://tbpl.mozilla.org/?tree=Try&rev=35074c4fade9 2. https://tbpl.mozilla.org/?tree=Try&rev=e5f22cdde4cc 3. https://tbpl.mozilla.org/?tree=Try&rev=3274dc02d783 4. https://tbpl.mozilla.org/?tree=Try&rev=c9363d0be87f 5. https://tbpl.mozilla.org/?tree=Try&rev=31e2b2366d46
Assignee | ||
Comment 52•13 years ago
|
||
The reftest failures were caused by part 1 (attachment 624760 [details] [diff] [review]). I haven't yet figured out why.
Comment 53•13 years ago
|
||
(In reply to Matt Brubeck (:mbrubeck) from comment #52) > The reftest failures were caused by part 1 (attachment 624760 [details] [diff] [review] > [diff] [review]). I haven't yet figured out why. Keep in mind that until Bug 748088 is fixed (which is hopefully very soon!), native reftests are actually testing a XUL-fennec-like multiprocess setup. So it could be (for example) that some of the code removed by this patch is unused under normal circumstances, but actually necessary during reftests.
Assignee | ||
Comment 54•13 years ago
|
||
(In reply to Ali Juma [:ajuma] from comment #53) > Keep in mind that until Bug 748088 is fixed (which is hopefully very soon!), > native reftests are actually testing a XUL-fennec-like multiprocess setup. > So it could be (for example) that some of the code removed by this patch is > unused under normal circumstances, but actually necessary during reftests. Thanks. I've pushed some patches to Try that restore the removed preferences and revert some of the other changes, to try to narrow it down: https://tbpl.mozilla.org/?tree=Try&rev=25bcc2f7edce https://tbpl.mozilla.org/?tree=Try&rev=6928b61546b3 https://tbpl.mozilla.org/?tree=Try&rev=a4605849dd82
Comment 55•13 years ago
|
||
Original landing and backout: https://hg.mozilla.org/mozilla-central/rev/93318e2225d6 https://hg.mozilla.org/mozilla-central/rev/593363418099 https://hg.mozilla.org/mozilla-central/rev/ce227a3c263a https://hg.mozilla.org/mozilla-central/rev/cc608831d13b https://hg.mozilla.org/mozilla-central/rev/29d9df6f59e9 https://hg.mozilla.org/mozilla-central/rev/6628bbca67f7
Assignee: bugmail.mozilla → mbrubeck
Flags: in-testsuite-
Target Milestone: --- → Firefox 15
Updated•13 years ago
|
Assignee: mbrubeck → bugmail.mozilla
status-firefox15:
--- → affected
Target Milestone: Firefox 15 → ---
Assignee | ||
Comment 56•13 years ago
|
||
Try run was green; re-landed on inbound without the pref removals: https://hg.mozilla.org/integration/mozilla-inbound/rev/18a75044ea04 https://hg.mozilla.org/integration/mozilla-inbound/rev/203bc6e6dc16 https://hg.mozilla.org/integration/mozilla-inbound/rev/92a73a5fd37a https://hg.mozilla.org/integration/mozilla-inbound/rev/0589d2df6cbc https://hg.mozilla.org/integration/mozilla-inbound/rev/9aa4f0b01c0b
Assignee | ||
Updated•13 years ago
|
Target Milestone: --- → Firefox 15
Comment 57•13 years ago
|
||
Assigning this back to you. I can keep an eye on it and request the uplift if you don't get around to it before you take off.
Assignee: bugmail.mozilla → mbrubeck
Assignee | ||
Comment 58•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/18a75044ea04 https://hg.mozilla.org/mozilla-central/rev/203bc6e6dc16 https://hg.mozilla.org/mozilla-central/rev/92a73a5fd37a https://hg.mozilla.org/mozilla-central/rev/0589d2df6cbc https://hg.mozilla.org/mozilla-central/rev/9aa4f0b01c0b
Status: ASSIGNED → RESOLVED
Closed: 13 years ago → 13 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 59•13 years ago
|
||
Creating a roll-up patch just to make approval easier. These can still land as separate patches. NOTE if someone else lands this on Aurora: Please land the versions that landed on m-c, because there were changes found in review that are not reflected in the attachments here. See comment 45 for approval request comments.
Attachment #625631 -
Flags: review+
Attachment #625631 -
Flags: approval-mozilla-aurora?
Comment 60•13 years ago
|
||
Comment on attachment 625631 [details] [diff] [review] roll-up patch for Aurora approval [Triage Comment] Mobile only and a 1.0 blocker, approving for Aurora 14.
Attachment #625631 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 61•13 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/68f63ff1743a https://hg.mozilla.org/releases/mozilla-aurora/rev/1ecdee8df588 https://hg.mozilla.org/releases/mozilla-aurora/rev/9f6b81c96aa5 https://hg.mozilla.org/releases/mozilla-aurora/rev/e08ab879997d https://hg.mozilla.org/releases/mozilla-aurora/rev/fa00ac254da3
Comment 62•12 years ago
|
||
Verified fixed on Nightly 15.0a1 (2012-05-23) Aurora 14.0a2 (2012-05-23) Beta 14.0b2 Samsung Galaxy SII (2.3.4)
Updated•4 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
•