483552 - ensureElementIsVisible on scrollBoxObject doesn't take border into account

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect)

Description

[Nochum Sossonko [:Natch]]

Calling foo.scrollBoxObject.ensureElementIsVisible may still leave the borders out of the visible area. See bug 475203.

Comment 1

[Nochum Sossonko [:Natch]]

To be more precise, it seems that the scrollBoxObject itself is 2 pixels off to
the left, when testing a scroll-up the tab to the left has 2 pixels still
hidden, and when testing a scroll-down the tab to the right is scrolled 2
pixels too much.

Comment 2

[Nochum Sossonko [:Natch]]

Gonna try working up a testcase...

Comment 3

[Nochum Sossonko [:Natch]]

Attachment: testcase

Here's a testcase showing the off by 2 thing with a bunch of hbox'es in an arrowscrollbox, needs priveleges to get to the native anon stuff.

Comment 4

[Nochum Sossonko [:Natch]]

Comment on attachment 367536 [details]
testcase

Scratch that, it doesn't actually need privileges.
:)

Comment 5

[Nochum Sossonko [:Natch]]

Requesting blocking, because this makes it impossible to scroll the leftmost tab all the way (both when clicking on the tab itself and when using the mousewheel). This bug is only there when smoothScroll is set to false. The only way to scroll it all the way is by clicking on the left arrow button.

Comment 6

[Nochum Sossonko [:Natch]]

Is it generally bad that I can QueryInterface on a scrollbox to its' scrollBoxObject without any permission escalation? Or is QueryInterface harmless?

Comment 7

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

This is not a regression, right? It's always been broken?

Comment 8

[Nochum Sossonko [:Natch]]

(In reply to comment #7)
> This is not a regression, right? It's always been broken?

Right. At least since 3.0.

Comment 9

[Dão Gottwald [:dao]]

Attachment: testcase 2

Comment 10

[Dão Gottwald [:dao]]

Attachment: drop scrollBoxObject, toolkit part

Comment 11

[Dão Gottwald [:dao]]

Attachment: drop scrollBoxObject, browser part

Comment 12

[Dão Gottwald [:dao]]

This fixes bug 485938 and bug 485841.

Comment 13

[Dão Gottwald [:dao]]

Attachment: drop scrollBoxObject, browser part v2

the previous patch removed a code block that shouldn't have been removed (but updated to not use the scrollBoxObject)

Comment 14

[Neil Deakin]

 
-      <field name="_scrollBoxObject">null</field>
-      <property name="scrollBoxObject" readonly="true">

Don't remove this. It's listed in the documentation.

Also, removing this caused you to change the tests to use internal properties which shouldn't be done if it isn't necessary.

+      <property name="visibleSize" readonly="true">
+        <getter><![CDATA[
+          return this.getAttribute("orient") == "horizontal" ?
+                 this._scrollbox.clientWidth :
+                 this._scrollbox.clientHeight;

The default orientation is horizontal though so if orient isn't specified this will be incorrect. Same with the other properties.
Also, this.orient should be ok here.

I'm not sure I like the idea of adding more properties, but I'm not as concerned as it only affects the arrowscrollbox which is fairly specialized. I assume you tested large menus too?
We should at least give the new properties names that correspond more with the source names.

scrollRect -> scrollClientRect or something.

Comment 15

[Dão Gottwald [:dao]]

Attachment: toolkit part, v2

I've tested this with the bookmarks menu.

Comment 16

[Dão Gottwald [:dao]]

Attachment: browser part, v3

s/scrollRect/scrollClientRect/

Comment 17

[Neil Deakin]

Comment on attachment 370448 [details] [diff] [review]
toolkit part, v2

I'd actually meant to change all the properties, so that it was clearer how they related to what they retrieved:

visibleSize -> scrollClientSize
scrolledSized -> scrollSize

I'm not too concerned about the first, but I think scrollSize should be used to mirror scrollPosition (rather that scrolled with an 'ed')

Comment 18

[Dão Gottwald [:dao]]

Attachment: toolkit part, v3 (checked in)

changed the property names as suggested

Comment 19

[Dão Gottwald [:dao]]

Attachment: browser part, v4

Comment 20

[Dão Gottwald [:dao]]

Comment on attachment 371723 [details] [diff] [review]
toolkit part, v3 (checked in)

http://hg.mozilla.org/mozilla-central/rev/510bd328a29d

Comment 21

[Boris Zbarsky [:bzbarsky]]

Backed out of m-c; the tree has unexplained leak orange.

Comment 22

[Dão Gottwald [:dao]]

Comment on attachment 371723 [details] [diff] [review]
toolkit part, v3 (checked in)

http://hg.mozilla.org/mozilla-central/rev/8d65c4e868ef

Comment 23

[Peter van der Woude [:Peter6]]

could this have caused Bug 487946 ?

Comment 24

[Dão Gottwald [:dao]]

Comment on attachment 371725 [details] [diff] [review]
browser part, v4

moving this to bug 485841