372797 - Convert ScrollFrameIntoView users to use ScrollContentIntoView

Status: RESOLVED FIXED

(Core :: General, defect)

Description

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

See bug 372665.
Better to handle scrolling into view consistently.
So, if possible, only ScrollContentIntoView should be used, and
ScrollFrameIntoView and ScrollContentIntoView could be then merged together.

Comment 1

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

Attachment: patch

The a bit ugly part is in nsAccessNode::ScrollTo.
I basically inlined part of nsAccessNode::GetFrame to get the right
content easily.

nsGenericHTMLElement::GetFormControlFrameFor should do at least as 
"powerful" flush as ScrollContentIntoView to ensure everything is ok.

With the patch nsLayoutUtils::ScrollIntoView isn't needed anymore, because nsGenericHTMLFormElement::SetFocusAndScrollIntoView does
the scrolling.

Comment 2

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

Why don't you just have nsAccessNode call GetFrame() and then use that frame's GetContent()?

Are you fixing a bug where nsAccessNode::ScrollTo needed to flush before calling GetFrame, but wasn't?

  nsIFrame* frame = presShell->GetPrimaryFrameFor(c);

Can you just remove this (from nsSpatialNavigation::setFocusedContent) and the "if (frame)" test?

Otherwise looks good.

Comment 3

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

Attachment: fixed comments

Comment 4

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

Didn't you already land this?

Comment 5

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

Yes. "RESOLVED FIXED" ;)

Comment 6

[Aaron Leventhal]

For the nsAccessNode change, it would have been better to do:
nsCOMPtr<nsIContent> content = do_QueryInterface(mDOMNode);

It avoids a GetPrimaryFrameFor() -- no big deal though.

Comment 7

[Boris Zbarsky [:bzbarsky]]

So I'm a little confused here.  What was the reason for the GetFormControlFrameFor change (to flush layout, not just frames)?

Comment 8

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

IIRC, (my memory isn't too good when trying to remember things I did 22 months ago) it was because the old ScrollContentIntoView used to flush
layout.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Yes, but this function is called for things other than scrolling, and some of those are perf-sensitive.  Shouldn't SetFocusAndScrollIntoView handle the flushing itself if that's what it needs?

Comment 10

[Boris Zbarsky [:bzbarsky]]

OK, so I'm going to back out that change in bug 67752.