Closed Bug 962249 Opened 10 years ago Closed 9 years ago

Horizontal autoscroll missing on right-to-left (RTL/Hebrew/Arabic) page that doesn't fit in the screen

Categories

(Toolkit :: UI Widgets, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox40 --- wontfix
firefox41 --- wontfix
firefox42 --- wontfix
firefox43 --- wontfix
firefox44 + fixed
firefox-esr38 --- wontfix

People

(Reporter: tomer, Assigned: xidorn)

References

()

Details

(Keywords: regression, rtl)

Attachments

(5 files, 2 obsolete files)

1.15 KB, text/html
Details
40 bytes, text/x-review-board-request
roc
: review+
bzbarsky
: review+
Details
40 bytes, text/x-review-board-request
roc
: review+
Details
40 bytes, text/x-review-board-request
roc
: review+
bzbarsky
: review+
Details
1023 bytes, patch
roc
: review+
Details | Diff | Splinter Review
Attached file testcase
A user on our community forum inform us[1] about a missing autoscroll support when scrolling inside an RTL block element. 

This sounds to me very similar to an issue we had in the past, and I guess that the patch on bug 192767 could be used here as well.

[1] http://mozilla.org.il/board/viewtopic.php?f=9&t=11907


Steps to reproduce:
1. Visit the attached testcase.
2. For each block element click the middle mouse button, and try to scroll horizontally by moving the mouse cursor to the sides. 

Current result:
1. On the LTR block element: When clicking the middle mouse button, there is an horizontal autoscroll icon, and it is possible to auto scroll by moving the mouse.
2. On the RTL block element: Nothing happens.
autoscroll is done in toolkit.
Component: Layout: Text → XUL Widgets
Product: Core → Toolkit
I believe that this is a problem with the scrollLeftMax property, which currently returns GetScrollFrame()->GetScrollRange().XMost(). This is a problem on RTL elements, where the scroll range goes from negative x to zero.

If it's correct that the start side of the scroll range is always zero, i.e. scrollRange.x == 0 in LTR elements and scrollRange.XMost() == 0 in RTL elements, then I think changing scrollLeftMax to return GetScrollFrame()->GetScrollRange().width should fix RTL without regressing LTR. Or we could fix bug 383026.

Clearly we also need more testcases.

https://tbpl.mozilla.org/?tree=Try&rev=c5a792b83137
Blocks: 766937
scrollLeftMax is always 0 in RTL. So it's not very useful there. Perhaps we should introduce scrollLeftMin as well.

There's an open issue about how to make scrollLeft work with RTL across browsers. We had a discussion in www-style last year with subject "value of scrollLeft in RTL situations is completely busted across browsers". If we adopt the CSSOM spec approach, then we wouldn't need scrollLeftMin because scrollLeft=0 is always the leftmost scroll position and scrollLeftMax would scroll you all the way to the right.
Depends on: 383026
Blocks: 1200137
Now, this bug also affected to CSS writing-mode: vertical-rl; etc. (See Bug1200137)
[Tracking Requested - why for this release]: Regression ux since Firefox26

Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=749739c77f73&tochange=e621399eb90f

Regressed by:
71b3fa2eb87d	Arpad Borsos — Bug 914251 - Autoscroll does not work properly in certain case; r=neil@parkwaycc.co.uk
The autoscroll code doesn't care what the correct value of scrollLeftMax is, just as long as it's non-zero when the element can be scrolled to the left.

That said, I can see an argument for it being GetScrollFrame()->GetScrollRange().width (but that would presumably require scrollLeft to default to that value in RTL, reducing to zero as the element is scrolled) or -GetScrollFrame()->GetScrollRange().width (as that would still allow scrollLeft to vary between zero and scrollLeftMax, so to speak).
Flags: needinfo?(neil)
Tracking this to make sure it gets some traction. Thanks for linking it to bug 1200137. 
ehsan or dao, can you help us find an owner for this? Or is there someone better to ask?
Flags: needinfo?(ehsan)
Flags: needinfo?(dao)
Flags: needinfo?(dao) → needinfo?(enndeakin)
I'm sorry, I don't have time to work on this.
Flags: needinfo?(ehsan)
It sounds like Jonathan and Xidorn work on RTL issues nowadays.
Flags: needinfo?(quanxunzhen)
Flags: needinfo?(jfkthame)
I don't have time to tackle this at the moment (nor any detailed knowledge of this code).
Flags: needinfo?(jfkthame)
I'm not familiar with the code here. It seems roc could probably look into bug 383026 and see what can be done there first. If roc doesn't have time, I could probably try to help.
Flags: needinfo?(quanxunzhen) → needinfo?(roc)
OK, as I mentioned in bug 383026 comment 18, I believe our current behavior matches the spec. We can then work on this bug based on that behavior.
Flags: needinfo?(roc)
Flags: needinfo?(enndeakin)
I think the correct approach here is to introduce scrollLeftMin which is 0 for LTR and negative for RTL elements that can be scrolled to the left.
Not a recent regression, I don't think we will be able to fix that in 42 but still tracking it for the next releases.
Bug 962249 part 1 - Add chrome-only attribute horizontalScrollable and verticalScrollable to Element, and convert most uses of scrollLeftMax (and part of uses of scrollTopMax) to them instead.
Attachment #8676597 - Flags: review?(roc)
Bug 962249 part 2 - Add chrome-only attribute scrollWidth and scrollHeight to Window, and convert most uses of scrollMaxX (and part of uses of scrollMaxY) to them instead.
Attachment #8676598 - Flags: review?(roc)
It seems we have had a scrollWidth/scrollHeight in Element, which doesn't match what scrollLeftMax/scrollTopMax does. I hope adding those attributes to Window as Chrome-only with different behavior won't make it too confusing.

You may use this page to test the behavior: https://people.mozilla.org/~xquan/scrollleft.html
Comment on attachment 8676597 [details]
MozReview Request: Bug 962249 part 1 - Add chrome-only attribute horizontalScrollable and verticalScrollable to Element, and convert most uses of scrollLeftMax (and part of uses of scrollTopMax) to them instead.

https://reviewboard.mozilla.org/r/22767/#review20241

these seem less generally useful than scrollLeftMin/scrollTopMin. Why are we taking this approach?
Attachment #8676597 - Attachment is obsolete: true
Attachment #8676598 - Attachment is obsolete: true
Attachment #8676598 - Flags: review?(roc)
Bug 962249 part 1 - Add Element.scroll{Top,Left}Min (chrome-only) and convert most of scrollLeftMax uses and part of scrollTopMax uses to combinations with the new properties.
Attachment #8676653 - Flags: review?(roc)
Bug 962249 part 2 - Add Edge() helper method to BaseRect.
Attachment #8676654 - Flags: review?(roc)
Bug 962249 part 3 - Add Window.scrollMin{X,Y} (chrome-only) and convert most of scrollMaxX uses and part of scrollMaxY uses to combinations with the new properties.
Attachment #8676655 - Flags: review?(roc)
Comment on attachment 8676653 [details]
MozReview Request: Bug 962249 part 1 - Add Element.scroll{Top,Left}Min (chrome-only) and convert most of scrollLeftMax uses and part of scrollTopMax uses to combinations with the new properties.

https://reviewboard.mozilla.org/r/22777/#review20249
Attachment #8676653 - Flags: review?(roc) → review+
Comment on attachment 8676654 [details]
MozReview Request: Bug 962249 part 2 - Add Edge() helper method to BaseRect.

https://reviewboard.mozilla.org/r/22779/#review20251
Attachment #8676654 - Flags: review?(roc) → review+
Comment on attachment 8676655 [details]
MozReview Request: Bug 962249 part 3 - Add Window.scrollMin{X,Y} (chrome-only) and convert most of scrollMaxX uses and part of scrollMaxY uses to combinations with the new properties.

https://reviewboard.mozilla.org/r/22781/#review20253
Assignee: nobody → quanxunzhen
Comment on attachment 8676653 [details]
MozReview Request: Bug 962249 part 1 - Add Element.scroll{Top,Left}Min (chrome-only) and convert most of scrollLeftMax uses and part of scrollTopMax uses to combinations with the new properties.

It seems changing to webidl needs review from DOM peers. Kyle, could you have a look at those parts? I only add some chrome-only properties, so I guess should be fine.
Attachment #8676653 - Flags: review?(khuey)
Attachment #8676655 - Flags: review?(khuey)
Comment on attachment 8676653 [details]
MozReview Request: Bug 962249 part 1 - Add Element.scroll{Top,Left}Min (chrome-only) and convert most of scrollLeftMax uses and part of scrollTopMax uses to combinations with the new properties.

I don't review patches in mozreview.
Attachment #8676653 - Flags: review?(khuey) → review-
Attachment #8676653 - Flags: review- → review?(bzbarsky)
Attachment #8676655 - Flags: review- → review?(bzbarsky)
Comment on attachment 8676653 [details]
MozReview Request: Bug 962249 part 1 - Add Element.scroll{Top,Left}Min (chrome-only) and convert most of scrollLeftMax uses and part of scrollTopMax uses to combinations with the new properties.

https://reviewboard.mozilla.org/r/22777/#review20851

r=me assuming it was just for the IDL changes.
Attachment #8676653 - Flags: review?(bzbarsky) → review+
Attachment #8676655 - Flags: review?(bzbarsky) → review+
Comment on attachment 8676655 [details]
MozReview Request: Bug 962249 part 3 - Add Window.scrollMin{X,Y} (chrome-only) and convert most of scrollMaxX uses and part of scrollMaxY uses to combinations with the new properties.

https://reviewboard.mozilla.org/r/22781/#review20853

Again r=me for just the IDL bits.
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc5b89d9eacecc9933c7669579bb7db9617c0d69
Bug 962249 part 1 - Add Element.scroll{Top,Left}Min (chrome-only) and convert most of scrollLeftMax uses and part of scrollTopMax uses to combinations with the new properties. r=roc,bz

https://hg.mozilla.org/integration/mozilla-inbound/rev/6b366732ec446e9ccd04d0e286d2b48f4db9fb83
Bug 962249 part 2 - Add Edge() helper method to BaseRect. r=roc

https://hg.mozilla.org/integration/mozilla-inbound/rev/c0a884282954a8aaaee14d3c315c4b7d215f1fac
Bug 962249 part 3 - Add Window.scrollMin{X,Y} (chrome-only) and convert most of scrollMaxX uses and part of scrollMaxY uses to combinations with the new properties. r=roc,bz
Actually part 3 of this bug does the things wrong: it returns the app units directly as a result for scroll{Max,Min}{X,Y} instead of CSS Pixels. I'm going to fix it in bug 1217330.
I think this bug also introduced a typo: [1] should be comparing scrollTopMax to scrollTopMin rather than scrollTopMin to itself.

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js?rev=60acc8a9cfb5#5105
Flags: needinfo?(quanxunzhen)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #36)
> I think this bug also introduced a typo: [1] should be comparing
> scrollTopMax to scrollTopMin rather than scrollTopMin to itself.
> 
> [1]
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/
> browser.js?rev=60acc8a9cfb5#5105

Yeah, good catch. I'll push a fix for this tomorrow.
After IRC discussion I just went ahead and landed the correction as part of bug 1219243. (If this gets uplifted anywhere please make sure to uplift that too)
Flags: needinfo?(quanxunzhen)
Since the merge day is moved earlier, I'd prefer to fix this issue in this bug instead of doing so in bug 1217330, so that we can do uplift if necessary.
Attachment #8680342 - Flags: review?(roc)
https://hg.mozilla.org/integration/mozilla-inbound/rev/78a27c3cdb414e50a1ec6cfa9b2f169129cbaa53
Bug 962249 followup - Fix the bug of Window.scroll{Max,Min}{X,Y} that they incorrectly return app units instead of css pixels. r=roc
This patchset could be risky, and it is not a recent regression, so I'm not going to uplift them to beta.
Depends on: 1244969
See Also: → 1851749
You need to log in before you can comment on or make changes to this bug.