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)
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)
1.78 KB,
text/html
|
Details | |
10.04 KB,
patch
|
dbaron
:
review+
roc
:
checkin+
|
Details | Diff | Splinter Review |
3.26 KB,
patch
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 1•17 years ago
|
||
Reporter | ||
Comment 2•17 years ago
|
||
This seems to fix it.
Comment 3•17 years ago
|
||
Did you mean to ask for review for this patch?
Reporter | ||
Comment 4•17 years ago
|
||
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)
Comment 5•17 years ago
|
||
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.
Reporter | ||
Comment 6•17 years ago
|
||
(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!
Reporter | ||
Updated•17 years ago
|
Attachment #267485 -
Flags: review?(sharparrow1)
Comment 7•17 years ago
|
||
(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•16 years ago
|
||
Mass-assigning the new rtl keyword to RTL-related (see bug 349193).
Keywords: rtl
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → roc
Assignee | ||
Comment 9•12 years ago
|
||
Browsers are currently quite inconsistent about this: http://lists.w3.org/Archives/Public/www-style/2012Aug/0156.html
Assignee | ||
Comment 10•12 years ago
|
||
Here's a refactoring which I think is a slight code improvement anyway.
Attachment #267485 -
Attachment is obsolete: true
Attachment #649239 -
Flags: review?(dbaron)
Assignee | ||
Comment 11•12 years ago
|
||
If we decide to do this.
Comment 12•12 years ago
|
||
Comment on attachment 649239 [details] [diff] [review] Part 1: refactoring r=dbaron
Attachment #649239 -
Flags: review?(dbaron) → review+
Assignee | ||
Comment 13•12 years ago
|
||
Landed part 1: https://hg.mozilla.org/integration/mozilla-inbound/rev/df597d36bcbb
Assignee | ||
Updated•12 years ago
|
Whiteboard: [leave open]
Assignee | ||
Updated•12 years ago
|
Attachment #649239 -
Flags: checkin+
Comment 14•12 years ago
|
||
(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.
Assignee | ||
Comment 15•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d6fd808c2b0
Comment 17•12 years ago
|
||
One of the bugs in this push caused bug 782196: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=b753e1dce89f&tochange=94e4dbce3b94
Comment 18•8 years ago
|
||
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]
Assignee | ||
Comment 19•8 years ago
|
||
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•6 years ago
|
||
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)
Assignee | ||
Comment 22•6 years ago
|
||
I don't know what that means and I don't work on this anymore. Try Xidorn.
Flags: needinfo?(roc)
Comment 23•6 years ago
|
||
Also, a new bug should be filed for a separate change -- not reopening of this bug.
Comment 24•6 years ago
|
||
(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•6 years ago
|
||
A new bug bug 1447743 has been filed for a separate change
Updated•6 years ago
|
Flags: needinfo?(xidorn+moz)
You need to log in
before you can comment on or make changes to this bug.
Description
•