After Bug 171237 has landed is the behavior now that finding a new search result will always center the view to the result. This is a confusing behavior when the next result is only a few lines above/below the current result as it makes the user think he is presented with an entirely new section of text.

+1 Moving the view port unnecessarily destroys the user's mental model of the displayed content. The view port should only shift and vertically center if the search result isn't currently visible.

It does move only when the result is not visible. But it moved the result to the bottom of the page. Now bug 171237 made it always move to the center of the page. The point of this bug is not remove the shifting when the result is anywhere visible on the page. That would be a return to pre-bug 171237. The point of this bug is to make it shift only when the new result is far away from the center (but still visible). If it is somewhere around the center, the shift would be distracting.

(In reply to :aceman from comment #2) > It does move only when the result is not visible. > But it moved the result to the bottom of the page. > Now bug 171237 made it always move to the center of the page. > > The point of this bug is not remove the shifting when the result is anywhere > visible on the page. That would be a return to pre-bug 171237. > > The point of this bug is to make it shift only when the new result is far > away from the center (but still visible). No, the bug summary and comment 0 clearly request that the content window not shift if the next result is visible. You could make the case that if the next result is close enough (say 5-10 lines) to the top or bottom of the window that the view port should center the next result for context, but you shouldn't shift the view port any more than absolutely necessary to avoid destroying the mental model of the visible content.

From reading the comments I assume that smooth scrolling as enabled per default in bug 198964 for "find in page" would solve the commentators issues. By that one sees how far the next result is away from the current. For very long pages that could be an annoyance at least as long bug ug 710372 and bug 202718 are not fixed. Maybe this could be toggled in the find bar similar to "Highlight all".

We're not going to add a preference for this, visible or otherwise. Only centering if the found element isn't already near the center seems like a promising possible approach.

roc, do any of our existing ScrollIntoView flags do what we need here? (not scrolling if the element is already in the viewport)

The closest option is NS_PRESSHELL_SCROLL_ANYWHERE which will scroll the whole target into view somewhere, but won't scroll at all if the target is fully visible. That's what we were using before. The problem is that we can't combine a percentage with NS_PRESSHELL_SCROLL_ANYWHERE. What we really need to be able to do is to separate the preferred scroll position percentage from the control over when to scroll. E.g. we could have, for each axis, -- mWhenToScroll: SCROLL_ALWAYS, SCROLL_IF_PARTIALLY_HIDDEN, SCROLL_IF_COMPLETELY_HIDDEN -- mWhereToScroll: percentage to center on When SCROLL_IF_PARTIALLY_HIDDEN or SCROLL_IF_COMPLETELY_HIDDEN are specified, we would first apply mWhereToScroll to choose a position and then adjust the position to ensure as much of the rectangle as possible is visible. It's probably a good idea to replace the PRIntn percentage parameters with a little struct with those two fields.

My paint-the-bikeshed comment. Keep the current behavior to still center the new word *that is already visible*, but instead of jumping right there, change it to a slower scroll as not to "destroy the user's mental model of the displayed content" As a visual idea, think an iPhone if you put your finger on the new word and dragged it to the center of the screen.

I think we should fix this before shipping bug 171237.

I agree with comment #1. Unless the result of a subsequent Find is beyond the view port (above or below), the page should not reposition. However, I would also reposition the page if the result of a subsequent Find is within the top or bottom very few lines. If the subsequent Find is within the bottom 1-3 lines, opening a tab can cause those lines to be shifted out of the view port (see bug #658188). If bug #658188 is ever fixed, however, then the problem will be at the top few lines.

I think comment 7 is a good idea. roc, do you know of anyone who can take this? I'm currently swamped so I can't do it myself, and I think without this we would need to back out bug 171237, which would make me a sad person...

Just the usual suspects :-)

