601352 - Console does not scroll expression result into view

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Kevin Dangoor]

If the log output fills the console output area and you type an expression (eg 1+1), the 1+1 is scrolled into view but the 2 is just below the view.

Comment 1

[Kevin Dangoor]

blocking requested, as this bug makes the JS command line very annoying in practice.

Comment 2

[Mihai Sucan [:msucan]]

Attachment: proposed fix and test code

Proposed fix and test code.

Comment 3

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 481255 [details] [diff] [review]
proposed fix and test code

I don't know. I'd try a full run of content mochitests with this patch before signing off on it. Ask a layout peer.

Comment 4

[Mihai Sucan [:msucan]]

I ran all browser-chrome-tests (9000+ of them). No failures related to this change. Shall I ask a layout peer then?

Comment 5

[Mihai Sucan [:msucan]]

Comment on attachment 481255 [details] [diff] [review]
proposed fix and test code

Asking for review from Simon Montagu.

Comment 6

[Simon Montagu :smontagu]

Comment on attachment 481255 [details] [diff] [review]
proposed fix and test code

Is there any reason not to make the same change in nsScrollBoxObject::ScrollToElement?

Comment 7

[Mihai Sucan [:msucan]]

(In reply to comment #6)
> Comment on attachment 481255 [details] [diff] [review]
> proposed fix and test code
> 
> Is there any reason not to make the same change in
> nsScrollBoxObject::ScrollToElement?

Thanks for looking into the patch!

Given it's my first patch for Gecko itself, I tried to keep changes minimal. If the change is fine, then I can do the same for ScrollToElement. Shall I do that?

Comment 8

[Mihai Sucan [:msucan]]

Attachment: updated patch

Updated patch. Changes:

- did the same change in ScrollToElement as in EnsureElementIsVisible - as requested by Simon. Thanks for your comment!

- rebased the patch on top of the latest mozilla-central. This required fixes for mochitest.

Asking for review from Robert O'Callahan as suggested by Simon.

Comment 9

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

I think rect.x/y are relative to the top-left of the document, ignoring scrolling, but crect is relative to the top-left of the scrollbox border-box. So this doesn't seem right.

How about you just rip out the guts of ScrollToElement and make it call nsIPresShell::ScrollContentIntoView? EnsureElementIsVisible can probably do that too (just pass NS_PRESSHELL_SCROLL_ANYWHERE).

Comment 10

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

You might want to extend ScrollContentIntoView to accept the SCROLL_FIRST_ANCESTOR_ONLY and SCROLL_OVERFLOW_HIDDEN flags, since to preserve current scrollbox behavior we'd need to pass those flags. (A call to scrollToElement does not currently cause any ancestors of the scrollbox to scroll.)

Comment 11

[Mihai Sucan [:msucan]]

Attachment: patch update 3

Updated patch with the changes requested. Thanks a lot for your comments!

Changes:

- added a new aFlags argument to PresShell::ScrollContentIntoView() which takes the SCROLL_FIRST_ANCESTOR_ONLY SCROLL_OVERFLOW_HIDDEN flags (same as ScrollFrameRectIntoView()).

- added the same aFlags argument to DoScrollContentIntoView().

- updated accordingly all the code that calls the two methods.

- changed nsScrollBoxObject::ScrollToElement() to use PresShell::ScrollContentIntoView().

- changed nsScrollBoxObject::EnsureElementIsVisible() to use PresShell::ScrollContentIntoView().

- updated the mochitest file to better check if the last logged message is visible.

All mochitests pass, no failures on my system.

I am not sure if the new aFlags function argument is fine... or if I should add a new ScrollContentIntoView() function that takes four arguments - such that all other calls wouldn't need to be changed. Perhaps this is only a matter of preference.

Please let me know if I have to make further changes. Thanks again!

Comment 12

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

Comment on attachment 485125 [details] [diff] [review]
patch update 3

Need to bump IID on nsIPresShell.

This may need to land on beta7 branch due to the IID bump. I'm not sure. Please ask #developers or elsewhere.

Great patch! I love to see all that nasty code being removed :-)

Comment 13

[Mihai Sucan [:msucan]]

Attachment: [checked-in] patch update 4

Bumped IID as requested.

Thanks for the review+ Robert!

Asking for approval2.0+ to get this patch checked into beta7 and into mozilla-central default branch.

Comment 14

[Mike Beltzner [:beltzner, not reading bugmail]]

Can go in over the b7 freeze, checkin with a=roc/beltzner for b7 or whatever. Post b7 if you need to make these sorts of changes, you'll have to create new and exciting interfaces.

Comment 15

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 485248 [details] [diff] [review]
[checked-in] patch update 4

http://hg.mozilla.org/mozilla-central/rev/0046c811c921