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

RESOLVED FIXED

Status

()

Core
Layout: Text
--
major
RESOLVED FIXED
15 years ago
3 years ago

People

(Reporter: Jari Kirma, Assigned: dbaron)

Tracking

(Depends on: 1 bug, Blocks: 2 bugs, {rtl})

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [patch], URL)

Attachments

(10 attachments, 14 obsolete attachments)

196 bytes, text/html
Details
642 bytes, text/html
Details
2.12 KB, application/xhtml+xml; charset=UTF-8
Details
17.43 KB, patch
dbaron
: review-
Details | Diff | Splinter Review
3.48 KB, patch
Details | Diff | Splinter Review
646 bytes, text/html
Details
2.05 KB, text/html; charset=UTF-8
Details
5.89 KB, text/plain; charset=utf-8
Details
65.99 KB, patch
Details | Diff | Splinter Review
1.57 KB, patch
roc
: review+
Details | Diff | Splinter Review
(Reporter)

Description

15 years ago
User-Agent:       Mozilla/5.0 (X11; U; FreeBSD i386; en-US; rv:1.3a) Gecko/20030128
Build Identifier: Mozilla/5.0 (X11; U; FreeBSD i386; en-US; rv:1.3a) Gecko/20030128

One or both scroll bars are missing if you display above page with 300% text
zoom at 1600x1200 resolution.

Reproducible: Sometimes

Steps to Reproduce:
1.See details.
2.
3.

Actual Results:  
One or no scroll bar available.

Expected Results:  
Both scroll bars rendered.
Confirming and adjusting summary. The |dir=rtl| on the <body> is what makes this
happen: we are obviously only checking whether the content extends to the right
of the viewport, and not whether it extends to the right of it.

As far as I remember this used to work, but I'm not 100% sure.
Assignee: mkaply → smontagu
OS: FreeBSD → All
Summary: Scroll bars missing when displaying bidirectional text page that doesn't fit in the screen → Scroll bars missing when displaying a right-to-left page that doesn't fit in the screen

Comment 2

15 years ago
Yep, this did work, but it broke some time ago. Not sure when thugh
Status: UNCONFIRMED → NEW
Ever confirmed: true
Hardware: PC → All

Comment 3

14 years ago
this is a major thing rgression- many rtl pages are cut off if they are too wide.
Severity: normal → major

Updated

14 years ago
Flags: blocking1.4?
Summary: Scroll bars missing when displaying a right-to-left page that doesn't fit in the screen → Scroll bars missing when displaying a right-to-left (rtl) page that doesn't fit in the screen

Updated

14 years ago
Blocks: 137995

Comment 4

14 years ago
This is one of the most FAQ in the Israeli mozilla forum. 
As noted before, this is a regression, that should be fixed.
No longer blocks: 137995
Summary: Scroll bars missing when displaying a right-to-left (rtl) page that doesn't fit in the screen → [regression] Scroll bars missing when displaying a right-to-left ( rtl / Hebrew / arabic) page that doesn't fit in the screen

Comment 5

14 years ago
Note that it has nothing with the zooming. Every pa

Comment 6

14 years ago
Strange. Somehow my post got cut. Well, just wanted to notice that this is very
major problem for us, while every large picture or <PRE> text make the page
functionless. 

See here: http://mozilla.org.il/board/viewtopic.php?t=53


Also relevant bug: bug #6976

Updated

14 years ago
Blocks: 137995

Comment 7

14 years ago
mozilla1.4 shipped. unsetting blocking1.4 request.

Updated

14 years ago
Flags: blocking1.4?

Updated

13 years ago
Blocks: 240501
*** Bug 251940 has been marked as a duplicate of this bug. ***
attachment 153561 [details] from bug 251940 is a minimized testcase.

Comment 10

13 years ago
a temoprary fix for this issue is to add this code to the stylesheet:

body > table { direction: ltr } body > table > * { direction: rtl }

or a bookmarklet:

javascript:styles='body > table { direction: ltr } body > table > * { direction:
rtl }'; newSS = document.createElement('link'); newSS.rel = 'stylesheet';
newSS.href = 'data:text/css,' + escape(styles);
document.documentElement.childNodes[0].appendChild(newSS); void 0

Comment 11

13 years ago
(In reply to comment #10)
> a temoprary fix for this issue is to add this code to the stylesheet:
> 
> body > table { direction: ltr } body > table > * { direction: rtl }
> 
> or a bookmarklet:
> 
> javascript:styles='body > table { direction: ltr } body > table > * { direction:
> rtl }'; newSS = document.createElement('link'); newSS.rel = 'stylesheet';
> newSS.href = 'data:text/css,' + escape(styles);
> document.documentElement.childNodes[0].appendChild(newSS); void 0

The CSS (in userContent.css) works here:
http://www.mozilla.org.il/board/viewtopic.php?p=8252

Comment 12