I believe this feature is currently to-spec, so no need to track for release. If product/UX agree with the feedback and proposed changes (now CC'd), we can either disable the feature for another cycle or take a low-risk fix here.

I don't think we want to ship this feature with this bug. Doesn't "back out or quick fix" mean tracking+?

FWIW, this UX guy thinks this is actually a UX call. The bug was filed (appropriately so) about a UX issue, and so the question should be: do the benefits of the change (centering find results) outweigh the costs of jumping the page for every result. Allow me to be clear: I completely agree that a smoother scroll to the result is a better solution, or even one where we only jump if deemed necessary. I just also believe that the current behaviour in nightlies (ie: jumping) is a strong improvement over what's in our shipping code. Adding uiwanted, as I don't see why the UX group hasn't been asked to make a judgement call on what clearly seems like a UX issue!

I'm on the same page as beltzner here. The product team will work with UX on coming to a recommendation. The feedback so far has been very helpful and highlights many of the concerns, but if comparing to the previous state per bug 171237, this does appear to be a much better experience (although still suboptimal). UX is cc'ed here so we should here from them soon.

I was wrong, we don't need to block bug 171237 on fixing this issue. But we should fix this issue, because it's annoying :)

Note that the original bug #171237 Summary (in 2002) indicated the need to see the context of the found term. Through several revisions of the Summary, this aspect -- providing space above and below the found term -- remained. No mention in the Summary was made about centering the found term until only a month ago. Repeatedly centering the found term when that term was already in the view port will be very annoying. Actually, it can be disorienting to the user.

This patch separates the where (percentage) and when of scrolling as suggested in Comment 7. At large: 1. Are the values used for consumers correct now that both when and where are specified? 2. Further tuning to ScrollToShowRect(). 3. Testing SCROLL_IF_NOT_VISIBLE and SCROLL_IF_PARTLY_VISIBLE. Inre [2]: The behavior is a bit refined. Currently items in view but more than 1/3 screen from ideal will cause scrolling to the middle of current and ideal. If within 1/3, it's left alone. This does help maintain context a bit better. The downside is if the next match is visible, but near the top, it may actually scroll up, which is a bit jarring. Same if jumping to previous matches near the bottom, it may scroll down while highlighting a higher-up match. Haven't found a good way to detect those cases, but will reexamine it.

Comment on attachment 597952 [details] [diff] [review] WIP: Implementation along the lines of comment 7 Review of attachment 597952 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsIPresShell.h @@ +158,5 @@ > +#define NS_PRESSHELL_SCROLL_IF_PARTLY_VISIBLE -3 > + > +typedef struct ScrollContentParameter { > + PRInt16 percentageToScroll; > + PRInt16 whenToScroll; Use "m" prefix for fields. @@ +561,5 @@ > * value of 50 (NS_PRESSHELL_SCROLL_CENTER) centers the frame > + * vertically. > + * WHEN is one of: > + * NS_PRESSHELL_SCROLL_ALWAYS means move the frame regardless > + * of its current visibility. That description doesn't match the current behavior of ANYWHERE. Currently ANYWHERE does not scroll if the frame is already entirely visible. It sounds like that's the same as SCROLL_IF_NOT_FULLY_VISIBLE. But the difference isn't all that clear in comment #7 either. Hmm. @@ +563,5 @@ > + * WHEN is one of: > + * NS_PRESSHELL_SCROLL_ALWAYS means move the frame regardless > + * of its current visibility. > + * NS_PRESSHELL_SCROLL_IF_NOT_VISIBLE means move the frame > + * only if it is not visible. "only if none of it is visible" @@ +566,5 @@ > + * NS_PRESSHELL_SCROLL_IF_NOT_VISIBLE means move the frame > + * only if it is not visible. > + * NS_PRESSHELL_SCROLL_IF_PARTLY_VISIBLE means move the frame > + * only if it is not fully visible (including if it's not > + * visible at all). Note that in this case if the frame is I think SCROLL_IF_NOT_FULLY_VISIBLE would be a bit more clear.

Is it enough to have just SCROLL_IF_NOT_VISIBLE and SCROLL_IF_NOT_FULLY_VISIBLE, combined with the percentages to scroll to if we need to scroll?

