Currently, reflow on zoom will zoom in and snap to an element on the page the user is looking at. This is probably too large of an area for the screen to adjust to. That is, the user may be looking at a specific line within that element, and it will get scrolled out of view simply because the top of the element (to which we snap) is too far away. Instead, we should identify a piece of text (similar to how we do text selection, perhaps), and snap in to that piece of text. I imagine the text should be only a few words (or perhaps a line at most), and we should try and center this on the screen for the user.

My intention here is to do this in two parts - 1. Add an API to nsIMarkupDocumentViewer (or perhaps nsIDocShell) that takes a point and a number of characters, and returns text centered around that point in the number of characters. For example, if we have the text: "abcdefghij klmn op*qrs tuvwx yz" Where the * is the point (x,y), then: nsIMarkupDocumentViewer::GetTextFromPoint(x, y, 10) should return: "mn opqrs t" The way I plan on doing this is to get the element in which the point lies, then from this element, get the frame. From here, if I transform the point to be frame-relative, I can use nsIFrame::GetContentOffsetsFromPoint() to get the offset (in characters, I believe) that corresponds to the character within the frame that is closest to the specified point. I can then offset this using the number of characters passed in to the function in order to get a range, and return the text in this range. From my experimentation, there appear to be three tricky parts to this - one is that we need to find the text frame associated with the point, not the parent content element. So, we'd need to navigate down the frame tree until we get to an nsTextFrame. Second, we need to make sure that if the point lies closer than floor(|n|/2) characters from the left edge of the frame, where |n| is the number of specified characters passed to the function, then our range will be (0,min(n,length_of_content(frame)) instead. Similarly, we'll need to check the end boundary. Finally, we'll need to do something sane if there isn't a text frame within the hierarchy the user specified. Ideally, this would simply disable reflow-on-zoom entirely, so we might be able to get around using heuristics specified in bug 808173. 2. Once we have the text block that the user zoomed in to (with some amount of certainty that it's unique... we'll probably have to test a variety of number of character values so that we can get one that works well), I intend to use a modified version of the FindHelper.doFind() code in browser.js to navigate to this text, after a reflow-on-zoom happens. The difference being, that it won't highlight the text, so the find code will have to be slightly modified. I'm not sure this is the most efficient method to do this, but it does seem like it's reasonably fast (testing the find code on my local build seems to be very quick). dbaron, do you think this is a reasonable approach, or is there another method you think might be a bit better suited to solve the problem?

Offhand, it's not clear to me why you need to "extend" from the point (where the user tapped to trigger the zoom, I assume) to a surrounding range of text. Can't you just get the character offset corresponding to the tap gesture, map that offset back to a position after the reflow, and then ensure this is suitably centered?

(In reply to Jonathan Kew (:jfkthame) from comment #2) > Offhand, it's not clear to me why you need to "extend" from the point (where > the user tapped to trigger the zoom, I assume) to a surrounding range of > text. Can't you just get the character offset corresponding to the tap > gesture, map that offset back to a position after the reflow, and then > ensure this is suitably centered? When we trigger reflow-on-zoom, we have a point that the user zoomed in to. We can get a character offset within the element that the user zoomed into, but "mapping it back to a position after the reflow" is the hard part, I think. I am open to suggestions, though... do you think there is an easier way to do this mapping after the reflow is finished?

Don't know where this came from...

(In reply to Scott Johnson (:jwir3) from comment #3) > When we trigger reflow-on-zoom, we have a point that the user zoomed in to. > We can get a character offset within the element that the user zoomed into, > but "mapping it back to a position after the reflow" is the hard part, I > think. > > I am open to suggestions, though... do you think there is an easier way to > do this mapping after the reflow is finished? Well... I don't have actual code at my fingertips, but it's essentially the same calculation as we'd use in order to display a caret there, isn't it? So layout and/or editor must have code that does it. Or it's similar to what HyperTextAccessible::GetBoundsForString does (that method expects a non-empty range and returns a rect, but the idea would be the same).

Jonathan, you made me think of something different just now... Instead of doing all of this craziness that goes from Javascript into the platform code by triggering the max line box API right now in nsIMarkupDocumentViewer like so: void nsIMarkupDocumentViewer::ChangeMaxLineBoxWidth(int32_t maxLineBoxWidth) we could pass in our point around which we want the text centered, and have two API functions like so: [The original, unchanged] void nsIMarkupDocumentViewer::ChangeMaxLineBoxWidth(int32_t maxLineBoxWidth) [A new one] nsPoint nsIMarkupDocumentViewer::ChangeMaxLineBoxWidthAboutPoint(int32_t maxLineBoxWidth, const nsPoint& center); Within this new function, we pass in the point about which we want to center, and then, after the reflow is finished, we calculate and return the new point, as you mentioned. The hard part is still determining this mapping, but now we have it all in one package. It would be easier to deal with in Javascript, I think, because we now would only have to make one call to the platform, instead of calling into the platform to adjust the line box width, waiting for the reflow, then calling back into Geckto to map the point back...

This seems to work better now for maintaining position after reflow-on-zoom. It's still not perfect, but it's much more reliable. There are two issues I think we should look into: 1) We should think about showing about 2 lines or so above where the user clicked (maybe play with this a bit to find an ideal setting), because where the user pinches to zoom in might not actually be where they were looking, and it might be frustrating, since pinch-zooming isn't super-precise in terms of a gesture. 2) There are invalidation issues that may be coming from the time it takes to actually perform the calculations necessary to maintain the scroll position (I'm thinking specifically about the GetFramesForPoint() call in CaretPositionFromPoint). I'm worried that with a complex zoom operation with many different OnPinch() calls might bog down the system such that invalidation isn't happening fast enough. On the other hand, it could be something simple that I'm doing incorrectly, but what I'm seeing are rectangles of white covering the whole page. Sometimes this can be extremely frustrating, since I can't tell what's being displayed on the page where I'm zooming. I'm not asking for review just yet, because there are still a few (hopefully minor) tweaks I want to make to it before I post a build, and then post the patch for review. I'm also hoping you guys might be able to help me with the invalidation bit before tomorrow evening.

