724585 - We need a way to scroll a page to center an element if the element is not visible

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Paul Rouget [:paul]]

Like webkit does with scrollIntoViewIfNeeded (see bug 403510).
This doesn't have to be exposed to the content.

Comment 1

[Paul Rouget [:paul]]

We could use that: https://gist.github.com/2581101

Comment 2

[hsablonniere]

My implementation doesn't handle nested scrolling areas and positionned elements. I'll work on that.

What do you mean by "this doesn't have to be exposed"? Can we expose a method on DOM Elements only for devtools? If yes, how?

Comment 3

[Paul Rouget [:paul]]

Hi Hubert!

(In reply to hsablonniere from comment #2)
> My implementation doesn't handle nested scrolling areas and positioned
> elements. I'll work on that.

We can see that after.

> What do you mean by "this doesn't have to be exposed"? Can we expose a
> method on DOM Elements only for devtools? If yes, how?

I meant we need a devtools-only function.

This is how I would do that:

1) Implement a scrollIntoViewIfNeeded method in this file: browser/devtools/shared/LayoutHelpers.jsm (http://mxr.mozilla.org/mozilla-central/source/browser/devtools/shared/LayoutHelpers.jsm) (make sure to do some incremental building, no need to rebuild the whole firefox tree - https://developer.mozilla.org/en/Incremental_Build)

2) Test manually with a chrome-enabled Scratchpad (see https://developer.mozilla.org/en/Tools/Scratchpad#Using_Scratchpad_to_access_Firefox_internals). Do something like that:

> Components.utils.import("resource:///modules/devtools/LayoutHelpers.jsm");
> LayoutHelpers.YourMethod(blablabla)

3) once you get something working, upload a patch, and flag it for "feedback", in the field, add my name: ":paul"

Once there, we will see what to do, but I guess the next step will be to write some tests

If you need any help, ping us on #devtools!


Thank you :D

Comment 4

[Thaddee Tyl [:espadrine]]

Attachment: ScrollIntoView that centers the focused element in the viewport.

This patch fixes this particular bug, and also uses the created api
by solving bug <https://bugzilla.mozilla.org/show_bug.cgi?id=724071>.

In the process, I'm also trying to standardize a general solution
so that we don't have to struggle for this again
<http://lists.w3.org/Archives/Public/www-style/2012May/1011.html>.

Comment 5

[Paul Rouget [:paul]]

Comment on attachment 628033 [details] [diff] [review]
ScrollIntoView that centers the focused element in the viewport.

