332246 - scrollIntoView(false) doesn't work correctly for inline elements that wrap at multiple lines

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Martijn Wargers (dead)]

See upcoming testcase.
Clicking on the 'Show bottom' button doesn't work correctly in Mozilla.
It only scrolls the first line into view (from the bottom).
It should show the last line into view from the bottom, like IE6 is doing.

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase

Comment 2

[Martijn Wargers (dead)]

Attachment: patch

This makes it work for me.

Comment 3

[Johnny Stenback (:jst)]

Comment on attachment 216762 [details] [diff] [review]
patch

Looks good to me, but bz or dbaron should sr.

Comment 4

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Would a fix for bug 66619 also fix this?

Comment 5

[Martijn Wargers (dead)]

(In reply to comment #4)
> Would a fix for bug 66619 also fix this?
Yes, I think so. Not sure what the patch in that bug is also trying to fix, though.

Comment 6

[Martijn Wargers (dead)]

Attachment: testcase2

Comment 7

[Martijn Wargers (dead)]

The first value in testcase2 returns 101 in current trunk builds, that shouldn't happen, so I filed bug 401904 for it.

Comment 8

[Martijn Wargers (dead)]

Attachment: patch

This is now enough to make it work correctly.

Comment 9

[Martijn Wargers (dead)]

Attachment: mochitest

Comment 10

[Erik Arvidsson]

I see this happening on Mac OS X 10.4 as well. To me it does not matter if it is an inline or block element or whether the element text wraps or not. I can reproduce the bug with any element.

Comment 11

[Mike Schroepfer]

This doesn't appear to be a regression from 1.8 - but would take the patch - so marking - for blocking but if you get reviews on patch ask for patch approval

Comment 12

[Martijn Wargers (dead)]

Comment on attachment 286845 [details] [diff] [review]
patch

Robert, perhaps you could review?

Comment 13

[Martijn Wargers (dead)]

Robert, you forgot to review (or take a quick look at) the mochitest.

Comment 14

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

Comment on attachment 286852 [details] [diff] [review]
mochitest

I have very little enthusiasm for reviewing tests :-)

Comment 15

[Martijn Wargers (dead)]

Checking in nsGenericHTMLElement.cpp;
/cvsroot/mozilla/content/html/content/src/nsGenericHTMLElement.cpp,v  <--  nsGen
ericHTMLElement.cpp
new revision: 1.745; previous revision: 1.744
done

Checked into trunk.

Checking in Makefile.in;
/cvsroot/mozilla/content/html/content/test/Makefile.in,v  <--  Makefile.in
new revision: 1.35; previous revision: 1.34
done
RCS file: /cvsroot/mozilla/content/html/content/test/test_bug332246.html,v
done
Checking in test_bug332246.html;
/cvsroot/mozilla/content/html/content/test/test_bug332246.html,v  <--  test_bug3
32246.html
initial revision: 1.1
done

Mochitest checked in.

Comment 16

[Martijn Wargers (dead)]

MacOSX Darwin 8.8.4 qm-xserve01 dep unit test was failing with the mochitest, so I removed this test from the Makefile.in file for now.

Comment 17

[Martijn Wargers (dead)]

Apparently, it also fails on Linux qm-centos5-01 dep unit test.

Comment 18

[Martijn Wargers (dead)]

Ok, it turns out that Linux qm-centos5-01 dep unit test is failing because it doesn't suffer from bug 401904, so it's failing on the TODO item!

Mac is failing for the first 2 tests, it returns 102 for both of them. I filed bug 411297 for that.

So I guess I could alter the Mochitest to do platform specific test to take all of that into account.

Comment 19

[Martijn Wargers (dead)]

Ok, I added some platform specific checks now in the mochitest, like this:
//bug 401904 for windows, bug 411297 for the Mac
if (navigator.platform == "Win32" || navigator.platform.indexOf("Mac") == 0)
  todo(a1.scrollTop == 100, "Wrong scrollTop value!");
else
  ok(a1.scrollTop == 100, "Wrong scrollTop value!");
a2.scrollIntoView(false);
if (navigator.platform.indexOf("Mac") == 0) //Mac bug 411297 again
  todo((a1.scrollHeight - a1.offsetHeight - a1.scrollTop) == 100, "Wrong (scrollHeight - offsetHeight - scrollTop) value!");
else
  ok((a1.scrollHeight - a1.offsetHeight - a1.scrollTop) == 100, "Wrong (scrollHeight - offsetHeight - scrollTop) value!");

It's ugly, but it should work.
The platform specific checks can be removed as soon as the corresponding bugs would be removed.

Comment 20

[Brad Jackson]

Since this checkin, on my nightly CVS trunk builds, forums such as the example below now jump to the top of the page when clicking the Prev or Next links with the page scrolled down. In previous nightlies, the scoll position remained where it was when clicking the links.  This is a major nuisance, although I didn't look at the JavaScript being run to see if it was working correctly or incorrectly before.

http://forums.roadbikereview.com/showthread.php?t=116961

Comment 21

[Martijn Wargers (dead)]

Brad, thanks for mentioning this, I filed bug 411665 for it.
Note that IE and Opera do the same, so I actually think that Mozilla is doing the correct thing now (although it looks ugly). But maybe the ugly flash of scrolling can be reduced in some other way.
The most simple thing off course would be if the site in question would fix their broken script.