13 years ago
(In reply to comment #11)
> The CSS (in userContent.css) works here:
> http://www.mozilla.org.il/board/viewtopic.php?p=8252

But I see your CSS confuses some pages in English: see
http://www.mozillazine.org/talkback.html?article=6133. I suggest:
html[dir="rtl"] > body > table, body[dir="rtl"] > table
{
	direction: ltr;
}
html[dir="rtl"] > body > table > *, body[dir="rtl"] > table > *
{
	direction: rtl;
}
It works to me in http://www.mozilla.org.il/board/viewtopic.php?p=8252, and also
in http://www.mozillazine.org/talkback.html?article=6133 - it may not work in
other pages, but I think it is inevitable.

Comment 13

13 years ago
indeed you solution seems better since it can tell if the page was originally
rtl (buggy) or not, and work upon it... but it still is a "hack" till the
codebase itself gets fixed from the root, but this could serve as something ok
till then..

Comment 14

13 years ago
(In reply to comment #12)
> I suggest:
> html[dir="rtl"] > body > table, body[dir="rtl"] > table
> {
> 	direction: ltr;
> }
> html[dir="rtl"] > body > table > *, body[dir="rtl"] > table > *
> {
> 	direction: rtl;
> }
> It works to me in http://www.mozilla.org.il/board/viewtopic.php?p=8252, and also
> in http://www.mozillazine.org/talkback.html?article=6133 - it may not work in
> other pages, but I think it is inevitable.

this still breaks some pages. you may want to have a look here:
http://www.mozilla.org.il/

Comment 15

12 years ago
*** Bug 300343 has been marked as a duplicate of this bug. ***
Created attachment 192992 [details]
minimal testcase

Well, it's about time this gets a minimal testcase.
Note that the "border: 1px solid red; text-align:left;" is there just to
demonstrate that the text (and the left border of the div) are off the screen,
and unreachable.

Unless you have a really big screen (>2000px), you shouldn't be able to see the
text on this page.
Adjusting summary.
I tested this on Mozilla 1.0 and Mozilla 1.2 and it's broken. So I don't believe
this is a regression. If you have evidence to the contrary, please present it.
Summary: [regression] Scroll bars missing when displaying a right-to-left ( rtl / Hebrew / arabic) page that doesn't fit in the screen → Horizontal scrollbar missing on right-to-left (RTL/Hebrew/Arabic) page that doesn't fit in the screen
(Assignee)

Comment 18

12 years ago
So (using this bug since bug 6976 is just too confused), the big question here is:  given that overflow to the left of the initial containing block should be reachable by scrolling on an rtl page, is it ok if overflow to the right is *not* reachable?  For symmetry, I think it should not be reachable; it's certainly nasty to present the user with a default scrollbar position in the middle.  Is that what WinIE does?  If so, it (allowing scrolling only to the left for rtl pages) would probably be safe for us to do; if not, Web pages may depend on things working other ways (how?).
(Assignee)

Updated

12 years ago

Comment 19

12 years ago
*** Bug 314895 has been marked as a duplicate of this bug. ***
(In reply to comment #18)
> So (using this bug since bug 6976 is just too confused), the big question here
> is:  given that overflow to the left of the initial containing block should be
> reachable by scrolling on an rtl page, is it ok if overflow to the right is
> *not* reachable?  For symmetry, I think it should not be reachable; it's
> certainly nasty to present the user with a default scrollbar position in the
> middle.  Is that what WinIE does?  If so, it (allowing scrolling only to the
> left for rtl pages) would probably be safe for us to do; if not, Web pages may
> depend on things working other ways (how?).

I tried testing this in IE using <div style="position:absolute; left:1000px;"> inside an RTL body (when the window width is less than 1000px). IE gets confused by this: it does show a scrollbar, but one which lets you scroll to the left! So the content (on the right) is, in fact, unreachable (unless you make the window wider, and then you can initially see the left portion of the content, then all of it, and the useless scrollbar disappears).
I doubt if any web pages are relying on this obviously buggy behavior of IE.
Safari and Opera do let you scroll to the right to see the overflowing content (that is, their behavior is not symmetrical in the RTL and LTR cases).

There is an underlying assymetry which I think is at the root of this: the coordinate system is not reversed in RTL context (it still goes from left to right). I wonder if reversing the coordinate system in RTL context was ever considered when the CSS standard was made - it would have led to much easier implementation of symmetrical behavior, I think. But this is now a theoretical issue, I suppose.

Are there other methods of overflowing content to the right (in RTL context) which you would like me to test? Off the top of my head, I can't think of any.
(Assignee)

Comment 21

12 years ago
The other common one is just <div style="width: 1000px; border: medium solid"></div>.  (It's possible that we currently incorrectly overflow some things on the right instead of the left in RTL cases, I'd want to fix those at the same time as this bug, and some are in fact marked dependencies of this bug.)
(Assignee)

Comment 22

12 years ago
(In reply to comment #20)
> right). I wonder if reversing the coordinate system in RTL context was ever
> considered when the CSS standard was made - it would have led to much easier
> implementation of symmetrical behavior, I think. But this is now a theoretical

The CSS standard doesn't specify a coordinate system, fwiw.  Using a top-left based coordinate system was an implementation decision.
(In reply to comment #21)
> The other common one is just <div style="width: 1000px; border: medium
> solid"></div>.  (It's possible that we currently incorrectly overflow some
> things on the right instead of the left in RTL cases, I'd want to fix those at
> the same time as this bug, and some are in fact marked dependencies of this
> bug.)

This case overflows to the left in IE (as in Mozilla). IE provides a scrollbar.
(In reply to comment #22)
> The CSS standard doesn't specify a coordinate system, fwiw.  Using a top-left
> based coordinate system was an implementation decision.

Yes, I was confused and wrong. Sorry about that.
Why shouldn't we present the user with a default scrollbar location in the middle for both LTR and RTL cases? It sounds reasonable to me.
(Assignee)

Comment 26

12 years ago
It's difficult to get back to the default location if it's in the middle.  For example, consider a user who wants to scroll to see one thing that overflows and then return to the initial position and keep reading.

Comment 27

12 years ago
(In reply to comment #26)
> It's difficult to get back to the default location if it's in the middle.  For
> example, consider a user who wants to scroll to see one thing that overflows
> and then return to the initial position and keep reading.

The thing is, that in 9 cases out of 10 (at least AFAICT), this kind of overflow happens due to an oversight, and the page doesn't turn out the way the user or the author expected anyway. And in these cases it's more important to allow the user to be able to view all of the content rather than enjoy a 'nicer' browsing experience which he/she is probably not getting anyway. It's much more frustrating not to be able to see some of the content than not to be able to scroll it to just the right position.
(Assignee)

Comment 28

12 years ago
The discussion about scrolling to both sides does NOT belong on this bug.  If you want to have that discussion, please use bug 6976.
(Assignee)

Comment 29

12 years ago
(OK, I admit, I sort of started it, but I shouldn't have, and there are other reasons it's really something we can't do.)

Comment 30

12 years ago
(In reply to comment #28)
> The discussion about scrolling to both sides does NOT belong on this bug.

But how can this bug be fixed other than by enabling scrolling to both sides? Moving the origin?
(Assignee)

Comment 31

12 years ago
When the root element is rtl, the page should scroll only to the left and not to the right (and we also fix some of the layout bugs that cause things in rtl pages to overflow to the right).

Comment 32

12 years ago
(In reply to comment #31)
> When the root element is rtl, the page should scroll only to the left and not
> to the right (and we also fix some of the layout bugs that cause things in rtl
> pages to overflow to the right).

You would still need a 4-coordinate scroll range, unless you want the 2-coordinate range to mean bottom-right point. Plus, what happens  when content overlows to the top? From what I remember of the code, it would probably be simpler to just allow scrolling in any direction than implement another solution.
(Assignee)

Comment 33

12 years ago
Not user-interface-wise it wouldn't.

If you want to advocate for that, please to it on bug 6976 instead, or this bug will also become unreadable and unfixable.

Comment 34

12 years ago
Created attachment 208622 [details] [diff] [review]
patch

This patch is effective in making horizontal scrollbar appear in the screen.
Attachment #208622 - Flags: review?(dbaron)
(Assignee)

Comment 35

12 years ago
Comment on attachment 208622 [details] [diff] [review]
patch

This should be fixed for all scrollframes, not just the canvas, and it shouldn't need another resize reflow.

I actually had a partial patch back around when I was commenting before, but there was some existing IBMBIDI code that I didn't have time to understand.
Attachment #208622 - Flags: review?(dbaron) → review-

Comment 36

12 years ago
Created attachment 210700 [details] [diff] [review]
patch

This patch tries to resolve following three problems as to rtl direction for all scrollframes, although it needs resize reflow.

1. Horizontal scrollbar missing
2. scroll position missing when the page is loaded
3. scroll position can not be retained when the page is reloaded

This patch is effective for testcase of Bug 202386.
Attachment #208622 - Attachment is obsolete: true

Comment 37

12 years ago
Created attachment 210701 [details]
testcase for patch

Comment 38

12 years ago
This patch uses a new flag |NS_REFLOW_DIRECTION_RTL| for |nsHTMLReflowMetrics| and passes to scrollframes because the container block does not have the same direction althouth the contents have the rtl direction.

The following change needs becase horizontal scrollbar moves from right to left.

nsGfxScrollFrameInner::ScrollToRestoredPosition
-    if (y > cy || x > cx) {
+    if (y > cy || x > cx || (NS_STYLE_DIRECTION_RTL == mLastDir && x < cx)) {

The following change needs becase horizontal scrollbar should be moved to left or right edge if the position was not restored.

nsGfxScrollFrameInner::AdjustHorizontalScrollbar
-  if (needScroll) {
+  if (needScroll && !mDidHistoryRestore) {

The following change needs becase default value of |mOnePixel| is 20, but this value should be 12 if on windows.

nsGfxScrollFrameInner::SetAttribute
+  mOnePixel = mOuter->GetPresContext()->IntScaledPixelsToTwips(1);

The following change needs becase the position at (0,0) should be restored for rtl direction.

nsGfxScrollFrameInner::SaveState
+#ifdef IBMBIDI
+    if (!mRestoreRect.x && !mRestoreRect.y)
+#endif

The change for |nsHTMLReflowState::CalculateBlockSideMargins| needs because the available margin space should include the width of the table frame's border and padding.
(In reply to comment #36)
> Created an attachment (id=210700) [edit]
> patch

I took the patch for a test-drive. It's doing well in most cases, but it also completely breaks RTL pages with <table align="center"> in them. You can see an example of this in http://mozilla.org.il/board/. With the patch, this page looks blank, and there is a horizontal scrollbar which lets you scroll a very large distance.

Comment 40

12 years ago
Created attachment 212380 [details] [diff] [review]
patch

Uri, thanks for your testing. I update to this patch.
I think that the changes for nsHTMLReflowState::CalculateBlockSideMargins and nsTableOuterFrame::SetDesiredSize resolve the problem for the calculated width of the contents using unset margins.
Attachment #210700 - Attachment is obsolete: true
Hideo, why not ask for a review on your patch? Many people (including me) will be very thankful if this gets fixed.
(Assignee)

Comment 42

12 years ago
I would really much prefer the other approach:  make nsGfxScrollFrame able to deal with scrolling to negative coordinates rather than change yet more other code to place things at different coordinates to work around the deficiencies of nsGfxScrollFrame.
(Assignee)

Comment 43

12 years ago
I'd note that I actually tried to write a patch to do that, bug gave up when I couldn't understand what nsGfxScrollFrameInner::AdjustHorizontalScrollbar was trying to do.  If somebody could explain what that function is *supposed* to do, then I could probably finish my patch.
That function is designed to scroll to the 'initial position' (all the way to the right for RTL, all the way to the left for LTR) when the scrollbar is first created or the RTL/LTR direction changes.
(Assignee)

Comment 45

12 years ago
(In reply to comment #44)
> That function is designed to scroll to the 'initial position' (all the way to
> the right for RTL, all the way to the left for LTR) when the scrollbar is first
> created or the RTL/LTR direction changes.

That doesn't make much sense given that we don't scroll to the left on RTL pages -- that's what this bug is about.
Hey, I'm just the messenger.

If we could actually scroll to the left, then I think we wouldn't need that function at all, even if it worked.
(Assignee)

Comment 47

12 years ago
Created attachment 212415 [details] [diff] [review]
patch to make our scrolling code accept negative coordinates

This is the approach I was talking about.  I suspect AdjustHorizontalScrollbar is some sort of bizarre workaround for the mHas*Scrollbar checks that I removed from nsGfxScrollFrameInner::LayoutScrollbars, but I still don't understand why the code there worked.  In any case, I removed it.

This removes all the hacks for RTL text controls, which shouldn't need hacks anymore.

This renames IsScrollbarOnRight to IsLTR and actually makes it work for the scrollbars around the root by looking at the root element or the body element.

The changes to the view code aren't as general as they could be; I should probably revisit those, and a few other things.
Assignee: smontagu → dbaron
Status: NEW → ASSIGNED
(Assignee)

Comment 48

12 years ago
Oh, and I forgot the biggest mess:  the fix to CanvasFrame doesn't really match what we do for LTR, so the canvas frame doesn't cover the area to which we'd do leftward scrolling.  That's bad for background painting but actually I think an improvement for the canvas focus outline.
(Assignee)

Comment 49

12 years ago
Oh, and the other point:  I'm not actually convinced that we should switch the sides that the viewport scrollbars are on; I used to think not, but I also think we shouldn't do it for non-viewport scrollbars but not viewport scrollbars.

I also seem to have broken left edge determination for the LTR case (for keyboard scrolling, at least).
(Assignee)

Comment 50

12 years ago
(In reply to comment #49)
> I also seem to have broken left edge determination for the LTR case (for
> keyboard scrolling, at least).

Er, actually, what I broke are pages, like bugzilla, that position things way off to the left in the hope that they're not reachable.  My use of the scrolling view's dimensions is incorrect -- I need a better way for nsScrollPortView to know what the dimensions of the thing that it's scrolling are.
I think that in a left-overflow situation in LTR, we should have something like this example:

Suppose the scrolled content is 100x100 and overflows to the left by 100px. Then when we're scrolled over to the right (ignoring scrollbars, i.e. overflow:hidden)

nsGfxScrollFrame: (x,y,100,100), no overflow area
  nsScrollPortView: position (0, 0), dimensions (0, 0, 100, 100)
  nsBlockFrame: (0,0,100,100), overflow area (-100, 0, 200, 100)
    nsView: position (0, 0), dimensions (-100, 0, 200, 100)

When we're scrolled over to the left

nsGfxScrollFrame: (x,y,100,100), no overflow area
  nsScrollPortView: position (0, 0), dimensions (0, 0, 100, 100)
  nsBlockFrame: (0,0,100,100), overflow area (-100, 0, 200, 100)
    nsView: position (100, 0), dimensions (-100, 0, 200, 100)

OK?
(Assignee)

Comment 52

12 years ago
Yeah, except my goal is only to create such situations in RTL.  Although I didn't fully attempt to parse the position/dimensions bit for the views within that, because that makes my head hurt.

And it should all be fixable with some math tweaking in the scrollframe code, actually.
(Assignee)

Comment 53

12 years ago
Created attachment 212421 [details] [diff] [review]
patch to make our scrolling code accept negative coordinates

This fixes a bunch of the math by making GetScrolledRect do the clamping, and then fixing all the callers of GetScrolledRect (and adding some new callers, including some that thus switch from using the overflow area from the metrics to using the frame property) to stop making assumptions about any clamping that it may or may not do.

Still has nsCanvasFrame issues.
Attachment #212415 - Attachment is obsolete: true
(Assignee)

Comment 54

12 years ago
...and makes resizing the window reset the scrollbars.  Oops.  I'll debug that later and attach something new.

Not worth testing this yet.
(Assignee)

Comment 55

12 years ago
So I fixed the problem in my previous comment by fixing the mHas*Scrollbar checks in LayoutScrollbars to do the right thing (restore old curpos) rather than skipping curpos completely.

I still have to:
 * fix CanvasFrame
 * test XULScrollFrame more (I think it needs some work)
 * figure out why I see problems when maximizing an RTL page scrolled all the way to the left such that the scrollbars go away (perhaps we're optimizing something we shouldn't be)
(Assignee)

Comment 56

12 years ago
Created attachment 212447 [details] [diff] [review]
patch to make our scrolling code accept negative coordinates

Here's the current state of my patch.  I'm putting it aside for now.

It seems to make XUL scroll frames a good bit worse than they used to be, unfortunately.
Attachment #212421 - Attachment is obsolete: true
(Assignee)

Comment 57

12 years ago
Created attachment 212448 [details]
some testcases, mostly xul-ish
(Assignee)

Updated

12 years ago
Assignee: dbaron → mozilla
Status: ASSIGNED → NEW
Are you planning to come back to this or do you want someone to take it over?
(Assignee)

Comment 59

12 years ago
Not planning to come back to it anytime soon, at least.  At least, I shouldn't...
(Assignee)

Comment 60

12 years ago
Actually, I think there's a quick way to leave the XUL case in the state it was before the patch.
Assignee: mozilla → dbaron
Whiteboard: [patch]
(Assignee)

Comment 61

12 years ago
Created attachment 212560 [details] [diff] [review]
patch to make our scrolling code accept negative coordinates

This leaves the XUL testcases as they were (by letting nsGfxScrollFrameInner know when it's got a XUL outer), except for changing the 6 that currently have an inconsistent state (display horizontal scrollbar in middle, but actually scrolled to right) by making the state consistent but different from both (scrolled to left).

I think this one should be worth testing; I can't make all that useful a contribution there.
Attachment #212447 - Attachment is obsolete: true

Comment 62

12 years ago
Created attachment 213316 [details] [diff] [review]
part-1/4

I separated my patch(attachment 212380 [details] [diff] [review]) into four parts as follows and merged into attachment 212560 [details] [diff] [review]. David, when you return to this, please check these changes.

Part1 is for nsGfxScrollFrameInner::IsLTR() because I think that the direction of the contents has an effect to the container block.

Part2 is for return to saved position for horizontal scrollbar, when the page is reloaded.

Part3 is for nsHTMLReflowState::CalculateBlockSideMargins because the available margin space should include the width of the table frame's border and padding. Testcase for part3 is attachment 210701 [details].

Part4 is for another case of horizontal scrollbar missing. The changes for nsHTMLReflowState::CalculateBlockSideMargins and nsTableOuterFrame::SetDesiredSize are required because the width of the contents is calculated using unset margins(the value is 0x40000000).
Attachment #212380 - Attachment is obsolete: true

Comment 63

12 years ago
Created attachment 213318 [details] [diff] [review]
part-2/4

Comment 64

12 years ago
Created attachment 213319 [details] [diff] [review]
part-3/4

Comment 65

12 years ago
Created attachment 213320 [details] [diff] [review]
part-4/4

Comment 66

12 years ago
Created attachment 213321 [details]
testcase for part-4/4
(Assignee)

Updated

12 years ago
Attachment #213319 - Flags: review?(dbaron)

Comment 67

12 years ago
Created attachment 213589 [details] [diff] [review]
part-4/4

I removed a code for resize reflow because blue box of testcase for part4 should not overflow to the right.
Attachment #213320 - Attachment is obsolete: true
(In reply to comment #61)
> Created an attachment (id=212560) [edit]
> patch to make our scrolling code accept negative coordinates
>
> I think this one should be worth testing; I can't make all that useful a
> contribution there.

I'm willing to do whatever I can to get this tested and landed. Is there any specific or systematic testing that has to be done, or would random surfing in RTL sites be enough?

If you think this should be tested by a wider audience, I can try and help recruit people on mozilla.org.il (although we'll have to find a way to supply them with builds).
(Assignee)

Comment 69

12 years ago
Well, I think the stuff that needs to be done is at least:
 * making sure the behavior this implements is what we want
 * figure out what needs to be done for the canvas frame issues
 * some testing of various cases, although probably not a huge amount
(Assignee)

Updated

12 years ago
Attachment #213320 - Flags: review?(dbaron)
One thing I'm noticing with this patch is that it doesn't work well with "caret scrolling" (automatic scrolling to keep the caret on-screen when it is moved).

This is most evident in textareas when I test this together with my patch for bug  318116, but can also be seen (in caret browsing mode) with the testcase attached to this bug: Moving the caret using arrow keys inside "Some text" not only doesn't bring the caret into view when necessary, but even, if the window is narrow enough, scrolls towards the middle of the div, so that the text is way off-screen to the left.

Comment 71

12 years ago
I don't want to jump the gun, but if we only find issues which need additional work rather than regressions, the patch/es could be landed and additional one/s could be written separately.

Updated

12 years ago
Attachment #213589 - Attachment is obsolete: true

Comment 72

12 years ago
Comment on attachment 213320 [details] [diff] [review]
part-4/4

Strings of testcase on attachment 204454 [details] of Bug 318116 are aligned to the right by applying this patch.
Attachment #213320 - Attachment is obsolete: false

Comment 73

12 years ago
Comment on attachment 213318 [details] [diff] [review]
part-2/4

As to following changes of part2:
+    if (mHasHorizontalScrollbar) {
+      SetAttribute(mHScrollbarBox, nsXULAtoms::maxpos, maxX - minX);
+    }

When boolean of mHasHorizontalScrollbar is false, an attribute of nsXULAtoms::maxpos is set to zero x-coordinates on window. However, when an attribute of nsXULAtoms::curpos is set to restored scroll position and the position is zero x-coordinates on windows, that value is already defined, then, the scroll position can not move to the left side from the right side.
Attachment #213318 - Flags: review?(dbaron)

Updated

12 years ago
Blocks: 324685

Comment 74

12 years ago
Comment on attachment 213589 [details] [diff] [review]
part-4/4

Comment #72 is wrong because the required resize reflow causes an unexpected change of the layout.
Attachment #213589 - Attachment is obsolete: false
Attachment #213589 - Flags: review?(dbaron)

Updated

12 years ago
Attachment #213320 - Attachment is obsolete: true
Attachment #213320 - Flags: review?(dbaron)
(Assignee)

Updated

12 years ago
Attachment #213316 - Flags: review-
While testing dbaron's patch, I came across a page which appeared to be regressed by it (i.e., it correctly has a horizontal scrollbar without the patch, but is lacking one with it).
A minimal testcase proved to be a simple table in an RTL page, with long, unbreakable content, and no alignment specified. But then I realized that this testcase is equivalent to the one in bug 202386 (attachment 120809 [details]), and that it is that bug which currently negates the effect of this bug on such cases, but exposes it once again with the patch.

So I don't think there is a fault with the patch, but I do think that we should make sure to fix bug 202386 for the same release that this bug is fixed for, otherwise we might end up breaking as many web pages as we fix.

Bugzilla won't let me create a circular dependency, so we'll just have to keep this in mind.
(Assignee)

Comment 76

12 years ago
Created attachment 214750 [details]
testcases for canvas background issues

These test some canvas background issues (note that the yellow background doesn't continue to the right).

(Note that this doesn't test the focus outline issues.)
(Assignee)

Comment 77

12 years ago
Created attachment 214756 [details] [diff] [review]
patch to make our scrolling code accept negative coordinates

This fixes the canvas issues by making the size of the canvas irrelevant, both for backgrounds and for focus painting.  It also fixes an old bug in PaintCanvasFocus (the scrollableView null-check was always failing).

I tested this for no more than a minute, but I'm getting hungry, so that's all it'll get for at least a few hours.
Attachment #212560 - Attachment is obsolete: true
(Assignee)

Comment 78

12 years ago
This one's pretty broken; I thought a change I made would have fixed some background bugs I saw, but it didn't.
(Assignee)

Comment 79

12 years ago
So bug 330302 was confusing me into thinking the repaint problems were worse than they really were (although there was a real bug that I've fixed).  But I've now noticed problems with textareas scrolling to keep up with typing.
(Assignee)

Comment 80

12 years ago
Created attachment 214875 [details] [diff] [review]
patch to make our scrolling code accept negative coordinates

I still need to figure out why this breaks vertical scrolling to match typing in textareas that have no horizontal scrollbar.
Attachment #214756 - Attachment is obsolete: true
(Assignee)

Comment 81

11 years ago
So the problem with textareas is related to my removal of the code that increases the size of the frame to fill the scrollport in both dimensions (which thus seems to increase the size of the view, although I'd thought I would still be increasing the size of the view).  That causes the code from bug 268352 to kick in:

(gdb) f 3
#3  0x013ba146 in nsScrollPortView::ScrollTo (this=0x8dab2d8, aDestinationX=0,
    aDestinationY=-708, aUpdateFlags=Variable "aUpdateFlags" is not available.
)
    at /builds/trunk/mozilla/view/src/nsScrollPortView.cpp:294
294           return ScrollToImpl(aDestinationX, aDestinationY, aUpdateFlags);
(gdb) up
#4  0x011069be in nsTextInputSelectionImpl::ScrollSelectionIntoView (
    this=0x8da8198, aType=1, aRegion=1, aIsSynchronous=1)
    at /builds/trunk/mozilla/layout/forms/nsTextControlFrame.cpp:646
646           return scrollableView->ScrollTo(PR_MAX(viewRect.width - portRect.width, 0), viewRect.y, 0);
(gdb) l
641           return rv;
642         }
643         const nsRect portRect = scrollableView->View()->GetBounds();
644         const nsRect viewRect = view->GetBounds();
645         if (viewRect.XMost() < portRect.width) {
646           return scrollableView->ScrollTo(PR_MAX(viewRect.width - portRect.width, 0), viewRect.y, 0);
647         }
648
649         return rv;
650       }
(gdb) p viewRect
$1 = {x = 0, y = -708, width = 696, height = 2700}
(gdb) p portRect
$2 = {x = 0, y = 0, width = 6732, height = 1992}

which scrolls us back to the top of the textarea.
(Assignee)

Comment 82

11 years ago
Created attachment 214945 [details] [diff] [review]
patch to make our scrolling code accept negative coordinates
Attachment #214875 - Attachment is obsolete: true
(Assignee)

Updated

11 years ago
Depends on: 330302
(Assignee)

Comment 83

11 years ago
The patches to bug 192767 (scrolling to right), bug 96394 (block overflow should go to right), and bug 318116 (inline overflow should go to right) should all be landed at about the same time.
(Assignee)

Updated

11 years ago
Attachment #214945 - Flags: superreview?(roc)
Attachment #214945 - Flags: review?(roc)
(Assignee)

Updated

11 years ago
Attachment #214945 - Flags: superreview?(roc)
Attachment #214945 - Flags: review?(roc)
(Assignee)

Comment 84

11 years ago
Created attachment 214954 [details] [diff] [review]
patch to make our scrolling code accept negative coordinates
Attachment #214945 - Attachment is obsolete: true
Attachment #214954 - Flags: superreview?(roc)
Attachment #214954 - Flags: review?(roc)
(Assignee)

Comment 85

11 years ago
Created attachment 214972 [details]
IRC log (from #developers) about one of the decisions in this patch
This patch is going to have some conflicts due to changes in nsDisplayItem and subclasses, but they should be easy to resolve.
Comment on attachment 214954 [details] [diff] [review]
patch to make our scrolling code accept negative coordinates

Not changing the size of the scrolled frame is a pretty big change, but I guess it's the right thing to do.

+  virtual nsIFrame* GetUnderlyingFrame() { return mFrame; }
+  CanvasFrame *mFrame;

You should remove these to adjust for the nsDisplayItem changes.

I couldn't find anything wrong, which is disturbing. Have you tested with smoothscroll?

I'll review the other patches soon.
Attachment #214954 - Flags: superreview?(roc)
Attachment #214954 - Flags: superreview+
Attachment #214954 - Flags: review?(roc)
Attachment #214954 - Flags: review+
(Assignee)

Comment 88

11 years ago
(In reply to comment #87)
> Not changing the size of the scrolled frame is a pretty big change, but I guess
> it's the right thing to do.

It's also something we could revisit during or after view removal.

> Have you tested with smoothscroll?

Just did; seems fine, both horizontal and vertical, by lines and by pages, including on a page where the scrolling is to the left.
(Assignee)

Comment 89

11 years ago
Created attachment 215135 [details] [diff] [review]
patch to make our scrolling code accept negative coordinates

This is merged to the trunk after bug 330300 landed.

I also added the following comment to GetScrolledRect, which I meant to add earlier:
  /**
   * GetScrolledRect is designed to encapsulate deciding which
   * directions of overflow should be reachable by scrolling and which
   * should not.  Callers should NOT depend on it having any particular
   * behavior (although nsXULScrollFrame currently does).
   *
   * Currently it allows scrolling down and to the right for
   * nsHTMLScrollFrames with LTR directionality and for all
   * nsXULScrollFrames, and allows scrolling down and to the left for
   * nsHTMLScrollFrames with RTL directionality.
   */
Attachment #214954 - Attachment is obsolete: true
(Assignee)

Comment 90

11 years ago
Checked in to trunk.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Updated

11 years ago
Attachment #213319 - Attachment is obsolete: true
Attachment #213319 - Flags: review?(dbaron)

Updated

11 years ago
Attachment #213589 - Attachment is obsolete: true
Attachment #213589 - Flags: review?(dbaron)
(Assignee)

Updated

11 years ago
Attachment #213319 - Flags: review?(dbaron)
(Assignee)

Updated

11 years ago
Attachment #213319 - Flags: review?(dbaron)
This caused a bit of Tdhtml hit.  Almost all of the hit was on the "zoom" test (which got about 30% slower) and the "imageslide" test (6% slower).  A number of the other tests actually got a little faster, and "replaceimages" got just a tiny bit slower.

Does this patch change the actual behavior on the "zoom" test, by chance?
It's also possible that bug 96394 is responsible, not this bug; this one seemed more likely to me.

Updated

11 years ago
Depends on: 330732

Updated

11 years ago
Depends on: 330734

Updated

11 years ago
Attachment #213318 - Flags: review?(dbaron)
(Assignee)

Comment 93

11 years ago
Created attachment 215353 [details] [diff] [review]
fix potential problems initially setting attributes to -1
Attachment #215353 - Flags: superreview?(roc)
Attachment #215353 - Flags: review?(roc)
(Assignee)

Comment 94

11 years ago
(In reply to comment #91)
> This caused a bit of Tdhtml hit.  Almost all of the hit was on the "zoom" test
> (which got about 30% slower) and the "imageslide" test (6% slower).  A number

I profiled the "zoom" testcase and didn't see anything stick out; it looked like less than 30% of the time was spent in reflow.

Updated

11 years ago
Depends on: 331049
Attachment #215353 - Flags: superreview?(roc)
Attachment #215353 - Flags: superreview+
Attachment #215353 - Flags: review?(roc)
Attachment #215353 - Flags: review+
(Assignee)

Comment 95

11 years ago
Comment on attachment 215353 [details] [diff] [review]
fix potential problems initially setting attributes to -1

Checked in to trunk.
(Assignee)

Updated

11 years ago
No longer depends on: 330302
Depends on: 331385
(Assignee)

Updated

11 years ago
Depends on: 330863

Updated

11 years ago
Blocks: 331562

Updated

11 years ago
Blocks: 331607
Depends on: 331594
(Assignee)

Updated

11 years ago
Depends on: 331458

Updated

11 years ago
Depends on: 331746
Depends on: 333817

Updated

11 years ago
Depends on: 338730

Updated

11 years ago
No longer depends on: 338730

Updated

11 years ago
Blocks: 285718
Depends on: 353460
*** Bug 359442 has been marked as a duplicate of this bug. ***

Comment 97

11 years ago
Could this have caused bug 365101 ?

Comment 98

11 years ago
Just noting that the 20060315 check-in from this bug (attachment 215135 [details] [diff] [review]) caused bug 330673.
(Assignee)

Updated

11 years ago
Depends on: 330673
(Assignee)

Updated

11 years ago
Blocks: 365666
(Assignee)

Comment 99

11 years ago
I filed bug 365666 as a followup bug on fixing comment 56 and comment 61.
No longer blocks: 365666
Comment on attachment 215353 [details] [diff] [review]
fix potential problems initially setting attributes to -1


>+  if (oldValue == newValue)

Should this not be comparing the integer values the 2 strings represent?  Perhaps I am confused, but this would appear to be comparing the pointers which I would think would always differ.
(Assignee)

Comment 101

11 years ago
Our string classes define operator==, so it does a string comparison.
(In reply to comment #101)
> Our string classes define operator==, so it does a string comparison.
> 
OK.  That is good to know.  Thanks for the explanation.  It seems this code was modified by bug 333896 anyway.

Updated

11 years ago
Depends on: 367246

Updated

11 years ago
Depends on: 369610

Updated

10 years ago
Duplicate of this bug: 388882
Mass-assigning the new rtl keyword to RTL-related (see bug 349193).
Keywords: rtl
Depends on: 439293

Updated

9 years ago
Component: Layout: BiDi Hebrew & Arabic → Layout: Text
QA Contact: zach → layout.fonts-and-text
The tests for this bug were (accidentally?!) landed as part of <http://hg.mozilla.org/mozilla-central/rev/2609e7fdf1bf>.
Flags: in-testsuite+
(Assignee)

Comment 106

7 years ago
I'm not sure in-testsuite+ is appropriate here, since the tests that were landed were only for the XUL half and not the HTML half (which was what was actually fixed in this bug back in 2006).
Flags: in-testsuite+ → in-testsuite?
(In reply to comment #105)
> The tests for this bug were (accidentally?!) landed as part of
> <http://hg.mozilla.org/mozilla-central/rev/2609e7fdf1bf>.

Not accidentally -- many of those tests failed without the fix for bug 508816. It would perhaps have been more accurate to name them 365666* instead of 192767*, but I wasn't aware of bug 365666 at the time.
Depends on: 992384
You need to log in before you can comment on or make changes to this bug.