Fixes an ongoing scroll exiting the view's boundary causing selection to jump all the way to that end. The last version used a percentage of SCROLL_CENTER for that, where this version uses a new SCROLL_MINIMUM (-1). MINIMUM will scroll the minimum needed to get the frame in view. The other main change here is that ALWAYS now means always, so anything that doesn't always scroll will be moved to IF_NOT_VISIBLE or IF_NOT_FULLY_VISIBLE. ALWAYS is needed for, eg, jumping to an anchor based on a URL fragment, which is always scrolled to the top, even if fully visible. That case was previously handled because it would always scroll to the percentage. Old values that used a percentage are equivalent to a percentage and ALWAYS. The old ANYWHERE is equivalent to MINIMUM and IF_NOT_FULLY_VISIBLE. The next revision will update values to properly reflect these equivalences.

Comment on attachment 598439 [details] [diff] [review] WIP v2: Changes based on suggestions, fix selection scrolling, adds MINIMUM pseudopercentage Review of attachment 598439 [details] [diff] [review]: ----------------------------------------------------------------- This is great work, we're getting a lot closer to something precisely specified and sane. ::: content/base/public/nsISelectionPrivate.idl @@ +191,5 @@ > + * move to a specific point, but just wants the frame visible. > + * When: A value of -1 means to always move the frame where > + * doing so improves the visibility. -2 for moving the > + * frame only when not visible, and -3 for moving the frame > + * only when not entirely visible. I think you should just refer to nsIPresShell.h for the documentation of ScrollContentParameter. ::: layout/base/nsIPresShell.h @@ +155,5 @@ > +#define NS_PRESSHELL_SCROLL_CENTER 50 > +#define NS_PRESSHELL_SCROLL_MINIMUM -1 > +#define NS_PRESSHELL_SCROLL_ALWAYS -1 > +#define NS_PRESSHELL_SCROLL_IF_NOT_VISIBLE -2 > +#define NS_PRESSHELL_SCROLL_IF_NOT_FULLY_VISIBLE -3 How about moving this all into nsIPresShell for namespace control and then we can have enum { SCROLL_TOP = 0, SCROLL_BOTTOM = 100, SCROLL_LEFT = 0, SCROLL_RIGHT = 100, SCROLL_CENTER = 50, SCROLL_MINIMUM = -1 }; and a named enum for mWhenToScroll, say enum WhenToScroll { SCROLL_ALWAYS, SCROLL_IF_NOT_VISIBLE, SCROLL_IF_NOT_FULLY_VISIBLE }; @@ +158,5 @@ > +#define NS_PRESSHELL_SCROLL_IF_NOT_VISIBLE -2 > +#define NS_PRESSHELL_SCROLL_IF_NOT_FULLY_VISIBLE -3 > + > +typedef struct ScrollContentParameter { > + PRInt16 mPercentageToScroll; Given "minimum", which isn't a percentage, let's call this "mWhereToScroll". @@ +159,5 @@ > +#define NS_PRESSHELL_SCROLL_IF_NOT_FULLY_VISIBLE -3 > + > +typedef struct ScrollContentParameter { > + PRInt16 mPercentageToScroll; > + PRInt16 mWhenToScroll; Then this can be WhenToScroll. @@ +562,5 @@ > * value of 50 (NS_PRESSHELL_SCROLL_CENTER) centers the frame > + * vertically. > + * A value of -1 (NS_PRESSHELL_SCROLL_MINIMUM) means the > + * frame is adjusted into view with minimal change from the > + * current view. The behavior of MINIMUM needs to be clarified. I presume it's supposed to be the minimum that makes the whole frame visible. But what if the whole frame doesn't fit in the viewport? @@ +576,5 @@ > + * try to give it increased visibility. > + * @param aHorizontal How to align the frame horizontally and when to do so. > + * See aVertical above, with the caveat that you should use > + * NS_PRESSHELL_SCROLL_LEFT for a value of 0 and use > + * NS_PRESSHELL_SCROLL_RIGHT for a value of 100. This parameter documentation should move to ScrollContentParameter. @@ +621,5 @@ > + virtual bool ScrollFrameRectIntoView(nsIFrame* aFrame, > + const nsRect& aRect, > + ScrollContentParameter aVertical, > + ScrollContentParameter aHorizontal, > + PRUint32 aFlags) = 0; You might as well fix the indent while you're here. ::: layout/base/nsPresShell.cpp @@ +3154,5 @@ > + NS_PRESSHELL_SCROLL_TOP, > + NS_PRESSHELL_SCROLL_ALWAYS), > + ScrollContentParameter( > + NS_PRESSHELL_SCROLL_LEFT, > + NS_PRESSHELL_SCROLL_ALWAYS), Shouldn't we make this SCROLL_MINIMUM instead of LEFT? @@ +3159,1 @@ > ANCHOR_SCROLL_FLAGS); Move this stuff further to the left so you can put each ScrollContentParameter constructor on a single line @@ +3249,5 @@ > + NS_PRESSHELL_SCROLL_TOP, > + NS_PRESSHELL_SCROLL_ALWAYS), > + ScrollContentParameter( > + NS_PRESSHELL_SCROLL_LEFT, > + NS_PRESSHELL_SCROLL_ALWAYS), Ditto

