1553013 - Factor out some functionalities in PresShell::ScrollFrameRectIntoView into new functions

Status: RESOLVED WONTFIX

(Core :: Layout, enhancement, P2)

Description

[Hiroyuki Ikezoe (:hiro)]

Similar to bug 1553012, but this is for selection. For selection we need to use PresShell::ScrollFrameRectIntoView across document boundaries. layout/base/tests/test_scroll_selection_into_view.html has a test case that is selection happens in an iframe?

Comment 2

[Hiroyuki Ikezoe (:hiro)]

I did a brief chat with masayuki about this (in the bus heading to Whister), and the conclusion is that we don't need to scroll selections across document boundaries. So probably we should set ScrollNoParentFrames in the case where we call ScrollFrameRectIntoView for selections.

Comment 3

[Matt Woodrow (:mattwoodrow)]

It'd be nice to split the function into two, have ScrollFrameRectIntoViewCrossDoc (that returns void, since we can't know the answer synchronously with fission), and normal ScrollFrameRectIntoView (which implies ScrollNoParentFrames, and returns bool).

Comment 4

[Hiroyuki Ikezoe (:hiro)]

This would be a bit cleaner once after we drop PresShell* argument from ScrollToShowRect in bug 1562503.

(In reply to Matt Woodrow (:mattwoodrow) from comment #3)

It'd be nice to split the function into two, have ScrollFrameRectIntoViewCrossDoc (that returns void, since we can't know the answer synchronously with fission), and normal ScrollFrameRectIntoView (which implies ScrollNoParentFrames, and returns bool).

Now I think we need three different ScrollFrameRectIntoViews;

1. ScrollRectIntoView
2. ScrollRectIntoViewSameOriginDoc for Element.scrollIntoView (bug 1561754 is to restrict it in the same origin)
3. ScrollRectIntoViewCrossOriginDoc for 'Find in page'

Comment 5

[Neha Kochar [:neha]]

Hiro, what are the next steps here? This sounds like something we should track for Fission, correct?

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Changing bug tile to reflect what we should do.

Comment 7

[Neha Kochar [:neha]]

I believe this should be Fission release blocker (M8).

Comment 8

[Neha Kochar [:neha]]

Hiro, will you be working on this for Fission M8?

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Though I didn't have the plan to work on this, but if this needs an owner, I will manage to take time on this until M8.

Comment 10

[Nika Layzell [:nika] (ni? for response)]

Is this bug for fixing an error in the fission case (e.g. scrolling works incorrectly), or is it a refactoring task? I'm having a bit of trouble telling from the bug description :-)

Comment 11

[Hiroyuki Ikezoe (:hiro)]

This is a refactoring task.

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Looking at the ScrollFrameRectIntoView function, it turns out splitting it into cross or in-process ones is not a good idea because the function is basically walking up frame tree and scrolling each scrollable frame until there is no parent frame and even if there is no parent frame if it's in OOP iframe, then deferring the scroll-frame-into-view task to the parent content. So I am going to just factor out some functionalities in the function to make it smaller.

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1553013 - Factor out a function to get target rectagle where we want to be visible in the given scrollable frame. r?emilio

Comment 14

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1553013 - Factor out a function to get the parent frame and its rect in the same process. r?emilio

Depends on D116958

Comment 15

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1553013 - Factor out a function to defer scroll-rect-into-view task to the parent content if it's necessary. r?emilio

Depends on D116959

Comment 16

[Hiroyuki Ikezoe (:hiro)]

As discussed with Emilo on Matrix, we are not going to touch the function (for now). If someone has idea to make it less messy, please feel free to do it in a different bug. Thanks!