707571 - user-scalable property of viewport meta tag is ignored

Status: VERIFIED FIXED

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

Description

[Steffen Wilberg]

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

Comment 1

[Matt Brubeck (:mbrubeck)]

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 2

[Brad Lassey [:blassey] (use needinfo?)]

Madhava and Mfinkle, make a call here

Comment 3

[Martijn Wargers (dead)]

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

[Martijn Wargers (dead)]

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.

Comment 8

[Matt Brubeck (:mbrubeck)]

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.)

Comment 9

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

Matt, hope you don't mind, but I was in the neighborhood working on bug 702907

Comment 10

[Brad Lassey [:blassey] (use needinfo?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/89833e1b3016

Comment 11

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/89833e1b3016

Comment 12

[Kartikaya Gupta (email:[e-mail])]

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

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 14

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 15

[Matt Brubeck (:mbrubeck)]

(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

[Kartikaya Gupta (email:[e-mail])]

(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.

Comment 17

[Matt Brubeck (:mbrubeck)]

Attachment: part 1: Make JavaScript viewport calculations respect zoom limits

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.)

Comment 18

[Mark Finkle (:mfinkle) (use needinfo?)]

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?

Comment 19

[Matt Brubeck (:mbrubeck)]

Attachment: part 1 (v2): Make JavaScript viewport calculations respect zoom limits

(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.

Comment 20

[Matt Brubeck (:mbrubeck)]

Attachment: part 2: Disable zooming for pages with user-scalable=no

This patch makes PanZoomController ignore double-tap and pinch gestures when zooming is disabled, and always keep the page at its default zoom level.

Comment 21

[Matt Brubeck (:mbrubeck)]

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.

Comment 22

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 23

[Matt Brubeck (:mbrubeck)]

Attachment: part 2 (v2): Disable zooming for pages with user-scalable=no

(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.

Comment 24

[Matt Brubeck (:mbrubeck)]

Attachment: part 3: Disable double-tap on pages with user-scalable=no

Comment 25

[Kartikaya Gupta (email:[e-mail])]

(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.

Comment 26

[Matt Brubeck (:mbrubeck)]

Attachment: part 2 (v3): Disable zooming for pages with user-scalable=no

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?

Comment 27

[Matt Brubeck (:mbrubeck)]

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.

Comment 28

[Matt Brubeck (:mbrubeck)]

Attachment: part 2 (v4): Disable zooming for pages with user-scalable=no

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.

Comment 29

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 30

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 31

[Matt Brubeck (:mbrubeck)]

Attachment: part 2 (v5): Disable zooming for pages with user-scalable=no

(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.

Comment 32

[Matt Brubeck (:mbrubeck)]

Attachment: part 3 (v2): Disable double-tap on pages with user-scalable=no

(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.

Comment 33

[Matt Brubeck (:mbrubeck)]

Attachment: part 4: Respect minimum- and maximum-scale values set by web pages

Comment 34

[Matt Brubeck (:mbrubeck)]

Attachment: part 2 (v5): Disable zooming for pages with user-scalable=no (WIP)

Forgot to qref.

Comment 35

[Matt Brubeck (:mbrubeck)]

Attachment: part 2 (v6): Disable zooming for pages with user-scalable=no

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.

Comment 36

[Matt Brubeck (:mbrubeck)]

Attachment: part 3 (v3): Disable double-tap on pages with user-scalable=no

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.

Comment 37

[Matt Brubeck (:mbrubeck)]

Attachment: part 4: Respect minimum- and maximum-scale values set by web pages

Rebased on top of the earlier patches.

Comment 38

[Matt Brubeck (:mbrubeck)]

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.

Comment 39

[Matt Brubeck (:mbrubeck)]

Attachment: part 4: Respect minimum- and maximum-scale values set by web pages

Forgot to qref again; sigh...

Comment 40

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 41

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 42

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 43

[Matt Brubeck (:mbrubeck)]

Attachment: part 5: Add a hidden pref to ignore user-scalable=no

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

Comment 44

[Matt Brubeck (:mbrubeck)]

(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

Comment 45

[Matt Brubeck (:mbrubeck)]

Attachment: roll-up patch for Aurora approval

(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.

Comment 46

[Matt Brubeck (:mbrubeck)]

I filed follow-up bug 756474 to harden the scale detector (comment 41) and bug 756473 to add more automated tests for this code.

Comment 47

[Matt Brubeck (:mbrubeck)]

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

Comment 48

[Matt Brubeck (:mbrubeck)]

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.

Comment 49

[Matt Brubeck (:mbrubeck)]

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.

Comment 50

[Matt Brubeck (:mbrubeck)]

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.

Comment 51

[Matt Brubeck (:mbrubeck)]

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

Comment 52

[Matt Brubeck (:mbrubeck)]

The reftest failures were caused by part 1 (attachment 624760 [details] [diff] [review]).  I haven't yet figured out why.

Comment 53

[Ali Juma [:ajuma]]

(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.

Comment 54

[Matt Brubeck (:mbrubeck)]

(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

[Ryan VanderMeulen [:RyanVM]]

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

Comment 56

[Matt Brubeck (:mbrubeck)]

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

Comment 57

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 58

[Matt Brubeck (:mbrubeck)]

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

Comment 59

[Matt Brubeck (:mbrubeck)]

Attachment: roll-up patch for Aurora approval

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.

Comment 60

[Alex Keybl [:akeybl]]

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.

Comment 61

[Mark Finkle (:mfinkle) (use needinfo?)]

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

[Catalin Suciu [:csuciu]]

Verified fixed on Nightly 15.0a1 (2012-05-23)
                  Aurora 14.0a2 (2012-05-23)
                  Beta 14.0b2
Samsung Galaxy SII (2.3.4)