The main change is moving the scroll parameter parts into the nsIPresShell namespace. Scrolls using MINIMUM will behave as ANYWHERE did, favoring the top/left if the frame is too large to fit. Fixed the IF_NOT_FULLY_VISIBLE logic to allow scrolling if the frame is completely out of view. Updated the rest of the consumers' parameters to match their behavior under the old constants.

Reexamining the problem with adjusting the scroll for search mentioned in comment 19. To restate that problem, consider a diagram: |-----!A!-| |---------| | B | | A | | | |!B! | | | > | | | | | | | | | | |---------| |---------| User found the match at A, then scrolled down so that the next match, B, is within the adjustment zone. When the user jumps to B, the view is scrolled up, despite the fact that the next selection is down. The view moving in opposition to the selection direction may confuse the user briefly. A flag could be added by nsTypeAheadFind if the new match is above the previous. If the adjustment would scroll down, but the hint is there, the adjustment doesn't happen, and vice versa. The tradeoff here is scrolling instability. With that change, the scrolling isn't stable (ie, going back from B to A will cause the view to scroll up where going A to B didn't). Simply removing the adjustment would make the scrolling behavior consistent with Webkit browsers (ie, center if not fully visible).

Shall we review the patch you've got before worrying about that?

Review can proceed. The patch mentioned in comment 25 is not much code (about 70 lines longer, but 2/3 of that is context). The third option mentioned would cut the patch by maybe 20 lines. The bulk of the changes will be whatever comes out of review from the current patch, and when UX weighs in the changes should be relatively minor (ie, on the order of changes mentioned above).

Comment on attachment 599029 [details] [diff] [review] WIP v3: Address comments, correct parameter values, fix IF_NOT_FULLY_VISIBLE. Review of attachment 599029 [details] [diff] [review]: ----------------------------------------------------------------- A lot of your constructors for ScrollParameters aren't relying on the default parameters when they could. That's worth fixing to make a lot of the calling code shorter. ::: layout/base/nsIPresShell.h @@ +555,5 @@ > + * are given precedence. > + * Other values are treated as a percentage, and the point > + * "percent" down the frame is placed at the point "percent" down > + * the visible area. > + * @param WHEN: Put this comment at the constructor and say aWhen/aWhere instead of WHEN/WHERE. Also document in the constructor what the default values are. @@ +571,5 @@ > + WhenToScroll mWhenToScroll; > + ScrollContentParameter(PRInt16 aWhere = SCROLL_MINIMUM, > + WhenToScroll aWhen = SCROLL_IF_NOT_FULLY_VISIBLE) : > + mWhereToScroll(aWhere), mWhenToScroll(aWhen) {} > +} ScrollContentParameter; How about shortening this name? Maybe ScrollAxis? ::: layout/base/nsPresShell.cpp @@ +3351,5 @@ > > + bool needScroll = false; > + bool canAdjustScroll = false; > + > + // FIXME if next spot is lower, don't scroll up (vice versa) Take this out @@ +3359,5 @@ > + // Used in common among branches > + bool topVis = (aRect.y >= visibleRect.y && > + aRect.y < visibleRect.YMost()); > + bool bottomVis = (aRect.YMost() > visibleRect.y && > + aRect.YMost() <= visibleRect.YMost()); remove unnecessary parens. Use better variable names: topEdgeVisible, bottomEdgeVisible. And move these down closer to where they'll be used. @@ +3360,5 @@ > + bool topVis = (aRect.y >= visibleRect.y && > + aRect.y < visibleRect.YMost()); > + bool bottomVis = (aRect.YMost() > visibleRect.y && > + aRect.YMost() <= visibleRect.YMost()); > + bool fitsInViewY = aRect.height <= visibleRect.height; Just inline this at its single usage point. @@ +3390,5 @@ > + needScroll = true; > + } else if (nsIPresShell::SCROLL_IF_NOT_VISIBLE == aVertical.mWhenToScroll) { > + // Scroll only if no part of the frame is visible in this view > + needScroll = (!topVis && aRect.y < visibleRect.y) || > + (!bottomVis && aRect.YMost() > visibleRect.YMost()); Can't this be simplified to needScroll = aRect.YMost() <= visibleRect.y || aRect.y >= visibleRect.YMost()? @@ +3395,5 @@ > + canAdjustScroll = !needScroll; > + } else if (nsIPresShell::SCROLL_IF_NOT_FULLY_VISIBLE == aVertical.mWhenToScroll) { > + // Scroll only if part of the frame is hidden and more can fit in view > + needScroll = !topVis || !bottomVis; > + canAdjustScroll = !needScroll; It's not clear what canAdjustScroll means. I'm not sure why it's needed at all. You've removed the references to lineSize. That seems like it's probably a mistake. I think this code could be made cleaner by first computing whether we need to scroll vertically. If we do need to scroll, then compute where to scroll by updating scrollPt.y. It might be worth having separate helper functions ComputeNeedToScroll and ComputeWhereToScroll that take ScrollAxis, visibleRect.y, visibleRect.YMost() etc as parameters. Then we could use those functions again for horizontal scrolling. @@ +3500,5 @@ > NS_ASSERTION(mDidInitialReflow, "should have done initial reflow by now"); > > mContentToScrollTo = aContent; > + mContentScrollVPosition = aVertical.mWhereToScroll; > + mContentScrollHPosition = aHorizontal.mWhereToScroll; Shouldn't we save the whole of aVertical/aHorizontal instead of just 'where'? ::: layout/generic/nsSelection.cpp @@ +310,5 @@ > + nsTypedSelection *mTypedSelection; > + SelectionRegion mRegion; > + nsIPresShell::ScrollContentParameter mVerticalScroll; > + nsIPresShell::ScrollContentParameter mHorizontalScroll; > + bool mFirstAncestorOnly; Don't reindent these fields.

