Closed Bug 383026 Opened 17 years ago Closed 8 years ago

scrollLeft property now returns negative values in rtl environment

Categories

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

x86
Windows XP
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: martijn.martijn, Assigned: roc)

References

(Blocks 1 open bug, )

Details

(Keywords: regression, rtl, testcase)

Attachments

(3 files, 1 obsolete file)

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
Attached file testcase
Attached patch patch (obsolete) — Splinter Review
This seems to fix it.
Did you mean to ask for review for this patch?
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?
Attachment #267485 - Flags: review?(sharparrow1)
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.
(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!
Attachment #267485 - Flags: review?(sharparrow1)
(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.
Mass-assigning the new rtl keyword to RTL-related (see bug 349193).
Keywords: rtl
Assignee: nobody → roc
Here's a refactoring which I think is a slight code improvement anyway.
Attachment #267485 - Attachment is obsolete: true
Attachment #649239 - Flags: review?(dbaron)
Comment on attachment 649239 [details] [diff] [review]
Part 1: refactoring

r=dbaron
Attachment #649239 - Flags: review?(dbaron) → review+
Whiteboard: [leave open]
(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.
Blocks: 979236
Blocks: 962249
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".)
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
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.
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).
Flags: needinfo?(xidorn+moz)
Please see comment 20
Flags: needinfo?(roc)
I don't know what that means and I don't work on this anymore. Try Xidorn.
Flags: needinfo?(roc)
Also, a new bug should be filed for a separate change -- not reopening of this bug.
(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.
A new bug bug 1447743 has been filed for a separate change
See Also: → 1447743
Flags: needinfo?(xidorn+moz)
You need to log in before you can comment on or make changes to this bug.