755971 - Scrolling does not work on reader.google.com

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[codewzrd]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0) Gecko/20100101 Firefox/13.0
Build ID: 20120501201020

Steps to reproduce:

I went to reader.google.com on my HTC EVO 4G. It is running CM7.1


Actual results:

I tried to scroll my list of feeds but nothing happens.


Expected results:

I should have been able to scroll the list from anywhere I place my finger. But the only place the scrolling appears to work is on the bar at the top where the refresh button to refresh your list and drop down button is.

Comment 1

[Aaron Train [:aaronmt]]

Dupe of bug 747493?

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Sounds more like bug 732364, which was fixed recently. What build did you see this error on?

Comment 3

[codewzrd]

This is happening on Firefox Beta 14 for Android.

Comment 4

[Adrian Tamas (:AdrianT)]

I am able to reproduce the issue on Firefox 13.0b3 build 2 on HTC Desire (Android 2.2), HTC Desire Z (Android 2.3.3), Samsung Galaxy S2 ( Android 2.3.4) and Samsung Galaxy Nexus (Android 4.0.2) and Nightly 15.0a1 2012-05-24 on HTC Desire Z. The feed subcategories list can't be scrolled(Scrolling hangs) but the feed list is scrollable. Please see the videocapture: http://youtu.be/QqOvxxzkcWk

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Weird. For some reason we're detecting a scrollable subdocument there and it's canceling the scroll. I will need to investigate further.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

This appears to be a layout issue. The individual divs on the page have a scrollHeight that is one greater than their clientHeight, so we detect them as scrollable and try to scroll them. However, their scrollTop doesn't change from zero, so we do this over and over, and it eats up all of the scroll.

On Fennec I see the divs having a scrollHeight of 35 pixels and a clientHeight of 34 pixels; on a desktop Firefox build they have a scrollHeight of 33 pixels and a clientHeight of 32 pixels.