The line size is used (for the IF_NOT_VISIBLE case) to require at least a full scroll line within the frame to be considered visible. The IF_NOT_FULLY_VISIBLE handles that by checking that both ends are in the visible frame. It's worth noting the current version of the patch doesn't have any consumers using IF_NOT_VISIBLE. The three uses from the original code are all replaced by the default of IF_NOT_FULLY_VISIBLE. The canAdjustScroll and the associated code was an attempt to adjust the view in cases where the rect was already visible but not in its ideal (as specified as the Where) position. That's the issue mentioned in comment 25. It's removed here, and that kind of tuning can be revisited as a separate patch and/or separate bug if needed.

Sudden thought: How will this work if the page is wider than the viewport and showing the next result requires horizontal scrolling?

(In reply to David E. Ross from comment #30) > Sudden thought: How will this work if the page is wider than the viewport > and showing the next result requires horizontal scrolling? Currently the same (that wasn't changed by bug 171237): minimally scrolled to get the far edge into the view, preferring the left edge if the target is longer than the view width. Webkit centers horizontally if scrolling is needed.

Comment on attachment 601455 [details] [diff] [review] WIP v4: Changes based on review comments. Review of attachment 601455 [details] [diff] [review]: ----------------------------------------------------------------- This is very close! ::: layout/base/nsPresShell.cpp @@ +3327,5 @@ > aRect = frameBounds; > } > } > > +static bool ComputeNeedToScroll(nsIPresShell::WhenToScroll aWhenToScroll, break after 'bool' so the function declaration doesn't have to indent so much. Same below. @@ +3334,5 @@ > + nscoord aRectMax, > + nscoord aRectSize, > + nscoord aViewMin, > + nscoord aViewMax, > + nscoord aViewSize) { aRectSize and aViewSize are not used. @@ +3346,5 @@ > + aRectMin + aLineSize >= aViewMax; > + } else if (nsIPresShell::SCROLL_IF_NOT_FULLY_VISIBLE == aWhenToScroll) { > + // Scroll only if part of the frame is hidden and more can fit in view > + return !(aRectMin >= aViewMin && aRectMin < aViewMax) || > + !(aRectMax > aViewMin && aRectMax <= aViewMax); Shouldn't this be return !(aRectMin >= aViewMin && aRectMax <= aViewMax); or maybe return !(aRectMin >= aViewMin && aRectMax <= aViewMax) && NS_MIN(aViewMax, aRectMax) - NS_MAX(aRectMin, aViewMin) < aViewMax - aViewMin; ? (The second option explicitly checks that more of the frame can be made visible than is currently visible.) @@ +3358,5 @@ > + nscoord aRectMax, > + nscoord aRectSize, > + nscoord aViewMin, > + nscoord aViewMax, > + nscoord aViewSize) { aRectSize and aViewSize are used, but they're just max-min, so just use max-min where you need to.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (away February 21 to March 17) from comment #32) > (The second option explicitly checks that more of the frame can be made > visible than is currently visible.) Went this route, and updated comment under nsIPresShell.h:ScrollAxis to clarify. Consumers can use ALWAYS if they want a specific scroll posture to result. This could go the other way, but if the scrolling is being done to highlight an element, it will either use ALWAYS to put it at a specific point in the view or will have secondary highlight mechanism (such as selection).

Comment on attachment 602110 [details] [diff] [review] WIP v5: Update based on comments Review of attachment 602110 [details] [diff] [review]: ----------------------------------------------------------------- Excellent!

patching file layout/base/nsIPresShell.h Hunk #1 FAILED at 141

backed out TEST-UNEXPECTED-FAIL | /tests/layout/base/tests/test_scroll_selection_into_view.html | an unexpected uncaught JS exception reported through window.onerror - selection.scrollIntoView is not a function at http://mochi.test:8888/tests/layout/base/tests/test_scroll_selection_into_view.html:61 TEST-UNEXPECTED-FAIL | /tests/layout/generic/test/test_plugin_mouse_coords.html | p2 mouse down Y - got 178, expected 18 TEST-UNEXPECTED-FAIL | /tests/layout/generic/test/test_plugin_mouse_coords.html | p3 mouse down X - got 26, expected 6

To fix the first test failure, this splits nsTypedSelection::ScrollIntoView into two functions. The function signature is reverted so that JavaScript calls only specify where to scroll. A new ScrollIntoViewInternal method is spun off to specify a full ScrollAxis for both axes, and the non-script consumers are updated to use it instead. The scriptable version simply wraps the percentages into ScrollAxis and calls the -Internal version. To fix the other pair of failures, the consumer nsFocusManager::ScrollIntoView needed to be adjusted to use SCROLL_IF_NOT_VISIBLE rather than the default (IF_NOT_FULLY_VISIBLE). An alternative would be to modify the test. Those two failures were caused by the frames being partially visible and being scrolled, where before they weren't scrolled.

Comment on attachment 607384 [details] [diff] [review] Split off nsSelection::ScrollIntoView into two methods, adjust nsFocusManager::ScrollIntoView Review of attachment 607384 [details] [diff] [review]: ----------------------------------------------------------------- Excellent! BTW, please get into the habit of requesting review on your patches :-) r+ with the IID rev ::: content/base/public/nsISelectionPrivate.idl @@ +68,2 @@ > > [scriptable, uuid(1820a940-6203-4e27-bc94-fa81131722a4)] You need to put a new IID in here. Sorry I missed this before.

Does the implementation of this RFE work when the preference variable browser.findbar.enabled is set to "False", which gives the Find popup in place of the Find tool bar? See bug #748991.