763570 - click events in frameset documents offset

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Timothy Nikkel (:tnikkel)]

Zoom in on the left frame on https://forums.craigslist.org/?forumID=76&areaID=1 a little bit, pan down, try to click on a link, the actually linked clicked will be offset vertically by what seems to be the vertical pan amount.

Comment 1

[Wesley Johnston (:wesj)]

This seems to be related to zoom to me more than scroll position. If I don't zoom, things are "fine". Quick test page:

http://dl.dropbox.com/u/72157/framesets.htm

Comment 2

[Wesley Johnston (:wesj)]

Dang it. I don't think there's a quick fix for this.

I now think there must be a bug somewhere in the buildDisplayList code that causes this. We pick up a target to send these events to in browser.js on touchstart:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#2609

The target we get in this position often isn't the right one if the user has zoomed. In some cases its not even the right frame. I.e. if you open:

http://dl.dropbox.com/u/72157/framesets.htm

zoom in some, and then pan, you often end up panning the wrong thing. I don't think there's a nice easy bandaid patch for this (or even a non easy one) other than digging into the display list builder. I'll keep banging my head against it for a bit longer.

Comment 3

[Timothy Nikkel (:tnikkel)]

I think what might be happening is the native browser calls scrollTo(x,y) on the document where x,y are the top left of whats visible on screen. The call has no effect on a frameset document, it doesn't scroll, it has no scroll frame. If the document had a scroll frame we'd build a display list that took this scrolling into account. But we don't so that value is lost.

We could either bias the event coordinate by what the frontend codes thinks is the scroll value for framesets before passing it to hit testing. Or we could record the scrollTo value somewhere and move some frame or display list item by that amount when building a display list. They both sound gross.

Comment 4

[Wesley Johnston (:wesj)]

Attachment: Hacky Patch

This is a hacky patch to account for the fast the win.scrollX/Y are always zero in these documents, even when we're scrolled. I super awesome code to cache scroll position in our Tab object. That didn't seem very happy with some of our "find something close by" code, so I disabled it for now in order to put this up. This will also not fix touch events.

Comment 5

[Wesley Johnston (:wesj)]

Looking for feedback on... whether there is a better place to cache these values, and if you've got any genius ideas about better places to "adjust" our event coordinates/targeting.

Comment 6

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

Does this fix touch events delivered to content? In general I think window.top.scrollX will be wrong for content, and that might result in strange behaviour. I feel like fixing it in layout is better. I haven't looked at the relevant layout code but is it at all possible to generate a scroll frame for framesets?

Comment 7

[Wesley Johnston (:wesj)]

This will not fix touch events. The number of pages that will ever exist using both touch events and framesets is likely zero. But if there is one, this will not fix them. Its a bandaid for this next release.

tn is probably a better person to answer about whether we can generate a scrollframe here. I'm worried fixing window.top.scrollX will require deviating from the spec, but I don't see anything in the spec saying that frameset pages can't scroll, there was just no way to make them scroll until now.

An alternative fix could catch events in presShell hereish:

http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#2325

If the doc is a frameset, look up the fake scroll position... somewhere (Java is the only person who would have it at this point?) and adjust the coordinates.

Comment 9

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

Comment on attachment 632892 [details] [diff] [review]
Hacky Patch

Review of attachment 632892 [details] [diff] [review]:
-----------------------------------------------------------------

So I was thinking something more like this:

diff --git a/layout/base/nsCSSFrameConstructor.cpp b/layout/base/nsCSSFrameConstructor.cpp
index f62ca24..2ca961a 100644
--- a/layout/base/nsCSSFrameConstructor.cpp
+++ b/layout/base/nsCSSFrameConstructor.cpp
@@ -2671,11 +2671,13 @@ nsCSSFrameConstructor::SetUpDocElementContainingBlock(nsIContent* aDocElement)
   bool isScrollable = !isXUL;
 
   // Never create scrollbars for frameset documents.
+#if 0
   if (isHTML) {
     nsCOMPtr<nsIHTMLDocument> htmlDoc = do_QueryInterface(mDocument);
     if (htmlDoc && htmlDoc->GetIsFrameset())
       isScrollable = false;
   }