Comment on attachment 702576 [details] [diff] [review] b803719 (v2) (WIP) >diff --git a/docshell/base/nsIMarkupDocumentViewer.idl b/docshell/base/nsIMarkupDocumentViewer.idl >+interface nsIDOMElement; Looks like you don't need this (anymore?). >+ >+ /** >+ * Retrieve a DOMRange (container + offset) for a point within an >+ * element. >+ * >+ * @param aX The horizontal coordinate of the point to convert to a >+ * DOMRange, in CSS pixels. >+ * @param aY The vertical coordinate of the point to convert to a >+ * DOMRange, in CSS pixels. >+ */ >+ nsIDOMRange getDOMRangeForPoint(in float aX, >+ in float aY); Probably worth mentioning that the resulting range will always be collapsed (i.e., empty), assuming that's true. (I'd expect that to be the case; if it's not, I'd like to know why.) It's also probably worth saying how this API differs from the document.caretPositionFromPoint API. Is the difference that it probes inside iframes, and/or that it handles textareas and text inputs differently (returning a range in the anonymous content rather than the CaretPosition object)? It would be easier to review the rest with a better idea of what those differences are. (I remember talking about it with you, but I don't remember details, or if we really discussed them.)

(In reply to David Baron [:dbaron] from comment #9) > Comment on attachment 702576 [details] [diff] [review] > b803719 (v2) (WIP) > > >diff --git a/docshell/base/nsIMarkupDocumentViewer.idl b/docshell/base/nsIMarkupDocumentViewer.idl > > >+interface nsIDOMElement; > > Looks like you don't need this (anymore?). > > >+ > >+ /** > >+ * Retrieve a DOMRange (container + offset) for a point within an > >+ * element. > >+ * > >+ * @param aX The horizontal coordinate of the point to convert to a > >+ * DOMRange, in CSS pixels. > >+ * @param aY The vertical coordinate of the point to convert to a > >+ * DOMRange, in CSS pixels. > >+ */ > >+ nsIDOMRange getDOMRangeForPoint(in float aX, > >+ in float aY); > > Probably worth mentioning that the resulting range will always be collapsed > (i.e., empty), assuming that's true. Yes, that is true. I can add that. > It's also probably worth saying how this API differs from the > document.caretPositionFromPoint API. Is the difference that it probes > inside iframes, and/or that it handles textareas and text inputs differently > (returning a range in the anonymous content rather than the CaretPosition > object)? No, the only difference is that a DOMRange has a getBoundingClientRect method, and CaretPosition does not. It seemed like an easier way to get the bounding client rect was to convert to a DOMRange, then call getBoundingClientRect on that range, rather than implement a new getBoundingClientRect on CaretPosition. I've noticed there are still some cases where this doesn't work. I'm trying to verify my assumptions this morning, so that I can give a clear picture (to myself, and others, as I'll probably need to ask for some assistance today) in order to see if this is a workable solution. One final thing I noticed is that calling the getDOMRangeFromPoint() method within onPinch() is way too slow. It really hangs things up when pinch-zooming, which is completely intolerable. My next thinking is to construct the domRange within the setViewport() method in Browser.js. setViewport() is called less frequently than onPinch(), but I'm not 100% sure this will work, either, since it is called frequently during a pinch-zoom gesture. I can't call it in onPinchFinish(), because this apparently (based on debug statements) takes place after the setViewport() call, which won't enable me to get the domRange from the viewport.

I have two test cases that kats and I have been using for this bug. One is here: http://people.mozilla.org/~sjohnson/junkyard/desperationSamba.html which has a song lyric, with each line in a different <p> element. This is useful if we want to use domRange.startContainer.wholeText, without it printing a jillion lines of text. The other one is http://people.mozilla.org/~sjohnson/junkyard/lemis.html This is the first couple of chapters of Le Miserables, in simple text underneath nothing more than a <body> element. This is useful to determine if the reflow-on-zoom snapping is happening to the correct location in a body of text. I feel like we're really close on this, but, it's still positioning itself incorrectly after the reflow happens.

kats, As you suspected, I don't think it's a java problem. The reason being that if I substitute the Browser:ZoomToRect code for a simple win.scrollTo(x,y), it doesn't get the correct scroll location, even if the range is non-null. I'm also getting a lot of range is null messages when I zoom in using the following patch. I'm going to try and see if I can debug why the range is returning null.

Comment on attachment 702576 [details] [diff] [review] b803719 (v2) (WIP) So I think it's good to document that in the code comments (but see both comments below, which I think might change that). That said, there are a few other issues here: * the CallChildren stuff is descending into children without adjusting the coordinate space for those children. And I'm actually somewhat skeptical that you even want CallChildren at all; I think if you want to descend into children if the point is inside an iframe, you should do that *after* the first call determines that the point is inside an iframe (rather than checking every single child... and possibly in a way that doesn't handle clipping correctly). Also, the fact that you're using CallChildren now means that your handling of iframes *is* different from document.caretPositionFromPoint. * The way you're constructing a range from an nsDOMCaretPosition doesn't really account for the fact that nsDOMCaretPosition is strange for text inputs and textareas, so it'll produce bogus results for them. Though you could manually undo that work in this code, it's probably preferable to refactor the nsIDocument::CaretPositionFromPoint code a bit so that you can call a version of it that has the guts of it but doesn't have the special handling of anonymous subtrees (though you might actually still want the check for textarea/input, so you can avoid returning native-anonymous content other than what's in an input or textarea). If you do that you'd presumably be changing this code into something that constructs a range from an nsFrame::ContentOffsets rather than from an nsDOMCaretPosition.

The patch attached in comment 15 is my current working draft of the patch. As a quick synopsis of the current state of this bug: * The position maintenance code seems to be working quite well now, with some minor issues at times during zoom out (I think it might not be getting the correct caret position in some cases, and we might need to cache a 'good enough' center point. This probably won't affect things too much, since once the user zooms out, they can read most of the text anyway, so as long as we get a position that isn't too far off from where they were, it should be ok. * There is a problem that seems to do with delayed repainting after a zoom-out operation. Zooming in is smooth, and works as expected, but zooming out can (depending on how much content is in the page) be somewhat slow. What seems to happen, especially with a test case I created, http://people.mozilla.com/~sjohnson/junkyard/lemis.html, is that the zoom-out operation displays the data that was already painted (i.e. the line boxes with constrained width, within the region the user WAS looking at) for several seconds before repainting with the new line box width (presumably after having reflowed to the new line boxes. An image of what I'm talking about is attached, since it's hard to describe in words. I think it could be a painting problem, because it seems to me that if it were a reflow problem, we'd be seeing the same delay when zooming in (although I'm not entirely confident this is a valid assumption). Also, if I move my finger to pan around the content during these hangs, it will often repaint more quickly than if I didn't pan at all, leading me to believe that it's an issue of not correctly painting a rect that was dirty. I added some debugging statements in to PresShell::DoReflow (to count the number of reflows, in the event that we're reflowing too often), and these appear to be normal. Additionally, I added debug statements in PresShell::Paint(), and it seems to invalidate the entire screen rect as expected. One thing I did notice is that the delay between these two debug statements does seem to correspond to the hang time, so it is still possible that it's a reflow issue (I don't know how much I can trust this, though, given that I don't know how much text output logcat buffers and if there is any delay there that I'm not accounting for). Chris, I was wondering if you could take a look at this, since, after speaking with blassey, he thinks it might be an issue with retained tiles.

This doesn't include fixes for the performance issues, which I will post in another bug, but it does fix the position-maintenance issues with reflow-on-zoom, and makes reflow-on-zoom much more usable.

Comment on attachment 707839 [details] [diff] [review] b803719 Review of attachment 707839 [details] [diff] [review]: ----------------------------------------------------------------- Some drive-by comments for the browser.js changes: ::: mobile/android/chrome/content/browser.js @@ +3205,5 @@ > this.setScrollClampingSize(aViewport.zoom); > > // Adjust the max line box width to be no more than the viewport width, but > // only if the reflow-on-zoom preference is enabled. > + let isZooming = aViewport.zoom != this.getViewport().zoom; I'd prefer if you used this._zoom instead of this.getViewport().zoom, since you only need the one thing from it. @@ +4305,5 @@ > + let data = JSON.parse(aData); > + let scrollTop = BrowserApp.selectedBrowser.contentDocument.documentElement.scrollTop || > + BrowserApp.selectedBrowser.contentDocument.body.scrollTop; > + let scrollLeft = BrowserApp.selectedBrowser.contentDocument.documentElement.scrollLeft || > + BrowserApp.selectedBrowser.contentDocument.body.scrollLeft; These variables are never used. @@ +4319,5 @@ > + BrowserApp.selectedTab._mReflozPoint.y = zoomPointY; > + } > + > + if (BrowserApp.selectedTab._mReflozPoint && > + !BrowserApp.selectedTab._mReflozPoint.range) { _mReflozPoint should always be non-null here, right? It was just created a few lines up. @@ +4327,5 @@ > + BrowserApp.selectedTab._mReflozPoint.range = range; > + } > + } > + } > + }, One or more of these braces is mis-indented.

kats, thanks for the comments. I addressed all of your review comments by making changes to the patch. (I'm not posting a new version of the patch, because I have a feeling dbaron is in the middle of reviewing it, and the comments were somewhat minor, so I didn't think posting a new version with those changes was warranted).

Comment on attachment 707839 [details] [diff] [review] b803719 dbaron and I discussed a few changes to this that I would like to add before posting for review again. Specifically, it would probably be better to wait some amount of time after pinching to see if the user is finished with their gestures (probably controlled by a pref). Also, we are going to discuss the possibility of adding a getClientRect() method to CaretPosition, instead of the current ToDOMRange() method, which doesn't exactly work as expected on input and textarea elements (due to anonymous content).

Another patch, this time with a better API (mozGetClientRect() vs. ToDOMRange()). It is visible to Chrome JS only (I've verified this).

I've noticed using pinch-to-zoom in Nightly that the Java-side ImmutableViewportMetrics are often(/always?) not updated with respect to page size, making parts of the page inaccessible - I mostly notice this when zooming in. Is that a known issue?

(In reply to Chris Lord [:cwiiis] from comment #23) > I've noticed using pinch-to-zoom in Nightly that the Java-side > ImmutableViewportMetrics are often(/always?) not updated with respect to > page size, making parts of the page inaccessible - I mostly notice this when > zooming in. Is that a known issue? No, I didn't know about this until just now. Would this be the cause of the page not being updated after a zoom out? BTW - It seems like the name "ImmutableViewportMetrics" is a strange name for something that can be mutated based on zoom. ;)

(In reply to Scott Johnson (:jwir3) from comment #24) > (In reply to Chris Lord [:cwiiis] from comment #23) > > I've noticed using pinch-to-zoom in Nightly that the Java-side > > ImmutableViewportMetrics are often(/always?) not updated with respect to > > page size, making parts of the page inaccessible - I mostly notice this when > > zooming in. Is that a known issue? > > No, I didn't know about this until just now. Would this be the cause of the > page not being updated after a zoom out? It could well cause this, yes - Perhaps MozScrolledAreaChanged isn't being fired after the reflow? (this gets handled in browser.js) > BTW - It seems like the name "ImmutableViewportMetrics" is a strange name > for something that can be mutated based on zoom. ;) Hah :) Perhaps ConstViewportMetrics would make more sense in that case, but the immutability refers to the member variables, rather than the interpretation of them. I think it'd make most sense to just rename it to ViewportMetrics now and add a comment about it being intentionally immutable, the prefix made more sense when there was a mutable version of it.

Comment on attachment 715606 [details] [diff] [review] b803719 (v2) Review of attachment 715606 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/base/src/nsDOMCaretPosition.cpp @@ +32,5 @@ > + // handle elements with anonymous content. What we do in this case > + // is a bit of a hack - we simply return the bounding box of the > + // element that contains anonymous content, so it's at least a > + // halfway sensible return value, anyway. > + nsCOMPtr<nsIDOMClientRect> rect; nsRefPtr<nsClientRect> rect @@ +36,5 @@ > + nsCOMPtr<nsIDOMClientRect> rect; > + > + if (mAnonymousContentNode) { > + nsCOMPtr<nsIDOMElement> element = do_QueryInterface(mOffsetNode); > + element->GetBoundingClientRect(getter_AddRefs(rect)); It's not at all clear to me why this would always be an element. Please check mOffsetNode->IsElement(), and make this rect = mOffsetNode->AsElement()->GetBoundingClientRect(); @@ +42,5 @@ > + nsRefPtr<nsRange> domRange; > + nsCOMPtr<nsIDOMNode> node = do_QueryInterface(mOffsetNode); > + nsresult creationRv = nsRange::CreateRange(node, mOffset, node, > + mOffset, > + getter_AddRefs<nsRange>(domRange)); Shouldn't need the <nsRange> @@ +49,5 @@ > + } > + > + NS_ASSERTION(domRange, "unable to retrieve valid dom range from CaretPosition"); > + > + domRange->GetBoundingClientRect(getter_AddRefs(rect)); And make this rect = domRange->GetBoundingClientRect(); ::: content/base/src/nsDOMCaretPosition.h @@ +12,3 @@ > #include "nsWrapperCache.h" > +#include "nsRect.h" > +#include "nsClientRect.h" No need to include any of those; just forward declare nsClientRect. @@ +55,5 @@ > + * @returns An nsIDOMClientRect representing the bounding rectangle of this > + * CaretPosition, if one can be successfully determined, otherwise > + * nullptr. > + */ > + already_AddRefed<nsIDOMClientRect> MozGetClientRect() const; This should return already_AddRefed<nsClientRect>, and probably should not be prefixed. ::: content/base/src/nsDocument.cpp @@ +9135,5 @@ > > nsRefPtr<nsDOMCaretPosition> aCaretPos = new nsDOMCaretPosition(node, offset); > + if (anonNode) { > + nsCOMPtr<nsINode> qiNode = do_QueryInterface(anonNode); > + aCaretPos->SetAnonymousContentNode(qiNode); nsIContent is a subclass of nsINode. Remove the QI.

Thanks for the review, Ms2ger. I've incorporated changes into the patch as you suggested. (In reply to :Ms2ger from comment #27) > This should return already_AddRefed<nsClientRect>, and probably should not > be prefixed. We decided to prefix this method and make it chrome-only, as it's an extension to the spec that we've added for our convenience. At some point, we hope to get it added to the spec, at which time we'll probably make it unprefixed.

kats, if you could look over this for general correctness when you get a chance, I'd appreciate it. I'll forward it to dbaron to review the layout changes, as well.

Henri, do you have an opinion on prefixing this?

It (CaretPosition.getClientRect()) shouldn't be prefixed; that was going to be one of my review comments. I think we should make an effort to standardize it, but I think the efforts so far didn't yield a response from any other implementors: http://lists.w3.org/Archives/Public/www-style/2013Feb/thread.html#msg323 I'm inclined to agree it should be [ChromeOnly] (but not prefixed) for now.

Though actually I'm considering that maybe it should just be exposed to content. I think it's relatively unlikely that there are multiple meanings we'd want for such an API.

Comment on attachment 727219 [details] [diff] [review] b803719 (v3) Review of attachment 727219 [details] [diff] [review]: ----------------------------------------------------------------- I'm only looking at the browser.js changes here since I don't think I'm qualified to review the rest of it. ::: mobile/android/chrome/content/browser.js @@ +3379,1 @@ > BrowserEventHandler._mLastPinchData = null; This line should be removed since _mLastPinchData isn't being used any more. Also take out the "_mLastPinchData: null" initializer, and grep the file for any other instances that I might have missed. @@ +4483,1 @@ > let viewport = BrowserApp.selectedTab.getViewport(); Move this line down to after the !aRange block. @@ +4497,5 @@ > + var scrollTop = BrowserApp.selectedBrowser.contentDocument.documentElement.scrollTop || BrowserApp.selectedBrowser.contentDocument.body.scrollTop; > + > + // We subtract half the height of the viewport so that we can (ideally) > + // center the area of interest on the screen. > + var topPos = scrollTop + drRect.top - (viewport.cssHeight / 2.0); If I'm understanding this correctly, it will take the top of a wrapped column of text (that the user is interested in) and center it on the screen. If so, I think it makes more sense to align it to the top of the screen, specially if the text is taller than the screen. If it's not taller than the screen, then maybe align it so that the center of the column is in the center of the screen. But that's just what I would expect based on thinking about it - your version might work better in practice. @@ +4527,5 @@ > + > + if (!BrowserApp.selectedTab._mReflozPoint.range) { > + let range = BrowserApp.selectedBrowser.contentDocument.caretPositionFromPoint(zoomPointX, zoomPointY); > + if (range) { > + BrowserApp.selectedTab._mReflozPoint.range = range; This part doesn't make sense to me. It looks like you're only setting the range object once for each pinch, and on the first onPinch call for that user pinch action. However, you're updating the x and y values on each onPinch call. Won't these get out of sync then? I think a better way would be to not bother doing anything with the range here, and just calculate the range once in onPinchFinish, just before it's needed. Don't even need to store it in _mReflozPoint, which will simplify this entire onPinch function to "BrowserApp.selectedTab._mReflozPoint = JSON.parse(aData)" (still inside the mReflozPref check).

(In reply to :Ms2ger from comment #30) > Henri, do you have an opinion on prefixing this? It looks like a rather straight-forward and unbikesheddable addition to CSSOM View. I think we shouldn't prefix.

(In reply to Henri Sivonen (:hsivonen) from comment #34) > (In reply to :Ms2ger from comment #30) > > Henri, do you have an opinion on prefixing this? > > It looks like a rather straight-forward and unbikesheddable addition to > CSSOM View. I think we shouldn't prefix. I'll make this change, then, and unprefix.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #33) > This line should be removed since _mLastPinchData isn't being used any more. > Also take out the "_mLastPinchData: null" initializer, and grep the file for > any other instances that I might have missed. Fixed. > Move this line down to after the !aRange block. Fixed. > If I'm understanding this correctly, it will take the top of a wrapped > column of text (that the user is interested in) and center it on the screen. > If so, I think it makes more sense to align it to the top of the screen, > specially if the text is taller than the screen. If it's not taller than the > screen, then maybe align it so that the center of the column is in the > center of the screen. But that's just what I would expect based on thinking > about it - your version might work better in practice. You are understanding it correctly. I've found this method tends to work better in practice, because when a user pinch-zooms in to text, it seems they don't always pinch in to the text they are reading. Instead, I've found they pinch in just a bit below the text they are currently reading. If I move the viewport so that it's at the top of the CaretPosition they zoomed in on, this often isn't actually the point they were reading. Instead, it end up scrolling the text they were looking at off of the screen to the top. I've found that, in the testing I've done personally, as well as the builds I've given to friends and family, that this works better. It's not perfect, because the point they pinch to zoom in on is not always the point where they want to read - it sometimes can be quite a distance off. So, it's not always going to center the text they want in the center of the screen, but it usually makes it at least possible that the last words they read are on the screen so they can continue reading. I think this is something that we should re-investigate after we get it so that reflow on zoom happens only on double-tap zoom. I think double-tap zoom might be more accurate than pinch-zoom, and this might be better reworked after this. > This part doesn't make sense to me. It looks like you're only setting the > range object once for each pinch, and on the first onPinch call for that > user pinch action. However, you're updating the x and y values on each > onPinch call. Won't these get out of sync then? I think a better way would > be to not bother doing anything with the range here, and just calculate the > range once in onPinchFinish, just before it's needed. Don't even need to > store it in _mReflozPoint, which will simplify this entire onPinch function > to "BrowserApp.selectedTab._mReflozPoint = JSON.parse(aData)" (still inside > the mReflozPref check). So, the reason I did this is because the onPinch() call never changes the point. It's always the same from one onPinch call to the next, until a new pinch gesture happens. Once the pinch gesture is finished, it calls onPinchFinish(). I use mReflozPoint.range as a sort of switch that we're in the midst of a pinch gesture. We utilize the first point that we get from onPinch (for a given pinch gesture), calculate the range once (since calculating the range is somewhat expensive) for this point, and then maintain the invariant that if mReflozPoint.range is set, we're in the midst of a pinch gesture, reflow on zoom is enabled, and we haven't yet snapped to the location we will eventually want (i.e. onPinchFinish hasn't yet been called). The reason I do it in onPinch instead of onPinchFinish is because the sequence of events is like the following: - User Starts pinching gesture - onPinch() called 0 or more times (usually ~ 40, but with all the same points) - setViewport() called when pinch gesture is finished - onPinchFinish() called. Because setViewport() is called BEFORE onPinchFinish(), we can't calculate the range within onPinchFinish() (because we're in a different viewport now), so it needs to be calculated in onPinch(). You're right, though, that if the point changes, we're going to be out of sync. This doesn't happen, however - the same point is always passed in to onPinch for a single pinch gesture (the center point of the user's fingers as they pinch). So, while I update the point, it's probably not necessary, and I can change that if you'd prefer. Again, this is likely to be something that will be moot once we switch to double-tap pinch-to-reflow-text.

(In reply to Scott Johnson (:jwir3) from comment #36) > You are understanding it correctly. I've found this method tends to work > better in practice, because when a user pinch-zooms in to text, it seems > they don't always pinch in to the text they are reading. Instead, I've found > they pinch in just a bit below the text they are currently reading. If I > move the viewport so that it's at the top of the CaretPosition they zoomed > in on, this often isn't actually the point they were reading. Instead, it > end up scrolling the text they were looking at off of the screen to the top. > I've found that, in the testing I've done personally, as well as the builds > I've given to friends and family, that this works better. > > It's not perfect, because the point they pinch to zoom in on is not always > the point where they want to read - it sometimes can be quite a distance > off. So, it's not always going to center the text they want in the center of > the screen, but it usually makes it at least possible that the last words > they read are on the screen so they can continue reading. > > I think this is something that we should re-investigate after we get it so > that reflow on zoom happens only on double-tap zoom. I think double-tap zoom > might be more accurate than pinch-zoom, and this might be better reworked > after this. Ok, that sounds good. I mostly just wanted to check my understanding was correct and there was some reason you did it this way. > The reason I do it in onPinch instead of onPinchFinish is because the > sequence of events is like the following: > - User Starts pinching gesture > - onPinch() called 0 or more times (usually ~ 40, but with all the same > points) > - setViewport() called when pinch gesture is finished > - onPinchFinish() called. > > Because setViewport() is called BEFORE onPinchFinish(), we can't calculate > the range within onPinchFinish() (because we're in a different viewport > now), so it needs to be calculated in onPinch(). > > You're right, though, that if the point changes, we're going to be out of > sync. This doesn't happen, however - the same point is always passed in to > onPinch for a single pinch gesture (the center point of the user's fingers > as they pinch). So, while I update the point, it's probably not necessary, > and I can change that if you'd prefer. Ok, this makes sense now. I would prefer it (and I think the code would be a lot more readable) if you dealt with all the x/y/range together. So something like this: if (BrowserEventHandler.mReflozPref && !BrowserApp.selectedTab._mReflozPoint) { let data = JSON.parse(aData); let zoomPointX = data.x; let zoomPointY = data.y; BrowserApp.selectedTab._mReflozPoint = { x: zoomPointX, y: zoomPointY, range: BrowserApp.selectedBrowser.contentDocument.caretPositionFromPoint(zoomPointX, zoomPointY) }; } Because the x and y values won't change during a given pinch action, caretPositionFromPoint will always return the same range during that pinch action. Therefore the code you have that checks the return value for nullness and re-tries seems unnecessary. That is, if the first call to caretPositionFromPoint returns null, they will all return null, but you will still end up calling it for every onPinch.

Fixed issues found during review of browser.js code.

I think this is the same patch as the last version.

Ah, sorry... Let me post another version.

Sorry about that - too many parallel trees on my machine. ;)

Also, I merged the formatting and var->let changes in bug 800805 into this patch, although it isn't shown, as I did it after I posted the patch.

Comment on attachment 729084 [details] [diff] [review] b803719 (v4) Review of attachment 729084 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! Couple of whitespace nits below. ::: mobile/android/chrome/content/browser.js @@ +3907,5 @@ > + if (!aRange) { > + Cu.reportError("aRange is null in zoomInAndSnapToRange. Unable to maintain position."); > + return; > + } > + Remove trailing whitespace @@ +3950,5 @@ > + // We only want to do this if reflow-on-zoom is enabled. > + if (BrowserEventHandler.mReflozPref) { > + let range = BrowserApp.selectedTab._mReflozPoint.range; > + this._zoomInAndSnapToRange(range); > + BrowserApp.selectedTab._mReflozPoint = null; Remove one of the spaces before the equals sign.

Updated to include whitespace changes, and forwarding review to dbaron for layout changes.

(In reply to Henri Sivonen (:hsivonen) from comment #34) > It looks like a rather straight-forward and unbikesheddable addition to > CSSOM View. I think we shouldn't prefix. Please send an email to www-style with [cssom-view] or file a spec bug. Thanks!

(In reply to Simon Pieters from comment #45) > (In reply to Henri Sivonen (:hsivonen) from comment #34) > > It looks like a rather straight-forward and unbikesheddable addition to > > CSSOM View. I think we shouldn't prefix. > > Please send an email to www-style with [cssom-view] or file a spec bug. > Thanks! Hi Simon: As David mentioned in comment 31, we did send a message to www-style with [cssom-view] in the subject line, but received very little response: http://lists.w3.org/Archives/Public/www-style/2013Feb/thread.html#msg323

(In reply to Scott Johnson (:jwir3) from comment #46) > Hi Simon: > > As David mentioned in comment 31, we did send a message to www-style with > [cssom-view] in the subject line, but received very little response: > http://lists.w3.org/Archives/Public/www-style/2013Feb/thread.html#msg323 Hm, so this turned out somewhat more harsh than I anticipated. It was not my intent to criticize you, Simon. I was simply trying to point out the message.

Hit enter too early - I meant to add that I apologize if you took it as a criticism.

OK, great, I'll get to it. No offense taken.

dbaron: review ping

Comment on attachment 729154 [details] [diff] [review] b803719 (v5) >+ //XXXjwir3: This currently doesn't work correctly with elements that have >+ // anonymous content in their subtree (e.g. text area elements, >+ // input elements). Once bug 824965 is finished, this method will >+ // likely need to be changed to correctly handle these cases. >+ // We can't successfully handle it now, because the method to >+ // retrieve caret position objects from a point doesn't correctly >+ // handle elements with anonymous content. What we do in this case >+ // is a bit of a hack - we simply return the bounding box of the >+ // element that contains anonymous content, so it's at least a >+ // halfway sensible return value, anyway. So the entire reason we want this separate method is that it can handle anonymous content (without exposing a range object that would give authors pointers into anonymous content that we don't want to expose). What would happen if you just wrote the correct code here now? And it seems like it should be simple to fix bug 824965 -- why not just do that if there's a problem?

Comment on attachment 729154 [details] [diff] [review] b803719 (v5) Re-requesting review of layout changes from tn, based on feedback in the mobile meeting. Timothy, if you don't feel comfortable reviewing this, could you suggest another reviewer? I think dbaron is just swamped, and this bug is blocking the landing of all other reflow-on-zoom bugs. Thanks!

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #51) > What would happen if you just wrote the correct code here now? And it seems > like it should be simple to fix bug 824965 -- why not just do that if > there's a problem? Sure, I can do that.

Comment on attachment 729154 [details] [diff] [review] b803719 (v5) So I think this should: * not be [ChromeOnly] * should behave correctly for offsets in textareas and inputs since we shouldn't expose it to Web content with the incorrect behavior. I don't think it should be hard to fix the latter.

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #54) > Comment on attachment 729154 [details] [diff] [review] > b803719 (v5) > > So I think this should: > * not be [ChromeOnly] > * should behave correctly for offsets in textareas and inputs since we > shouldn't expose it to Web content with the incorrect behavior. > > I don't think it should be hard to fix the latter. So, just to be clear, you'd like bug 824965 to be fixed before we land this bug?

(In reply to Scott Johnson (:jwir3) from comment #55) > So, just to be clear, you'd like bug 824965 to be fixed before we land this > bug? It's not clear to me why that would be necessary.

(In reply to David Baron [:dbaron] (vacation until April 15) (don't cc:, use needinfo? instead) from comment #56) > It's not clear to me why that would be necessary. It seems to me that this is the bug that was filed that indicates caretPositionFromPoint doesn't work correctly within text controls. In order for getClientRect() to work correctly within text controls, I think this needs to be fixed. The good news is that, you were correct, it wasn't difficult to fix this. I'm going to be posting a patch for review to bug 824965 shortly that addresses this issue.

Addressed review comments made by dbaron in comment 54.

Comment on attachment 737506 [details] [diff] [review] b803719 (v6) >+nsDOMCaretPosition::GetClientRect() const >+ if (mAnonymousContentNode) { >+ // If the anonymous content node has a child, then we need to make sure >+ // that we get the appropriate child, as otherwise the offset may not be >+ // correct when we construct a range for it. >+ node = do_QueryInterface(mAnonymousContentNode); >+ nsCOMPtr<nsIDOMNode> firstChild; >+ nsresult rv = node->GetFirstChild(getter_AddRefs(firstChild)); >+ if (NS_SUCCEEDED(rv) && firstChild) { >+ node = firstChild; >+ } Why do we have to get the first child here?

(In reply to Timothy Nikkel (:tn) from comment #60) > Why do we have to get the first child here? Because, in order to construct the dom range, we need the node in which the offset actually occurs (otherwise, we might have the parent, non-anonymous node, which has at most 2 children, and an offset of > 1, which will be rejected by the dom range constructor). Since we're in a text control, it has at most one child, followed by an optional br node. The first child actually contains the textual content, so we get this node and utilize it, if we've detected we're in this situation.

(In reply to Scott Johnson (:jwir3) from comment #61) > (In reply to Timothy Nikkel (:tn) from comment #60) > > Why do we have to get the first child here? > > Because, in order to construct the dom range, we need the node in which the > offset actually occurs (otherwise, we might have the parent, non-anonymous > node, which has at most 2 children, and an offset of > 1, which will be > rejected by the dom range constructor). Since we're in a text control, it > has at most one child, followed by an optional br node. The first child > actually contains the textual content, so we get this node and utilize it, > if we've detected we're in this situation. Ok, I reminded myself what the frametree is. mAnonymousContentNode will be an anonymous-div and it's first child will be text, is that right?

In which case, can we just set mAnonymousContentNode to be the text node when we call SetAnonymousContentNode in nsIDocument::CaretPositionFromPoint instead of adjusting it later?

(In reply to Timothy Nikkel (:tn) from comment #62) > Ok, I reminded myself what the frametree is. mAnonymousContentNode will be > an anonymous-div and it's first child will be text, is that right? Yes. (In reply to Timothy Nikkel (:tn) from comment #63) > In which case, can we just set mAnonymousContentNode to be the text node > when we call SetAnonymousContentNode in nsIDocument::CaretPositionFromPoint > instead of adjusting it later? Yes, I originally planned to do that, but there was a problem. I think we didn't know for certain what the node was at the time the DOMCaretPosition was constructed. The code probably changed from that original state, though, so let me take another look real quick...

> (In reply to Timothy Nikkel (:tn) from comment #63) > > In which case, can we just set mAnonymousContentNode to be the text node > > when we call SetAnonymousContentNode in nsIDocument::CaretPositionFromPoint > > instead of adjusting it later? Ah, ok, I see what you mean. Yeah, I can do this all in one place in CaretPositionFromPoint().

Comment on attachment 737506 [details] [diff] [review] b803719 (v6) Review of attachment 737506 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/base/src/nsDOMCaretPosition.cpp @@ +45,5 @@ > + } else { > + node = do_QueryInterface(mOffsetNode); > + } > + > + nsresult creationRv = nsRange::CreateRange(node, mOffset, node, You should add a CreateRange overload that takes nsINodes instead of nsIDOMNodes.

Updated to address review comments made by tn and Ms2ger (thanks, guys!).

Comment on attachment 738027 [details] [diff] [review] b803719 (v7) >@@ -9319,16 +9319,23 @@ nsIDocument::CaretPositionFromPoint(floa > if (nodeIsAnonymous) { >+ // If the anonymous content node has a child, then we need to make sure >+ // that we get the appropriate child, as otherwise the offset may not be >+ // correct when we construct a range for it. >+ nsCOMPtr<nsIContent> firstChild = anonNode->GetFirstChild(); >+ if (firstChild) { >+ anonNode = firstChild; >+ } > node = ptFrame->GetContent(); > nsIContent* nonanon = node->FindFirstNonChromeOnlyAccessContent(); > nsCOMPtr<nsIDOMHTMLInputElement> input = do_QueryInterface(nonanon); > nsCOMPtr<nsIDOMHTMLTextAreaElement> textArea = do_QueryInterface(nonanon); > bool isText; > if (textArea || (input && > NS_SUCCEEDED(input->MozIsTextField(false, &isText)) && > isText)) { Forgive me for being nitpicky, but should the setting anonNode to the first child be moved into the if (textarea || textfield)? If it's an anonymous node that isn't a textarea or textfield then we don't know what kind of structure it has. Otherwise looks good.

(In reply to Timothy Nikkel (:tn) from comment #68) > Forgive me for being nitpicky, but should the setting anonNode to the first > child be moved into the if (textarea || textfield)? If it's an anonymous > node that isn't a textarea or textfield then we don't know what kind of > structure it has. > > Otherwise looks good. Ah, you're right. Ok, I'll add it in there. r=tn with that?

Comment on attachment 738027 [details] [diff] [review] b803719 (v7) Yeah, just wanted to make sure we were on the same page there.

Added requested fix, and landed on inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/6d29c583d5c0

And, backed out because I forgot to change the commit message from the old one: https://hg.mozilla.org/integration/mozilla-inbound/rev/ab03ce8e20c7 And, relanded with correct commit message: https://hg.mozilla.org/integration/mozilla-inbound/rev/6e5159fc2289