749481 - BuildDisplayList should take into account resolution of document

Status: RESOLVED INVALID

(Firefox for Android Graveyard :: General, defect)

Description

[Wesley Johnston (:wesj)]

Split from Bug 741247. Touch events in Fennec need to propagate across different PresShell's that have different resolutions. We need to take that into account both when building display lists, and when doing hit testing.

Will move the patch from there over and keep working on it myself, but if someone from layout can grab and finish faster, would take this for Fennec 1.0 beta/release.

Comment 1

[Wesley Johnston (:wesj)]

Attachment: WIP Patch

Comment 2

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

SetResolution doesn't change appunits coordinates, right? So I'm not sure why we need to change anything in display lists.

Comment 3

[Wesley Johnston (:wesj)]

A tap at 200,200 in display pixels will be at a different point in the the two coordinate systems, throwing hit testing off.

Comment 4

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

It seems to me that the conversion between widget coordinates and layout coordinates is the part that needs to be changed here --- nsLayoutUtils::GetEventCoordinatesRelativeTo.

Or, better still, maybe you should be modifying the refPoint coordinates in the event?

Comment 5

[Wesley Johnston (:wesj)]

Hmm.. at the point where we are building this display list (i.e. the point where we are hopping across documents), we no longer have the Event. Just a dirtyRect. The WIP here basically adds a nsLayoutUtils::GetPOINTCoordinatesRelativeTo(aPoint, aFrame) (although with a worse name) that we can call at different places during display list construction. Maybe that's what you mean?

While building up the list, the transformed dirty rect is passed in to buildDisplayListForChild each time, so we have to be careful we don't transform it each step. I've been really hacky about that just to see if this would work.

During HitTesting we already have a flattened displayList and we only need to transform when we step into the first DisplayListItem in the scaled document.

Comment 6

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

I just don't understand the problem :-(.

What's in the frame tree and display list should already be correct and sufficient to correctly convert points. I don't understand why it is not. My only theory is that some extra layer transforms applied by the Android browser front end need to be accounted for, and that should happen in the Android browser front end.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

As far as I see nothing in hittesting handles PresShell "Resolution".
(On desktop we don't use that for Zoom.)
So the chrome shell has normal resolution but content shell has some other resolution.

Comment 8

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

As far as I can tell, the resolution of non-root presshells isn't even used.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Oh. Then how do we zoom in mobile?

Comment 10

[Wesley Johnston (:wesj)]

We seem to set the resolution of the parent window:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#1715

As far as I know, the xul:browser elements in browser.xul never change size though. They're always screenWidth (in display pixels) wide, and the content inside them is scaled. Hence, they clip their content? Thats part of this I guess I don't understand very well either. Digging.

Comment 11

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

Don't we set clipSubdocument to null on them, so the <browser> content isn't clipped?

Comment 12

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

Wes - We don't seem to be negatively affected by this, so I am not tracking anymore. If we really need this, let's discuss and get a plan.

Comment 13

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

I don't think we need to do anything here. Bug 901962 and friends fixed a bunch of the issues with hit testing outside the viewport so I'm going to close this as invalid. It wasn't going anywhere anyway.