383026 - scrollLeft property now returns negative values in rtl environment

Status: RESOLVED FIXED

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

Description

[Martijn Wargers (dead)]

The testcase is from bug 336736. It was discovered in bug 336736 that the .scrollLeft property returns negative values in rtl environments.
That's not what IE is doing, so I don't think that Mozilla should be doing that, either.

See also:
http://developer.mozilla.org/en/docs/DOM:element.scrollLeft

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase

Comment 2

[Martijn Wargers (dead)]

Attachment: patch

This seems to fix it.

Comment 3

[Eli Friedman]

Did you mean to ask for review for this patch?

Comment 4

[Martijn Wargers (dead)]

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

(In reply to comment #3)
> Did you mean to ask for review for this patch?

Sorry for the delay.
Do you think the patch is any good?

Comment 5

[Eli Friedman]

Hmm, the math in this patch doesn't look right.

In GetScrollLeft, you're returning a number for scrollLeft that isn't dependent on the actual scroll position if the scroll position is negative.  That doesn't seem likely to be correct.

In SetScrollLeft, what exactly is "aScrollLeft - scrolledRect.width/2" supposed to calculate?  And will that change affect ltr situations where xpos == 0?

Also, if we are going to take a patch like this, I'd like to see some tests so that we don't regress it.

Comment 6

[Martijn Wargers (dead)]

(In reply to comment #5)
> In GetScrollLeft, you're returning a number for scrollLeft that isn't dependent
> on the actual scroll position if the scroll position is negative.  That doesn't
> seem likely to be correct.

It seems to work just fine with my tests. scrolledRect.XMost() changes when the scroll position is changed, no?

> In SetScrollLeft, what exactly is "aScrollLeft - scrolledRect.width/2" supposed
> to calculate?  And will that change affect ltr situations where xpos == 0?

I don't know. I see now that it doesn't work in rtl situations where .scrollLeft is set to 0.

> Also, if we are going to take a patch like this, I'd like to see some tests so
> that we don't regress it.

I know.

The whole patch was more or less made by trial and error. If you want to take it over, please!

Comment 7

[Eli Friedman]

(In reply to comment #6)
> (In reply to comment #5)
> It seems to work just fine with my tests. scrolledRect.XMost() changes when the
> scroll position is changed, no?

Oh, right.

> The whole patch was more or less made by trial and error. If you want to take
> it over, please!

I'll consider it.

Comment 8

[(no longer active)]

Mass-assigning the new rtl keyword to RTL-related (see bug 349193).

Comment 9

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

Browsers are currently quite inconsistent about this:
http://lists.w3.org/Archives/Public/www-style/2012Aug/0156.html

Comment 10

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

Attachment: Part 1: refactoring

Here's a refactoring which I think is a slight code improvement anyway.

Comment 11

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

Attachment: Part 2: make DOM scroll positions be relative to the top-left of the scroll range

If we decide to do this.

Comment 12

[David Baron :dbaron:]

Comment on attachment 649239 [details] [diff] [review]
Part 1: refactoring

r=dbaron

Comment 13

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

Landed part 1: https://hg.mozilla.org/integration/mozilla-inbound/rev/df597d36bcbb

Comment 14

[Ed Morley [:emorley]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #13)
> Landed part 1:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/df597d36bcbb

Push backed out for failures in meter-native-style.html:
https://tbpl.mozilla.org/php/getParsedLog.php?id=14292529&tree=Mozilla-Inbound

Couldn't see you in #developers and didn't seem obvious which changeset caused it, so had to back all three out.

Given this failure and the bustage on the initial landing that required a fixup, please can you consider https://groups.google.com/d/topic/mozilla.dev.platform/nrbJE1ixkIs/discussion next time using inbound.

Comment 15

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/2d6fd808c2b0

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2d6fd808c2b0

Comment 17

[Ed Morley [:emorley]]

One of the bugs in this push caused bug 782196:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=b753e1dce89f&tochange=94e4dbce3b94

Comment 18

[Xidorn Quan [:xidorn] UTC+11]

After reading the CSSOM View spec detailedly, I believe our current behavior matches what the spec says. We consistently return negative values in rtl and vertical-rl mode in all cases. WebKit/Blink and Edge both have inconsistency themselves.

See result in https://people.mozilla.org/~xquan/scrollleft.html

Given this bug has several patches landed, let me close it as FIXED. (Note that the issue itself described here is actually "INVALID".)

Comment 19

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

Look for threads in www-style titled "[cssom-view] value of scrollLeft in RTL situations is completely busted across browsers" as to how we got here. There was a consensus that our behavior is preferred, though I don't know if other browsers have aligned on that or not.

Comment 20

[張俊芝(ZHANG Junzhi)]

This bug should be reopened.

I believe the newest CSSOM View Module(https://drafts.csswg.org/cssom-view/#dom-element-scrollleft) has now given a clear definition of the origin of |scrollLeft| for an element.

Here are the related statements:

Return the x-coordinate of the scrolling area at the alignment point with the left of the padding edge of the element.

Changes From 17 December 2013

The origin of scrollLeft on Element was changed (for RTL).

Comment 21

[張俊芝(ZHANG Junzhi)]

Please see comment 20

Comment 22

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

I don't know what that means and I don't work on this anymore. Try Xidorn.

Comment 23

[David Baron :dbaron:]

Also, a new bug should be filed for a separate change -- not reopening of this bug.

Comment 24

[張俊芝(ZHANG Junzhi)]

(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #23)
> Also, a new bug should be filed for a separate change -- not reopening of
> this bug.

Yes, I'll file a new one. I notice that this bug already has a push-and-backed-out history.

Comment 25

[張俊芝(ZHANG Junzhi)]

A new bug bug 1447743 has been filed for a separate change