+#endif
 
   if (isPaginated) {
     isScrollable = presContext->HasPaginatedScrolling();


Applying the above along with a revert of bug 747493 seems to work pretty well. Build at https://people.mozilla.com/~kgupta/tmp/frameset.apk

Comment 10

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

(Obviously, the #if 0 should be replaced by #ifndef ANDROID or some such more appropriate define)

Comment 11

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

Attachment: (1/2) Revert of patch from bug 747439

Comment 12

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

Attachment: (2/2) Generate a scrollframe for framesets on fennec

This works great for me, fixes both bug 747439 and this one.

Comment 13

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

Also fixes bug 764639 as far as I can tell (I'm not able to reproduce it exactly, but the test page behaves normally with these patches).

Comment 14

[Timothy Nikkel (:tnikkel)]

Comment on attachment 633243 [details] [diff] [review]
(1/2) Revert of patch from bug 747439

r+ for backout.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 633244 [details] [diff] [review]
(2/2) Generate a scrollframe for framesets on fennec

You need to fix the comments at the beginning of the method a bit.

Is this the right ifdef?  Does b2g define MOZ_WIDGET_ANDROID?

Have you checked _why_ we don't create scrollframes for framesets, by any chance?  Might be worth knowing what this has the potential to break....

Comment 16

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

Attachment: (2/2) Generate a scrollframe for framesets on fennec (v2)

(In reply to Boris Zbarsky (:bz) from comment #15)
> You need to fix the comments at the beginning of the method a bit.

Done, attached updated patch.

> Is this the right ifdef?

I believe so. It's the one that seems to be used in all the OMTC code as well for fennec async pan/zoom stuff.

> Does b2g define MOZ_WIDGET_ANDROID?

No, b2g has MOZ_WIDGET_GONK.

> Have you checked _why_ we don't create scrollframes for framesets, by any
> chance?  Might be worth knowing what this has the potential to break....

Not really, no. From the code it looks like just a easy way to disable scrolling on framesets (and probably an optimization) but there might be some other underlying reason that I'm not aware of. I'm guessing roc would be the best person to answer that? File history points to him, anyway.

Comment 17

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

CC'ing roc for comment on ^

Comment 18

[Boris Zbarsky [:bzbarsky]]

> No, b2g has MOZ_WIDGET_GONK.

OK.  Does it not need this behavior, then?

> File history points to him, anyway.

It does?  File history afaict points to bug 138540, which doesn't seem to have much in the way of rationale, sadly.

Comment 19

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

(In reply to Boris Zbarsky (:bz) from comment #18)
> > No, b2g has MOZ_WIDGET_GONK.
> 
> OK.  Does it not need this behavior, then?

I don't know for sure, but I think it doesn't. I know that at the moment b2g doesn't use the android scrolling behaviour (bug 750974 is being worked on to make this eventually possible). Regardless I'm hesitant to change this for b2g as well without first knowing that there is a need for it, particularly as this patch may get uplifted to the 15 or even 14.x releases. If b2g needs this behaviour I'd rather do it in a separate patch.

> 
> > File history points to him, anyway.
> 
> It does?  File history afaict points to bug 138540, which doesn't seem to
> have much in the way of rationale, sadly.

Ah, I took some wrong turns in my file history navigation, I think. From the bugs I looked at (72747, 28670, and 243519) and the one you pointed out, it looks like maybe the original purpose of removing the scrollframe was to remove the visible scrollbars. On android this is moot because we don't draw scrollbars in gecko anyway.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 633501 [details] [diff] [review]
(2/2) Generate a scrollframe for framesets on fennec (v2)

Thanks for looking into that.  r=me

Comment 21

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

Landed on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d5397eb7d95
https://hg.mozilla.org/integration/mozilla-inbound/rev/47a56cdd7869

I also have a try run pushed to here:
https://tbpl.mozilla.org/?tree=Try&rev=1c86c866bfd5

It's colourful, but I'm fairly confident that all the failures are randoms and pre-existing.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/47a56cdd7869

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5d5397eb7d95

Comment 24

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

Comment on attachment 632892 [details] [diff] [review]
Hacky Patch

Obsoleting the hacky patch.

Comment 25

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

Comment on attachment 633243 [details] [diff] [review]
(1/2) Revert of patch from bug 747439

Requesting aurora/beta approval for both patches

[Approval Request Comment]
Bug caused by (feature/regressing bug #): partly by bug 747493, partly pre-existing
User impact if declined: frameset pages are pretty broken. they don't paint fully on load (need to pan/zoom to trigger paints), rotation makes things paint in strange places (bug 764639), touch events go to the wrong spot
Testing completed (on m-c, etc.): tested on m-c
Risk to taking this patch (and alternatives if risky): kinda risky; would like feedback from roc and/or other people who know layout intimately to minimize risk of fallout. the risk is mobile-only since the patch is basically an #ifdef android change
String or UUID changes made by this patch: none

Comment 27

[Timothy Nikkel (:tnikkel)]

A data point that could be used to determine riskyness would be to apply the same change to the desktop browser and see what are the nature of the failures (if any).

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 633501 [details] [diff] [review]
(2/2) Generate a scrollframe for framesets on fennec (v2)

Review of attachment 633501 [details] [diff] [review]:
-----------------------------------------------------------------

very nice

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Actually, you know what? We should just do this for all documents. It'll simplify things by reducing the differences between mobile and desktop, and because frameset documents will look more like regular documents. I don't see any significant benefit from keeping the current path.

We might want to change frameset { overflow: -moz-hidden-unscrollable } to "hidden" in html.css, as a related but separate change.

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Filed bug 766007 for that, with a patch.

Comment 31

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

Landed patches on aurora:

https://hg.mozilla.org/releases/mozilla-aurora/rev/6109374ecaf3
https://hg.mozilla.org/releases/mozilla-aurora/rev/ed0da1490444

Comment 32

[Johnathan Nightingale [:johnath]]

Comment on attachment 633243 [details] [diff] [review]
(1/2) Revert of patch from bug 747439

Both patches approved to land on beta tip, to track 14.0.1, but not the relbranch

Comment 33

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

Landed on default branch of mozilla-beta:

https://hg.mozilla.org/releases/mozilla-beta/rev/04661ed6772f
https://hg.mozilla.org/releases/mozilla-beta/rev/3343cb8a2d22

Comment 34

[Adrian Tamas (:AdrianT)]

Tap targets are correct for both the craiglist link and for the test page, also all the frames are correctly painted. Marking as verified.

Verified on:
Nightly 16.0a1 2012-07-11/Aurora 15.0a2 2012-07-11/Firefox Mobile 14 beta 12 build 1
HTC Desire
Android 2.2.2