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

RESOLVED FIXED in Firefox 16

Status

()

Firefox
Developer Tools
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: paul, Assigned: espadrine)

Tracking

Trunk
Firefox 16
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 7 obsolete attachments)

(Reporter)

Description

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

Comment 1

5 years ago
We could use that: https://gist.github.com/2581101
(Reporter)

Updated

5 years ago
Blocks: 754163
(Reporter)

Updated

5 years ago
Component: DOM → Developer Tools
Product: Core → Firefox
QA Contact: general → developer.tools
(Reporter)

Updated

5 years ago
Blocks: 754659

Comment 2

5 years ago
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?
(Reporter)

Comment 3

5 years ago
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
(Assignee)

Comment 4

5 years ago
Created attachment 628033 [details] [diff] [review]
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>.
Assignee: nobody → thaddee.tyl
Attachment #628033 - Flags: review?(rcampbell)
Attachment #628033 - Flags: review?(paul)
(Reporter)

Comment 5

5 years ago
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.
Attachment #628033 - Flags: review?(paul)
(Assignee)

Comment 6

5 years ago
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?
tests live in:

browser/devtools/highlighter/test

Comment 8

5 years ago
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 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!)
Attachment #628033 - Flags: review?(rcampbell)
(Assignee)

Comment 10

5 years ago
(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

5 years ago
Thaddée, do you handle nested scrolling areas?
(Assignee)

Comment 12

5 years ago
hsablonniere: in this patch? I think so.
Can you test that?
(Assignee)

Comment 13

5 years ago
Created attachment 628253 [details] [diff] [review]
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.
Attachment #628033 - Attachment is obsolete: true
Attachment #628253 - Flags: review?(rcampbell)
(Assignee)

Comment 14

5 years ago
Created attachment 628392 [details] [diff] [review]
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.
Attachment #628253 - Attachment is obsolete: true
Attachment #628253 - Flags: review?(rcampbell)
Attachment #628392 - Flags: review?(rcampbell)
(Assignee)

Updated

5 years ago
Attachment #628392 - Flags: review?(rcampbell)
(Assignee)

Comment 15

5 years ago
Created attachment 628397 [details] [diff] [review]
Way to scroll a page to center an element if the element is not visible.
Attachment #628392 - Attachment is obsolete: true
Attachment #628397 - Flags: review?(rcampbell)
(Assignee)

Updated

5 years ago
Attachment #628397 - Flags: review?(rcampbell)
(Assignee)

Comment 16

5 years ago
Created attachment 628398 [details] [diff] [review]
Way to scroll a page to center an element if the element is not visible.
Attachment #628397 - Attachment is obsolete: true
Attachment #628398 - Flags: review?(rcampbell)
(Assignee)

Updated

5 years ago
Attachment #628398 - Flags: review?(dcamp)

Comment 17

5 years ago
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.
Attachment #628398 - Flags: review?(dcamp)
(Assignee)

Comment 18

5 years ago
> ::: 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.
(Assignee)

Comment 19

5 years ago
Created attachment 630032 [details] [diff] [review]
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.
Attachment #628398 - Attachment is obsolete: true
Attachment #628398 - Flags: review?(rcampbell)
Attachment #630032 - Flags: review?(rcampbell)
Attachment #630032 - Flags: review?(dcamp)
(Reporter)

Comment 20

5 years ago
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).
(Assignee)

Comment 21

5 years ago
(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?
(Assignee)

Comment 22

5 years ago
Created attachment 630398 [details] [diff] [review]
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.
Attachment #630032 - Attachment is obsolete: true
Attachment #630032 - Flags: review?(rcampbell)
Attachment #630032 - Flags: review?(dcamp)
Attachment #630398 - Flags: review?(rcampbell)
Attachment #630398 - Flags: review?(dcamp)
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.
Attachment #630398 - Flags: review?(rcampbell) → review+
(Assignee)

Comment 24

5 years ago
(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.
(Assignee)

Comment 25

5 years ago
Created attachment 632395 [details] [diff] [review]
Way to scroll a page to center an element if the element is not visible.

Corrected the nitpickings.
Attachment #630398 - Attachment is obsolete: true
Attachment #630398 - Flags: review?(dcamp)
Attachment #632395 - Flags: review?(rcampbell)
(Assignee)

Comment 26

5 years ago
There we go.

Also, when this is committed, bug 724071 should be marked as fixed.
> 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.
Status: NEW → ASSIGNED
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.
Attachment #632395 - Flags: review?(rcampbell) → review+
Whiteboard: [needs-try]
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=fe9ac340a9f6.
Whiteboard: [needs-try]
(Reporter)

Comment 30

5 years ago
Looks green enough to me.
(Reporter)

Updated

5 years ago
Whiteboard: [land-in-fx-team]
Rebased and landed:
https://hg.mozilla.org/integration/fx-team/rev/1dd1770cc77e
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/1dd1770cc77e
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 16
Whiteboard: [fixed-in-fx-team] → b
Whiteboard: b
https://hg.mozilla.org/mozilla-central/rev/902f1d5ca14a
You need to log in before you can comment on or make changes to this bug.