Closed Bug 192767 Opened 22 years ago Closed 19 years ago

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

Categories

(Core :: Layout: Text and Fonts, defect)

Product:
Core
Component:
Layout: Text and Fonts
Type:
defect
Priority:
Not set
Severity:
major

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: kirma, Assigned: dbaron)

References

(Blocks 2 open bugs,
URL
)

Details

(Keywords: rtl, Whiteboard: [patch])

Attachments

(10 files, 14 obsolete files)

minimal testcase
(deleted), text/html
Details
testcase for patch
(deleted), text/html
Details
some testcases, mostly xul-ish
(deleted), application/xhtml+xml; charset=UTF-8
Details
part-1/4
(deleted), patch
dbaron
: review-
Details | Diff | Splinter Review
part-2/4
(deleted), patch
Details | Diff | Splinter Review
testcase for part-4/4
(deleted), text/html
Details
testcases for canvas background issues
(deleted), text/html; charset=UTF-8
Details
IRC log (from #developers) about one of the decisions in this patch
(deleted), text/plain; charset=utf-8
Details
patch to make our scrolling code accept negative coordinates
(deleted), patch
Details | Diff | Splinter Review
fix potential problems initially setting attributes to -1
(deleted), patch
roc
: review+
roc
: superreview+
Details | Diff | Splinter Review
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
Yep, this did work, but it broke some time ago. Not sure when thugh
Status: UNCONFIRMED → NEW
Ever confirmed: true
Hardware: PC → All
this is a major thing rgression- many rtl pages are cut off if they are too wide.
Severity: normal → major
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
Blocks: 137995
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
Note that it has nothing with the zooming. Every pa
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
Blocks: 137995
mozilla1.4 shipped. unsetting blocking1.4 request.
Flags: blocking1.4?
Blocks: 240501
*** Bug 251940 has been marked as a duplicate of this bug. ***
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
(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
(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.
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..
(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/
*** Bug 300343 has been marked as a duplicate of this bug. ***
Attached file minimal testcase (deleted) —
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
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?).
Blocks: 96394, bidi_relnotes, 202386, 318116
*** 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.
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.)
(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.
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.
(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.
The discussion about scrolling to both sides does NOT belong on this bug. If you want to have that discussion, please use bug 6976.
(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.)
(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?
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).
(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.
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.
Attached patch patch (obsolete) (deleted) — — Splinter Review
This patch is effective in making horizontal scrollbar appear in the screen.
Attachment #208622 - Flags: review?(dbaron)
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-
Attached patch patch (obsolete) (deleted) — — Splinter Review
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
Attached file testcase for patch (deleted) —
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.
Attached patch patch (obsolete) (deleted) — — Splinter Review
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.
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.
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.
(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.
Attached patch patch to make our scrolling code accept negative coordinates (obsolete) (deleted) — — Splinter Review
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
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.
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).
(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?
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.
Attached patch patch to make our scrolling code accept negative coordinates (obsolete) (deleted) — — Splinter Review
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
...and makes resizing the window reset the scrollbars. Oops. I'll debug that later and attach something new. Not worth testing this yet.
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)
Attached patch patch to make our scrolling code accept negative coordinates (obsolete) (deleted) — — Splinter Review
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
Attached file some testcases, mostly xul-ish (deleted) —
Assignee: dbaron → mozilla
Status: ASSIGNED → NEW
Are you planning to come back to this or do you want someone to take it over?
Not planning to come back to it anytime soon, at least. At least, I shouldn't...
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]
Attached patch patch to make our scrolling code accept negative coordinates (obsolete) (deleted) — — Splinter Review
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
Attached patch part-1/4 (deleted) — — Splinter Review
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
Attached patch part-2/4 (deleted) — — Splinter Review
Attached patch part-3/4 (obsolete) (deleted) — — Splinter Review
Attached patch part-4/4 (obsolete) (deleted) — — Splinter Review
Attached file testcase for part-4/4 (deleted) —
Attachment #213319 - Flags: review?(dbaron)
Attached patch part-4/4 (obsolete) (deleted) — — Splinter Review
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).
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
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.
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.
Attachment #213589 - Attachment is obsolete: true
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 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)
Blocks: 324685
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)
Attachment #213320 - Attachment is obsolete: true
Attachment #213320 - Flags: review?(dbaron)
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.
Attached file testcases for canvas background issues (deleted) —
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.)
Attached patch patch to make our scrolling code accept negative coordinates (obsolete) (deleted) — — Splinter Review
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
This one's pretty broken; I thought a change I made would have fixed some background bugs I saw, but it didn't.
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.
Attached patch patch to make our scrolling code accept negative coordinates (obsolete) (deleted) — — Splinter Review
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
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.
Attached patch patch to make our scrolling code accept negative coordinates (obsolete) (deleted) — — Splinter Review
Attachment #214875 - Attachment is obsolete: true
Depends on: 330302
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.
Attachment #214945 - Flags: superreview?(roc)
Attachment #214945 - Flags: review?(roc)
Attachment #214945 - Flags: superreview?(roc)
Attachment #214945 - Flags: review?(roc)
Attached patch patch to make our scrolling code accept negative coordinates (obsolete) (deleted) — — Splinter Review
Attachment #214945 - Attachment is obsolete: true
Attachment #214954 - Flags: superreview?(roc)
Attachment #214954 - Flags: review?(roc)
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+
(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.
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
Checked in to trunk.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Attachment #213319 - Attachment is obsolete: true
Attachment #213319 - Flags: review?(dbaron)
Attachment #213589 - Attachment is obsolete: true
Attachment #213589 - Flags: review?(dbaron)
Attachment #213319 - Flags: review?(dbaron)
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.
Depends on: 330732
Depends on: 330734
Attachment #213318 - Flags: review?(dbaron)
Attachment #215353 - Flags: superreview?(roc)
Attachment #215353 - Flags: review?(roc)
(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.
Depends on: 331049
Attachment #215353 - Flags: superreview?(roc)
Attachment #215353 - Flags: superreview+
Attachment #215353 - Flags: review?(roc)
Attachment #215353 - Flags: review+
Comment on attachment 215353 [details] [diff] [review] fix potential problems initially setting attributes to -1 Checked in to trunk.
No longer depends on: 330302
Depends on: 331385
Depends on: 330863
Blocks: 331562
Blocks: 331607
Depends on: 331594
Depends on: 331458
Depends on: 331746
Depends on: 333817
Depends on: 338730
No longer depends on: 338730
Blocks: Persian
*** Bug 359442 has been marked as a duplicate of this bug. ***
Could this have caused bug 365101 ?
Just noting that the 20060315 check-in from this bug (attachment 215135 [details] [diff] [review]) caused bug 330673.
Depends on: 330673
Blocks: 365666
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.
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.
Depends on: 367246
Depends on: 369610
Mass-assigning the new rtl keyword to RTL-related (see bug 349193).
Keywords: rtl
Depends on: 439293
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+
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.

Attachment

General

Creator:
Created:
Updated:
Size: