Closed Bug 707571 Opened 8 years ago Closed 8 years ago

user-scalable property of viewport meta tag is ignored

Categories

(Firefox for Android :: General, defect, P2)

ARM
Android
defect

Tracking

()

VERIFIED FIXED
Firefox 15
Tracking Status
firefox14 --- verified
firefox15 --- verified
blocking-fennec1.0 --- +
fennec 14+ ---

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+
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
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.
Madhava and Mfinkle, make a call here
Priority: -- → P3
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
Duplicate of this bug: 711414
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.
tracking-fennec: --- → 11+
Duplicate of this bug: 719469
Duplicate of this bug: 729198
blocking-fennec1.0: --- → ?
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: ? → +
Attached patch patch (obsolete) — Splinter Review
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)
Attachment #604092 - Flags: review?(mbrubeck) → review+
https://hg.mozilla.org/mozilla-central/rev/89833e1b3016
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → Firefox 13
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.
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
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.
Whiteboard: maple → maple, [viewport]
Assignee: bugmail.mozilla → mbrubeck
Target Milestone: Firefox 13 → ---
(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?
(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.
Priority: P3 → P2
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 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+
(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+
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)
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 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+
(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+
Attachment #624814 - Flags: review?(bugmail.mozilla)
(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.
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)
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)
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 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 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+
(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+
(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.
Attachment #624814 - Attachment is obsolete: true
Attachment #624922 - Flags: review?(bugmail.mozilla)
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)
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)
Attachment #624968 - Attachment is patch: true
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)
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.
Forgot to qref again; sigh...
Attachment #624969 - Attachment is obsolete: true
Attachment #624969 - Flags: review?(bugmail.mozilla)
Attachment #625003 - Flags: review?(bugmail.mozilla)
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 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 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+
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)
Attachment #625113 - Flags: review?(mark.finkle) → review+
(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
tracking-fennec: 11+ → 14+
Target Milestone: --- → Firefox 15
Depends on: 756473
Blocks: 756474
(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?
I filed follow-up bug 756474 to harden the scale detector (comment 41) and bug 756473 to add more automated tests for this code.
Attachment #625128 - Attachment is obsolete: true
Attachment #625128 - Flags: approval-mozilla-aurora?
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 → ---
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.
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.
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
The reftest failures were caused by part 1 (attachment 624760 [details] [diff] [review]).  I haven't yet figured out why.
(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.
(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
Assignee: mbrubeck → bugmail.mozilla
Target Milestone: Firefox 15 → ---
Target Milestone: --- → Firefox 15
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
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 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+
Verified fixed on Nightly 15.0a1 (2012-05-23)
                  Aurora 14.0a2 (2012-05-23)
                  Beta 14.0b2
Samsung Galaxy SII (2.3.4)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.