+    if (centered && elem.ownerDocument.body.offsetHeight

Are we sure there's always a body? What about SVG and XUL documents?

Some other things would be nice to have: horizontal alignment and alignment of elements in iframes.

We will need some tests too.

Comment 6

[Thaddee Tyl [:espadrine]]

I have manually tested XUL with chrome://browser/content/browser.xul
and SVG with https://thefiletree.com/test/test.svg?plug=none
and it works.

But I agree, we need tests.

Can you give me an indication of where I can put those tests?

Comment 7

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

tests live in:

browser/devtools/highlighter/test

Comment 8

[hsablonniere]

I was working on this bug as well :-(

Thaddée seems more advanced on the subject.

I'll just check updates on this ticket from now on.

Comment 9

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

Comment on attachment 628033 [details] [diff] [review]
ScrollIntoView that centers the focused element in the viewport.

+
+  /**
+   * Scroll the document so that the elemen `elem appears in the viewport.
+   *

the element "elem" ?

+    centered = centered === undefined? true: !!centered;

you could just move !!centered into the if condition down below.

+    elem.scrollIntoView(true);
+    let window = elem.ownerDocument.defaultView;
+
+    if (centered && elem.ownerDocument.body.offsetHeight

paul's concern about .body and .offset were good, but in this case, it's being called with an element from the InsideOutBox. It's always an HTML document.

if we allow nodes from any arbitrary doctype, we'll need to be more clever.

Probably worth documenting that the node "elem" has to be from an HTML document for this function to work.

+        !== window.scrollY + window.innerHeight) {

.scrollY
+ innerHeight  (as in, we're not already scrolled to bottom?)

I have a feeling this calculation should be more of a >= kind of comparison. Like, if the off

+      // It hasn't hit the bottom of the page.
+      window.scrollBy(0, -window.innerHeight / 2);

word.

canceling review until round 2 (with tests!)

Comment 10

[Thaddee Tyl [:espadrine]]

(In reply to Rob Campbell [:rc] (:robcee) from comment #9)
> Comment on attachment 628033 [details] [diff] [review]
> ScrollIntoView that centers the focused element in the viewport.
> 
> +    centered = centered === undefined? true: !!centered;
> 
> you could just move !!centered into the if condition down below.

This line is there to deal with the default value, which should be "true".
Putting !!centered in the if condition would make it "false".

This is to approximate webkit's behaviour. I can remove the "centered"
parameter altogether.

> +    elem.scrollIntoView(true);
> +    let window = elem.ownerDocument.defaultView;
> +
> +    if (centered && elem.ownerDocument.body.offsetHeight
> 
> paul's concern about .body and .offset were good, but in this case, it's
> being called with an element from the InsideOutBox. It's always an HTML
> document.
> 
> if we allow nodes from any arbitrary doctype, we'll need to be more clever.
> 
> Probably worth documenting that the node "elem" has to be from an HTML
> document for this function to work.

Yes, my bad… I misunderstood Paul's comment. I've been working on
a more generic solution last week and during the week-end, but
it turns out to be much harder than I imagined. The DOM inspector
is a challenge, too.

I would like to continue working on that, but this patch is meant as
something that works in the general case (meaning, here, when applied
to documents with a body).

> +        !== window.scrollY + window.innerHeight) {
> 
> .scrollY
> + innerHeight  (as in, we're not already scrolled to bottom?)
> 
> I have a feeling this calculation should be more of a >= kind of comparison.
> Like, if the off
> 
> +      // It hasn't hit the bottom of the page.
> +      window.scrollBy(0, -window.innerHeight / 2);
> 
> word.

Done! (It really achieves the same thing, because this always has
a pixel-perfect precision, except for pages whose body is vertically
bigger than 2^16 pixels.)

Comment 11

[hsablonniere]

Thaddée, do you handle nested scrolling areas?

Comment 12

[Thaddee Tyl [:espadrine]]

hsablonniere: in this patch? I think so.
Can you test that?

Comment 13

[Thaddee Tyl [:espadrine]]

Attachment: Way to scroll a page to center an element if the element is not visible.

The function is in LayoutHelpers.scrollIntoViewIfNeeded.

It is based on an exact reverse-engineering of Webkit's behavior
in the function of the same name.

This patch solves bug 724071.

It also includes several tests to check that the behavior is what
we'd expect.

Comment 14

[Thaddee Tyl [:espadrine]]

Attachment: Way to scroll a page to center an element if the element is not visible.

This patch is pretty much the same as above, without the commented out part.

Comment 15

[Thaddee Tyl [:espadrine]]

Attachment: Way to scroll a page to center an element if the element is not visible.

Comment 16

[Thaddee Tyl [:espadrine]]

Attachment: Way to scroll a page to center an element if the element is not visible.

Comment 17

[Dave Camp (:dcamp)]

Comment on attachment 628398 [details] [diff] [review]
Way to scroll a page to center an element if the element is not visible.

Review of attachment 628398 [details] [diff] [review]:
-----------------------------------------------------------------

I have a few questions inline, feel free to re-ask review with answers or a new patch.

::: browser/devtools/shared/LayoutHelpers.jsm
@@ +217,5 @@
> +  /**
> +   * Scroll the document so that the elemen "elem" appears in the viewport.
> +   *
> +   * @param Element elem the element that needs to appear in the viewport.
> +   * @param bool centered true if you want it centered, false if you want it to

What about "closest match"?  In some cases (for example, keybindings moving between items in a list) if something's below the current viewport you want it to show up at the bottom.

@@ +225,5 @@
> +  scrollIntoViewIfNeeded:
> +  function LH_scrollIntoViewIfNeeded(elem, centered) {
> +    centered = centered === undefined? true: !!centered;
> +
> +    let win = elem.ownerDocument.defaultView;

This scrolls the window in which the element lives.  This is right sometimes, but what about when the element lives in an iframe that is offscreen?

In that case we also need to scroll the parent element into view, right?

@@ +235,5 @@
> +      do {
> +        offtop += el.offsetTop;
> +        offleft += el.offsetLeft;
> +      } while (!!(el = el.offsetParent));
> +    }

Can you just get this with getBoundingClientRect?

@@ +248,5 @@
> +        leftToRight = offleft + elem.offsetWidth,
> +        rightToLeft = - win.innerWidth + leftToRight - elem.offsetWidth,
> +        xAllowed = true,  // We allow one translation on the x axis,
> +        yAllowed = true;  // and one on the y axis.
> +

There's a preference in the browser codebase for let-statement-per-line rather than comma-separated lines.

Comment 18

[Thaddee Tyl [:espadrine]]

> ::: browser/devtools/shared/LayoutHelpers.jsm
> @@ +217,5 @@
> > +  /**
> > +   * Scroll the document so that the elemen "elem" appears in the viewport.
> > +   *
> > +   * @param Element elem the element that needs to appear in the viewport.
> > +   * @param bool centered true if you want it centered, false if you want it to
> 
> What about "closest match"?  In some cases (for example, keybindings moving
> between items in a list) if something's below the current viewport you want
> it to show up at the bottom.

I actually have that implemented in this patch (and in the next,
which I am working on as I write this). I test for that behavior
in the mochitests.

Do you want me to document all those cases in the head comment?

> @@ +225,5 @@
> > +  scrollIntoViewIfNeeded:
> > +  function LH_scrollIntoViewIfNeeded(elem, centered) {
> > +    centered = centered === undefined? true: !!centered;
> > +
> > +    let win = elem.ownerDocument.defaultView;
> 
> This scrolls the window in which the element lives.  This is right
> sometimes, but what about when the element lives in an iframe that is
> offscreen?
> 
> In that case we also need to scroll the parent element into view, right?

Isn't there some CORS security issues there?

> @@ +235,5 @@
> > +      do {
> > +        offtop += el.offsetTop;
> > +        offleft += el.offsetLeft;
> > +      } while (!!(el = el.offsetParent));
> > +    }
> 
> Can you just get this with getBoundingClientRect?

Sure! I actually did the thing that obviously works,
but it might be faster using getBoundingClientRect.

I'm rewriting it so as to use this method. The end result is
exactly the same.

> @@ +248,5 @@
> > +        leftToRight = offleft + elem.offsetWidth,
> > +        rightToLeft = - win.innerWidth + leftToRight - elem.offsetWidth,
> > +        xAllowed = true,  // We allow one translation on the x axis,
> > +        yAllowed = true;  // and one on the y axis.
> > +
> 
> There's a preference in the browser codebase for let-statement-per-line
> rather than comma-separated lines.

Done.

Comment 19

[Thaddee Tyl [:espadrine]]

Attachment: Way to scroll a page to center an element if the element is not visible.

Relying on Dave Camp's review, this patch uses getBoundingClientRect
and corrects the coding style.

Comment 20

[Paul Rouget [:paul]]

Just tested the patch. It looks good. Let me describe the iframe problem Dave mentioned:

when we will use this method for the highlighter, we will want to scroll to the highlighted element.
This element can be in an iframe. Maybe in 2 iframes (window > iframe > iframe > element).

So this code needs to include a recursive-magic-way to make sure the element is visible, as well as its window!
See this URL: http://jsbin.com/inanes/3 (scroll to the bottom right corner).

Comment 21

[Thaddee Tyl [:espadrine]]

(In reply to Paul Rouget [:paul] from comment #20)
> Just tested the patch. It looks good. Let me describe the iframe problem
> Dave mentioned:
> 
> when we will use this method for the highlighter, we will want to scroll to
> the highlighted element.
> This element can be in an iframe. Maybe in 2 iframes (window > iframe >
> iframe > element).
> 
> So this code needs to include a recursive-magic-way to make sure the element
> is visible, as well as its window!
> See this URL: http://jsbin.com/inanes/3 (scroll to the bottom right corner).

I'll do what I can. I was (and still am) wondering how chrome javascript
interacts with CORS. Do we have access to everything?
Is there a risk of modifying the scrolled state of XUL components?

Comment 22

[Thaddee Tyl [:espadrine]]

Attachment: Way to scroll a page to center an element if the element is not visible.

The scrollIntoViewIfNeeded function now takes care of nested iframes.
There are four more mochitests to check for correct behavior.

Comment 23

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

Comment on attachment 630398 [details] [diff] [review]
Way to scroll a page to center an element if the element is not visible.

in LayoutHelpers.jsm:

nit… 
+  /**
+   * Scroll the document so that the elemen "elem" appears in the viewport.

typo, "element".

+  scrollIntoViewIfNeeded:
+  function LH_scrollIntoViewIfNeeded(elem, centered) {
+    centered = centered === undefined? true: !!centered;

this is a case where it might be nice to have your parameter named aCentered.

then you could just do:

let centered = !!aCentered;

and do away with the conditional.

I think you should do that and comment why you're doing it.

+    // Think of them as geometrical vectors, it helps.
+    // The axes are directed downwards and towards the right.

I think this comment is more confusing than without.

"Origin is at top-left" is less confusing to my brain than "Axes directed downwards and towards the right". :)

I think those two lines should go.

+    let xAllowed = true;  // We allow one translation on the x axis,
+    let yAllowed = true;  // and one on the y axis.

+    if ((topToBottom > 0 || !centered) && topToBottom <= elem.offsetHeight) {
+      if (yAllowed) {

yAllowed will be true here, no need to check it.

+
+    if ((leftToRight > 0 || !centered) && leftToRight <= elem.offsetWidth) {
+      if (xAllowed) {

no need for this check either.

This looks OK to me, nits aside. should work. R+ with the above changes.

Comment 24

[Thaddee Tyl [:espadrine]]

(In reply to Rob Campbell [:rc] (:robcee) from comment #23)
> +  scrollIntoViewIfNeeded:
> +  function LH_scrollIntoViewIfNeeded(elem, centered) {
> +    centered = centered === undefined? true: !!centered;
> 
> this is a case where it might be nice to have your parameter named aCentered.
> 
> then you could just do:
> 
> let centered = !!aCentered;

Actually, we want to default to true, which requires "centered" to be true when
"aCentered" is undefined, which "!!aCentered" does not do.

> I think those two lines should go.
> 
> +    if ((topToBottom > 0 || !centered) && topToBottom <= elem.offsetHeight)
> {
> +      if (yAllowed) {
> 
> yAllowed will be true here, no need to check it.
> 
> +
> +    if ((leftToRight > 0 || !centered) && leftToRight <= elem.offsetWidth) {
> +      if (xAllowed) {
> 
> no need for this check either.

Ok!

I have put them there because I like to have symmetry in my code. It helps legibility.

I'll push a patch soon.

Comment 25

[Thaddee Tyl [:espadrine]]

Attachment: Way to scroll a page to center an element if the element is not visible.

Corrected the nitpickings.

Comment 26

[Thaddee Tyl [:espadrine]]

There we go.

Also, when this is committed, bug 724071 should be marked as fixed.

Comment 27

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

> let centered = !!aCentered;

Actually, we want to default to true, which requires "centered" to be true when
"aCentered" is undefined, which "!!aCentered" does not do.

ah, right right.

Comment 28

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

Comment on attachment 632395 [details] [diff] [review]
Way to scroll a page to center an element if the element is not visible.

I think this'll work.

Should have a pass through the try servers before landing to make sure that test doesn't blow up anywhere.

Comment 29

[Paul Adenot (:padenot)]

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=fe9ac340a9f6.

Comment 30

[Paul Rouget [:paul]]

Looks green enough to me.

Comment 31

[Panos Astithas (he/him) [:past] (please ni?)]

Rebased and landed:
https://hg.mozilla.org/integration/fx-team/rev/1dd1770cc77e

Comment 32

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/1dd1770cc77e

Comment 33

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/902f1d5ca14a