To reproduce on desktop:
- Install the UA switcher add-on and spoof the Fennec UA string (as described at https://staktrace.com/spout/entry.php?id=745)
- Log in to Google Reader and go to a page that lists feeds (e.g. the "Staff Picks" listing)
- Examine one of the feed item divs in Firebug (these are the ones with id="tree-item-xx", where xx is a number). These are the misbehaving divs.

I ran the following in the Firebug console to verify the behaviour in the desktop browser is the same as what we're seeing in Fennec:

>>> e = document.getElementById('tree-item-13')
<div id="tree-item-13" class="row whisper sub-row unread-sub sub nested-sub" onclick="void(0)">
>>> e.scrollHeight
33
>>> e.clientHeight
32
>>> e.scrollTop
0

// since e.scrollHeight > e.clientHeight it should be able to scroll down to e.scrollTop = 1


>>> e.scrollTop=1
1
>>> e.scrollTop
0

// scrolling failed.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

CC'ing jet to reassign this to somebody in layout based on above comment.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Also unassigning from myself as per above.

Comment 9

[Jet Villegas (inactive)]

To tn for a look...

Comment 10

[Timothy Nikkel (:tnikkel)]

The site that reader.google.com is sending must be different because I am not seeing at all what comment 6 describes and I can't reproduce the bug.

Comment 11

[Kartikaya Gupta (email:[e-mail])]

I'm still able to reproduce. Did you install the UA switcher to spoof the Fennec UA string? You need to do that in order to get the mobile version of the page.

Comment 12

[Timothy Nikkel (:tnikkel)]

Yeah I installed the UA switcher. I also tried it on Fennec.

Comment 13

[Kartikaya Gupta (email:[e-mail])]

I saved a copy of the page using console.log(document.documentElement.innerHTML) and put it at http://people.mozilla.org/~kgupta/tmp/greader.html - it's not the full page but it still reproduces the problem. You can use document.getElementById("tree-item-31") as the element to test.

In case it matters, the desktop build of firefox I tested this on is from http://hg.mozilla.org/releases/mozilla-aurora/rev/74f5e0d677f2

Comment 14

[Johnathan Nightingale [:johnath]]

We think this should be fixed on tomorrow's nightly, can QA please re-test?

Comment 15

[Aaron Train [:aaronmt]]

Running a debug inbound from today, this seems WFM:

Tested via:
Samsung Galaxy Nexus (Android 4.0.4)
20120606065339
http://hg.mozilla.org/integration/mozilla-inbound/rev/7d817c805af5

Comment 16

[Kartikaya Gupta (email:[e-mail])]

Do we know what fixed this? And does the fix need uplifting?

Comment 17

[Aaron Train [:aaronmt]]

Unless I'm misunderstanding the original STR, merely scrolling through my feed on Google Reader works for me on Aurora (06/06, 15.0a2") too.

Comment 18

[Timothy Nikkel (:tnikkel)]

You have to view a list of feeds, not just a list of items that come from feeds. For me after going to reader.google.com I have to click on the "<< Feeds" button before I can reproduce. (I can still reproduce in the latest nightly.)

Comment 19

[Timothy Nikkel (:tnikkel)]

Based on the information in comment 6 I can put forth a hypothesis for what might be going on. The values returned for the element height, the scrollable height, and the scroll position are rounded to the nearest integer. So if the element height was 32 px, and the scrollable height was actually 32.6 px, scrollHeight would return 33 px. When layout is asked to scroll to 1 px it will see that it can scroll down to at most 0.6 px and it will scroll to the nearest whole pixel that is still within the scroll range, so it will choose to scroll to 0 px.

Comment 20

[Kartikaya Gupta (email:[e-mail])]

Does it make sense for the element height to be 32px if the scrollable height is actually 32.6px? I just wrote a quick test page with <div style="height: 32.6px"> and both the clientHeight and scrollHeight are 33px in that case. What other restrictions come into play when determining the clientHeight?

Comment 21

[Kartikaya Gupta (email:[e-mail])]

Also, reopening so it doesn't drop off the blocker list prematurely.

Comment 22

[Timothy Nikkel (:tnikkel)]

(In reply to Kartikaya Gupta (:kats) from comment #20)
> Does it make sense for the element height to be 32px if the scrollable
> height is actually 32.6px? I just wrote a quick test page with <div
> style="height: 32.6px"> and both the clientHeight and scrollHeight are 33px
> in that case. What other restrictions come into play when determining the
> clientHeight?

"<div style="height: 32.6px">" wouldn't show the problem because that div is not scrollable. Something like <div style="height: 32px; overflow: scroll;">(content that is 32.6px tall)</div> would be required.

The height of the element is how much space is takes on the page. The scrollHeight is the height of the content that is contains (if it is a scrollable element). Not all the content will be shown if the scrollHeight exceeds the height.

Comment 23

[Kartikaya Gupta (email:[e-mail])]

This doesn't do it either:

<div style="height: 32px; overflow-y: scroll"><div style="height: 32.6px">blah</div></div>

The scrollHeight of the outer div comes out as 32px even though the clientHeight of the inner div is 33px.

Comment 24

[Timothy Nikkel (:tnikkel)]

Ah so the in the testcase we aren't even looking at scrollable frames, so comment 19 and the replies were irrelevant. They problematic elements are display: table, so we are actually looking at table frames. The code we have in nsGenericElement for clientHeight basically returns the padding rect (ie without the border). scrollHeight basically returns the rect (which includes the border). The element has a style rule from the page that gives it a bottom border of 1px. So that explains the discrepancy. What clientHeight and scrollHeight should actually be returning I'm not sure yet.

Comment 25

[Timothy Nikkel (:tnikkel)]

Firefox and Opera include the border in scrollHeight, Chrome and IE do not.

Can you use offsetHeight instead of clientHeight? Both offsetHeight and scrollHeight in Gecko seem to include the border.

Longer term though we should look to make this match across browsers, and if we change scrollHeight to not include border we'd need to revisit this to determine another way of determining if there is something to scroll.

Comment 26

[Kartikaya Gupta (email:[e-mail])]

Changing browser.js to use offsetHeight fixes this bug, but breaks other cases where the scrollable container has a thick border. For example, this:

<div style="overflow: auto; height: 50px; border: solid black 50px"><div style="height: 80px">a<br>b<br>c</div></div>

should be scrollable (and is on desktop) but ends up not being scrollable in Fennec.

Comment 27

[Timothy Nikkel (:tnikkel)]

Good point.

So we either change the definition of scrollHeight or we find or create another method of determining when an element is scrollable.

Comment 28

[Kartikaya Gupta (email:[e-mail])]

Based on various documentation [1][2][3] I think scrollHeight should never include the border. I'll look into changing nsGenericElement::GetScrollHeight so that it doesn't include the border.

[1] http://www.w3.org/TR/cssom-view/#dom-element-scrollheight
[2] https://developer.mozilla.org/en/DOM/element.scrollHeight
[3] http://msdn.microsoft.com/en-us/library/ms534615%28VS.85%29.aspx

Comment 29

[Timothy Nikkel (:tnikkel)]

(In reply to Kartikaya Gupta (:kats) from comment #28)
> Based on various documentation [1][2][3] I think scrollHeight should never
> include the border. I'll look into changing
> nsGenericElement::GetScrollHeight so that it doesn't include the border.
> 
> [1] http://www.w3.org/TR/cssom-view/#dom-element-scrollheight

This is the only clear one.

> [2] https://developer.mozilla.org/en/DOM/element.scrollHeight

This one is vague and contradictory. There is one picture showing that scrollHeight is the padding box.

> [3] http://msdn.microsoft.com/en-us/library/ms534615%28VS.85%29.aspx

This one says "The height is the distance between the top and bottom edges of the object's content." which implies the content box, which does not include the padding, which is contrary to what every browser does including IE.

Comment 30

[Kartikaya Gupta (email:[e-mail])]

Attachment: WIP (1/3) - Add stuff to nsLayoutUtils

Uploading the WIPs I have because I'm twiddling my thumbs waiting for it to compile.

Comment 31

[Kartikaya Gupta (email:[e-mail])]

Attachment: WIP (2/3) - Overload GetOffsetRect

This might not even compile yet.

Comment 32

[Kartikaya Gupta (email:[e-mail])]

Attachment: WIP (3/3) - Point GetScrollXXX to the padding rect

Comment 33

[Timothy Nikkel (:tnikkel)]

Comment on attachment 631148 [details] [diff] [review]
WIP (2/3) - Overload GetOffsetRect

In nsGenericElement::GetOffsetRect aRect.x/y are still for the border box, not the padding box in both cases (exclude border or not).

Same thing for nsGenericHTMLElement, but it is more complicated there.

Comment 34

[Kartikaya Gupta (email:[e-mail])]

There were some minor compile errors; I fixed them up and pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=94a3e69dc99b

I also verified that these patches fix the problem on google reader. I don't know if they'll regress other things though.

Comment 35

[Kartikaya Gupta (email:[e-mail])]

Attachment: WIP (2/3) - Add GetPaddingRectSize

Rather than overload GetOffsetRect and try to get the right x/y coordinates I just added a new method to return the size of the padding rect. We don't use the x/y coordinates anyway so it would be unnecessarily complicated.

Comment 36

[Kartikaya Gupta (email:[e-mail])]

Attachment: WIP (3/3) - Update GetScrollXXX to use the padding rect

Comment 37

[Kartikaya Gupta (email:[e-mail])]

Attachment: WIP (4/3) - Some test updates

This is to fix the first batch of test failures from the try run above. There might be more fixes needed.

Comment 38

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Note to tester: it's best to use Kats's test : 
http://people.mozilla.org/~kgupta/tmp/greader.html 

to reproduce the issue.  You won't be able to easily pan down... but it is possible after several attempts.  I don't see the issue on my regular reader feeds.

Comment 39

[Kartikaya Gupta (email:[e-mail])]

Copied test page to a place I won't eventually delete it.

Comment 40

[Kartikaya Gupta (email:[e-mail])]

Latest update: still trying to figure out one test failure (browser_overflowScroll.js on Linux). Latest try run is at https://tbpl.mozilla.org/?tree=Try&rev=3d99e1fd21fe

That test was failing on all platforms, and I tried the patch at https://hg.mozilla.org/try/rev/49653a44f7be to see if that would fix it - it worked on everything except Linux. Not really sure why that patch even made a difference - the code for generating mouse events doesn't use scrollHeight as far as I can tell. Also not sure why it failed to fix the test on Linux.

The test in question opens a bunch of tabs and makes sure that the tab list scroller buttons work properly and show the right set of tabs. The scroller buttons have a 1px border on them, so presumably getting the scrollHeight/scrollWidth of those buttons would be affected by my patch, but looking through the code in toolkit/content/widget/scrollbox.xml I don't see anywhere that might happen.

Comment 41

[Kartikaya Gupta (email:[e-mail])]

Attachment: (1/4) Add stuff to nsLayoutUtils

Comment 42

[Kartikaya Gupta (email:[e-mail])]

Attachment: (2/4) Add nsGenericElement::GetPaddingRectSize

Comment 43

[Kartikaya Gupta (email:[e-mail])]

Attachment: (3/4) Update GetScrollXXX to use the padding rect

Comment 44

[Kartikaya Gupta (email:[e-mail])]

Attachment: (4/4) Updates to make tests pass

This should be folded into part 3 on landing; just keeping it separate for easier review. There's a try run going with all of these patches at https://tbpl.mozilla.org/?tree=Try&rev=15215a0092a9 - I believe they should be green since the change to tabbrowser.xml fixes the failure I was seeing earlier.

Comment 45

[Kartikaya Gupta (email:[e-mail])]

Attachment: (4/4) Updates to make tests pass (v2)

Whoops, missed a line in tabbrowser, causing some tests to still fail in the previous try run. This version fixes that; https://tbpl.mozilla.org/?tree=Try&rev=fc1f72e709f7

Comment 46

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 631920 [details] [diff] [review]
(1/4) Add stuff to nsLayoutUtils

Instead of duplicating the code, I'd prefer a macro like:

 #define BoxToRect(rectType_, sizeGet_) \
 struct BoxTo##rectType_##Rect : public nsLayoutUtils::BoxCallback { \
...
    r = nsRect(nsPoint(0, 0), aFrame->sizeGet_);

or something.  Or perhaps just set up two static inline functions called GetBorderSize() and GetPaddingSize() that take an nsIFrame* and template on the size-getter function?

In either case, I'd like the declaration of each callback class immediately before the method that uses it.

r=me with those fixed.

Comment 47

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 631921 [details] [diff] [review]
(2/4) Add nsGenericElement::GetPaddingRectSize

There's no need for this to be virtual, and I think you can just return an nsIntSize by value...  (note: nsIntSize, not nsSize).  That would incidentally fix the issue with your code in part 3 where it returns completely random numbers for elements with no frame.

>+  nsIFrame* parent = frame->GetParent() ? frame->GetParent() : frame;

Why would |frame->GetParent()| ever be null for the return value of GetStyledFrame()?  That seems pretty odd to me....  I think you should be able to assert it's non-null here.

Comment 48

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 631922 [details] [diff] [review]
(3/4) Update GetScrollXXX to use the padding rect

Are we sure this is the right thing to do?  Note that the cssom-view spec for scrollWidth and scrollHeight is completely bonkers in the cases that matter (e.g. the scrollable case), and they're more or less ignoring feedback, so I have some serious doubts about pretty much everything it says on the matter.

I guess this is what Trident and WebKit do per earlier comments in this bug, so ok.  But please do double-check that's what they do!

r=me, but note that this will need to be updated to the changes I asked for in part 2 (at which point the rcFrame temporary with it's completely incorrect name should just go away).

Comment 49

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 631989 [details] [diff] [review]
(4/4) Updates to make tests pass (v2)

The test changes look fine.  Please land them in the same changeset as part 3, though I appreciate you breaking them out for review!

The tabbrowser change needs review from someone who knows that code.

Comment 50

[Timothy Nikkel (:tnikkel)]

(In reply to Boris Zbarsky (:bz) from comment #48)
> Are we sure this is the right thing to do?

I was hoping you would have some more insight into that. :)

Comment 51

[Boris Zbarsky [:bzbarsky]]

I dunno that anyone has any insight into this mess.  ;)

Comment 52

[Kartikaya Gupta (email:[e-mail])]

Attachment: (1/4) Add stuff to nsLayoutUtils (v2)

(In reply to Boris Zbarsky (:bz) from comment #46)
> Comment on attachment 631920 [details] [diff] [review]
> (1/4) Add stuff to nsLayoutUtils
> 
> Instead of duplicating the code, I'd prefer a macro like:
> 
>  #define BoxToRect(rectType_, sizeGet_) \
>  struct BoxTo##rectType_##Rect : public nsLayoutUtils::BoxCallback { \
> ...
>     r = nsRect(nsPoint(0, 0), aFrame->sizeGet_);
> 
> or something.  Or perhaps just set up two static inline functions called
> GetBorderSize() and GetPaddingSize() that take an nsIFrame* and template on
> the size-getter function?

I decided to use static functions, and pass in a function pointer to the struct; this should prevent the duplicated object code that results from defines/templates and also leave the code relatively greppable. Let me know if you prefer something else.

> In either case, I'd like the declaration of each callback class immediately
> before the method that uses it.

I put the static functions above the methods that use them.

> r=me with those fixed.

Re-requesting review since I'm not sure if function pointer usage is discouraged in mozilla code; I didn't see too much other code using it. As I write this, I realize I could also turn the struct into a class and use a virtual function but that might be too much overhead for something this simple.

Comment 53

[Kartikaya Gupta (email:[e-mail])]

Attachment: (2/4) Add nsGenericElement::GetPaddingRectSize (v2)

(In reply to Boris Zbarsky (:bz) from comment #47)
> Comment on attachment 631921 [details] [diff] [review]
> (2/4) Add nsGenericElement::GetPaddingRectSize
> 
> There's no need for this to be virtual, and I think you can just return an
> nsIntSize by value...  (note: nsIntSize, not nsSize).

Done.

> That would
> incidentally fix the issue with your code in part 3 where it returns
> completely random numbers for elements with no frame.

Wouldn't it return 0? I though nsSize gets initialized to zero if not explicitly initialized to anything.

> >+  nsIFrame* parent = frame->GetParent() ? frame->GetParent() : frame;
> 
> Why would |frame->GetParent()| ever be null for the return value of
> GetStyledFrame()?  That seems pretty odd to me....  I think you should be
> able to assert it's non-null here.

Done. The reason I had the check was because GetOffsetRect does the exact same thing and has the check (i.e. I copy-pasted that code).

Comment 54

[Kartikaya Gupta (email:[e-mail])]

Attachment: (3/4) Update GetScrollXXX to use the padding rect (v2)

(In reply to Boris Zbarsky (:bz) from comment #48)
> Comment on attachment 631922 [details] [diff] [review]
> (3/4) Update GetScrollXXX to use the padding rect
> 
> Are we sure this is the right thing to do?  Note that the cssom-view spec
> for scrollWidth and scrollHeight is completely bonkers in the cases that
> matter (e.g. the scrollable case), and they're more or less ignoring
> feedback, so I have some serious doubts about pretty much everything it says
> on the matter.
> 
> I guess this is what Trident and WebKit do per earlier comments in this bug,
> so ok.  But please do double-check that's what they do!
> 

I'm running a test page through browsershots.org to double-check various browsers and versions. I'll update this bug with the results.

> r=me, but note that this will need to be updated to the changes I asked for
> in part 2 (at which point the rcFrame temporary with it's completely
> incorrect name should just go away).

Updated, carrying r+ since the changes aren't very significant.

Comment 55

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 632254 [details] [diff] [review]
(1/4) Add stuff to nsLayoutUtils (v2)

This is probably fine; the template would give slightly faster but larger code.... who knows.

Comment 56

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 632256 [details] [diff] [review]
(2/4) Add nsGenericElement::GetPaddingRectSize (v2)

> I though nsSize gets initialized to zero 

It sure didn't use to, but looks like that footgun got fixed.

In any case, r=me

Comment 57

[Kartikaya Gupta (email:[e-mail])]

(In reply to Kartikaya Gupta (:kats) from comment #54)
> I'm running a test page through browsershots.org to double-check various
> browsers and versions. I'll update this bug with the results.
> 

So, Gecko, Opera, and Konqueror browsers include the border. The WebKit browsers do not. Trident is a little weird, in that they do seem to include the border but also give very different results on different versions of IE. I'm also not sure if they include the border on all versions; browsershots kept timing out trying to get the results I wanted. On IE 9 it definitely includes the border.

(In reply to Timothy Nikkel (:tn) from comment #25)
> Firefox and Opera include the border in scrollHeight, Chrome and IE do not.

:tn, what version of IE did you see this on?

I also dug around on the www-style mailing list and this post was probably the clearest in explaining what should happen when the content doesn't overflow: http://lists.w3.org/Archives/Public/www-style/2010Aug/0415.html

I also think the CSSOM-View spec is fairly clear on this. I don't see why Firefox should return a different scrollHeight for a div if the only thing that changes is the addition of display:table. That is the current behaviour and it feels wrong, so I'd like to land this patch on those grounds alone.

Comment 58

[Kartikaya Gupta (email:[e-mail])]

Attachment: Test page

This is the test case I'm using

Comment 59

[Boris Zbarsky [:bzbarsky]]

> On IE 9 it definitely includes the border.

In which modes?  Might be worth testing that and hoping it matches actual old IE behavior.

> I don't see why Firefox should return a different scrollHeight for a div if the only
> thing that changes is the addition of display:table.

Well, apart from tables having different heights than divs in many cases given the same styles, right?

Comment 60

[Boris Zbarsky [:bzbarsky]]

roc, thoughts?

Comment 61

[Kartikaya Gupta (email:[e-mail])]

(In reply to Boris Zbarsky (:bz) from comment #59)
> In which modes?  Might be worth testing that and hoping it matches actual
> old IE behavior.

In IE9 I tried quirks mode, IE7 standards mode, IE8 standards mode, and IE9 standards mode. Of these only IE7 standards mode didn't include the border.

> Well, apart from tables having different heights than divs in many cases
> given the same styles, right?

Right. But in this case there's no height change, and the behaviour changes from not including the border to including the border.

Comment 62

[Boris Zbarsky [:bzbarsky]]

> Of these only IE7 standards mode didn't include the border.

Sounds like everyone but WebKit includes the border and the spec is just bogus, to me, in that case.

> and the behaviour changes from not including the border to including the border.

Er... where?  I must have missed that earlier in the bug.

Comment 63

[Kartikaya Gupta (email:[e-mail])]

(In reply to Boris Zbarsky (:bz) from comment #62)
> Sounds like everyone but WebKit includes the border and the spec is just
> bogus, to me, in that case.

Yeah, kinda.

> Er... where?  I must have missed that earlier in the bug.

Consider four cases:

<div style="border-bottom: solid 5px black; overflow: auto; height: 20px"></div>
This has scrollHeight 20px

<div style="border-bottom: solid 10px black; overflow: auto; height: 20px"></div>
Increase the border, scrollHeight is still 20px

<div style="display: table; border-bottom: solid 5px black; overflow: auto; height: 20px"></div>
Switch to display: table, scrollHeight now includes border and is 25px

<div style="display: table; border-bottom: solid 10px black; overflow: auto; height: 20px"></div>
Increase the border, scrollHeight goes up too, now 30px

Comment 64

[Boris Zbarsky [:bzbarsky]]

> Consider four cases:

The "auto" value of the "overflow" property does not apply to display:table boxes, per spec, so your table testcases should have the same behavior as if "overflow" were set to "visible".

Comment 65

[Kartikaya Gupta (email:[e-mail])]

Hm, I didn't know that. In that case it seems like:

- the cssom-view spec for scrollWidth/scrollHeight should be updated so that step 4 reads as follows:
  4. If the first CSS layout box associated with the element is a block container box (as defined in CSS2.1) return the computed value of the 'padding-left' property, plus the computed value of the 'padding-right', plus the content width of the element. Otherwise, return the sum of the computed values of the 'border-left-width', 'padding-left', 'padding-right', and 'border-right-width' properties, plus the content width of the element.

- webkit should be updated to match the spec as above

- we need to find another fix for the original bug here. the fix most likely to be correct seems like updating the browser.js code to take into account the computed display property and making sure it is 'block' or 'inline-block' along with comparing the scrollHeight to the clientHeight.

Comment 66

[Boris Zbarsky [:bzbarsky]]

> - the cssom-view spec for scrollWidth/scrollHeight should be updated

It should, but your proposed change isn't quite right either.  I sent mail earlier today about the issues with the spec; see http://lists.w3.org/Archives/Public/www-style/2012Jun/0247.html

I think the right way to fix the browser.js code is to stop trying to guesstimate whether the element can scroll and just expose an API for finding out that information, because the underlying C++ code most certainly knows whether there's a scrollframe and if so whether it can scroll.  Probably put the API on windowutils unless we think it would be generally useful on the web (in which case we should propose it for standardization).

Comment 67

[j.j.]

Needs better documentation (see comment 29)

Comment 68

[Kartikaya Gupta (email:[e-mail])]

(In reply to Boris Zbarsky (:bz) from comment #66)
> I think the right way to fix the browser.js code is to stop trying to
> guesstimate whether the element can scroll and just expose an API for
> finding out that information, because the underlying C++ code most certainly
> knows whether there's a scrollframe and if so whether it can scroll. 
> Probably put the API on windowutils unless we think it would be generally
> useful on the web (in which case we should propose it for standardization).

Fair enough. I do think this API would be generally useful on the web; there are a lot of stackoverflow posts that are about detecting overflow (e.g. [1][2]), and they all have the same (wrong) implementation that we put in browser.js.

[1] http://stackoverflow.com/questions/2059743/detect-elements-overflow-using-jquery
[2] http://stackoverflow.com/questions/4369990/scrollheight-property-in-firefox

Comment 69

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 631989 [details] [diff] [review]
(4/4) Updates to make tests pass (v2)

r=me on the tabbrowser.xml changes

Comment 70

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

(In reply to Boris Zbarsky (:bz) from comment #48)
> Are we sure this is the right thing to do?  Note that the cssom-view spec
> for scrollWidth and scrollHeight is completely bonkers in the cases that
> matter (e.g. the scrollable case),

You mean because most browsers include the border but the spec doesn't?

> and they're more or less ignoring
> feedback, so I have some serious doubts about pretty much everything it says
> on the matter.

I think they're only ignoring feedback because there is no editor. I don't think anyone's actively ignoring feedback :-).

I think we should make scrollWidth and scrollHeight use the padding-edge always. For scrollable elements we ignore the border, which makes sense because the "height/width of the scrollable area" doesn't include the border (it doesn't scroll). So I think we should ignore the border for non-scrollable elements too. Then scrollHeight - clientHeight usually* gives you the maximum value for scrollTop: 0 for non-scrollable elements, which is a useful invariant. Only 
"usually", because of rounding: if the true clientHeight is 1.4 and true scrollHeight is 1.6, then scrollHeight - clientHeight gives a maximum value for scrollTop of 2 but the true maximum is still 0.

So I think that's a reasonable change to make, but we still need an API to compute the correct min/max for scrollLeft/scrollTop. We have window.scrollMaxX/Y, which we may or may not consider a precedent.

In the RTL case we allow a negative scrollLeft. Webkit doesn't seem to, scrollLeft=0 is always the leftmost scrollable position in Webkit. This has the slightly unpleasant effect that changes to the content that don't cause a visible scroll but change the scrollable amount can cause a change in scrollLeft, but it has the nice effect that the minimum value is always 0 for scrollLeft/scrollTop. This is very nice because I bet most Web devs have never even considered it might not be 0 and won't code for it no matter what we tell them o do. So I think we should copy the Webkit behavior.

Then we only need a max API. I could go with element.scrollLeftMax/scrollTopMax or element.scrollMaxX/scrollMaxY.

All of this would be easy to implement.

Comment 71

[Kartikaya Gupta (email:[e-mail])]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #70)
> Only 
> "usually", because of rounding: if the true clientHeight is 1.4 and true
> scrollHeight is 1.6, then scrollHeight - clientHeight gives a maximum value
> for scrollTop of 2 but the true maximum is still 0.

Are you able to construct a test case that demonstrates this? I tried before and wasn't able to do it; the scrollHeight was coming back as 1 as well.

> In the RTL case we allow a negative scrollLeft. Webkit doesn't seem to,
> scrollLeft=0 is always the leftmost scrollable position in Webkit. This has
> the slightly unpleasant effect that changes to the content that don't cause
> a visible scroll but change the scrollable amount can cause a change in
> scrollLeft, but it has the nice effect that the minimum value is always 0
> for scrollLeft/scrollTop. This is very nice because I bet most Web devs have
> never even considered it might not be 0 and won't code for it no matter what
> we tell them o do. So I think we should copy the Webkit behavior.

I agree here, this behaviour in Gecko was very unexpected to me as well. The Fennec front-end originally assumed that 0,0 was always the top-left corner of the page, and the patches on bug 748384 were needed to correct that mess.

> Then we only need a max API. I could go with
> element.scrollLeftMax/scrollTopMax or element.scrollMaxX/scrollMaxY.

This assumes the rounding problem above, right?

Comment 72

[Boris Zbarsky [:bzbarsky]]

> You mean because most browsers include the border but the spec doesn't?

No, because in the scrollable case the content overlaps the paddings afaict but the spec says to add up content width and padding.

> I think they're only ignoring feedback because there is no editor.

http://lists.w3.org/Archives/Public/www-style/2012Feb/0573.html says otherwise. Glenn Adams tried really hard to get that job afaict... and then is lying down on it.  But whatever.

For the rest, sounds ok.  We should mail the list just to let them know what we plan to do.  Not like it'll matter for spec purposes....

Comment 73

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

(In reply to Boris Zbarsky (:bz) from comment #72)
> > You mean because most browsers include the border but the spec doesn't?
> 
> No, because in the scrollable case the content overlaps the paddings afaict
> but the spec says to add up content width and padding.

OK, yes, in general the content we measure for scrollHeight does overlap the bottom padding, so the spec is broken there. However, the border is definitely excluded.

> > I think they're only ignoring feedback because there is no editor.
> 
> http://lists.w3.org/Archives/Public/www-style/2012Feb/0573.html says
> otherwise. Glenn Adams tried really hard to get that job afaict... and then
> is lying down on it.  But whatever.

Ah, right. Dunno about Glenn Adams, but Shane Stephens should be able to handle this feedback. I'll get onto him.

Comment 74

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

Attachment: testcase

> Are you able to construct a test case that demonstrates this?

Yes :-)

Comment 75

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

So I think we should land the patches here.

In a separate bug we should add scrollMaxX/scrollMaxY (moz-prefixed?) and advocate them on www-style.

And then in another bug we should fix scrollLeft to handle the RTL case. That shouldn't be hard actually.

Kartikaya, how much of that are you up for? :-)

Comment 76

[Kartikaya Gupta (email:[e-mail])]

I can land these patches and throw up a patch for scrollMaxX/scrollMaxY, but doing the RTL changes is probably more complicated than I'd like to get into.

Comment 77

[Kartikaya Gupta (email:[e-mail])]

Landed on inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/4dd0152f1be7
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae7de32fb83a
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d64d1543738 (this contains the parts 3 and 4 folded together).

Also filed bug 766937 for the second part of comment 75 (adding scrollMaxX/Y).

Comment 79

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/4dd0152f1be7
https://hg.mozilla.org/mozilla-central/rev/ae7de32fb83a
https://hg.mozilla.org/mozilla-central/rev/3d64d1543738

Comment 80

[Kartikaya Gupta (email:[e-mail])]

Since this changes core gecko behaviour I don't really want it uplifted; marking wontfix for 14 and 15 so that it rides the trains as usual.