Closed Bug 336736 Opened 15 years ago Closed 14 years ago

Marquee text not shown when direction is RTL

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: nir123, Assigned: dholbert)

References

Details

(Keywords: regression, rtl, testcase, Whiteboard: [dbaron-1.9:RwCo])

Attachments

(5 files, 4 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060504 Minefield/3.0a1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060504 Minefield/3.0a1

When body dir set to "RTL" (Body dir="RTL"), Firefox doesn't show marquees.

Reproducible: Always
Version: unspecified → Trunk
Attached file testcase
Keywords: regression, testcase
Regression range is between 2006-03-15 and 2006-03-16.
Seeing this on OS X.

According to the regression range, this is probably due to one of bug 192767 / bug 96394 / bug 318116.
Status: UNCONFIRMED → NEW
Component: General → Layout
Ever confirmed: true
OS: Windows XP → All
Product: Firefox → Core
QA Contact: general → layout
Hardware: PC → All
This affects all RTL marquees, regardless if the RTL direction is set on the body, on the marquee itself, or somewhere in between.

Backing out bug 318116 does not fix this, although it does cause a very narrow portion (about 1/4 letter) of the marquee text to be shown - without scrolling.

This leaves us with bug 192767 or bug 96394 as candidates.
Summary: Firefox does not show marquee when body direction set to RTL → Marquee text not shown when direction is RTL
Flags: blocking1.9a1?
Flags: blocking1.9a1? → blocking1.9+
Bug 376891 has a testcase for this.
Assignee: nobody → dholbert
The culprit seems to be the patch for bug 96394.

In a working build checked out from 2006-03-15, the patch for bug 96394 (https://bugzilla.mozilla.org/attachment.cgi?id=215137) causes the bug to appear.   I also tried applying the 2 patches for bug 192767, but they didn't cause the bug to appear.
Attached patch patch (obsolete) — Splinter Review
Updated marquee implementation to use negative coordinates in RTL mode.  

(This is needed for our RTL code to handle marquees correctly.  Alternately, we could rewrite some of the C++ RTL-handling code, but this fix seems simpler and more correct.)
Attachment #266837 - Flags: review?(dbaron)
Attached patch patch, with reftest included (obsolete) — Splinter Review
Added a reftest to the patch.  (The reftest verifies that a frozen, rightward-scrolling marquee looks the same in RTL and LTR mode.)
Attachment #266837 - Attachment is obsolete: true
Attachment #266839 - Flags: review?(dbaron)
Attachment #266837 - Flags: review?(dbaron)
d'oh -- my last patch caught a few whitespace changes I'd unintentionally made to other files.  Removed those in this version.
Attachment #266839 - Attachment is obsolete: true
Attachment #266842 - Flags: review?(dbaron)
Attachment #266839 - Flags: review?(dbaron)
Attached patch updated patch (cosmetic change) (obsolete) — Splinter Review
Cosmetic change to my added code. :)  I just moved a blank line to make more sense in the block I added  to xbl-marquee.xml.

(sorry for all the patch revisions -- :) this is the last one, I think, aside from responses to any forthcoming suggestions from dbaron)
Attachment #266842 - Attachment is obsolete: true
Attachment #266844 - Flags: review?(dbaron)
Attachment #266842 - Flags: review?(dbaron)
Attached file testcase2
Thanks, Daniel.
Your patch makes it clear that the .scrollLeft property for rtl elements can become negative in current trunk builds. Something that can be tested with this simple testcase.
I don't think that is something that should be happening, since IE doesn't do this.
Blocks: 96394
(In reply to comment #11)

Hi Martijn,

Yeah, scrollLeft can currently become negative on Trunk in RTL elements.  I believe this has been supported since the patch to bug 192767 called "patch to make our scrolling code accept negative coordinates". (This patch modified nsScrollPortView.cpp:ClampScrollValues(), which effectively governs the bounds on the scrollLeft property.)

I tend to think that it makes intuitive sense to have a negative value for scrollLeft in RTL regions.  We'd really like to have a *scrollRight* property for RTL regions, to mirror scrollLeft in LTR regions, but barring that, we can use negative values for scrollLeft to achieve the same thing.

On the other hand, if having negative scrollLeft value is really a bad thing, we should have a separate bug for that, because that patch would need to be more complex and would affect more things than just marquees.
Ok, let's see what dbaron thinks about this. 
Comment on attachment 266844 [details] [diff] [review]
updated patch (cosmetic change)

>Index: reftests/bugs/336736-1-ref.html

How do you guarantee that the test hasn't scrolled by the time the image is captured?

>+            // RTL Support.  If text direction is right-to-left and marquee 
>+            // axis is horizontal, then negative-swap stopAt and startAt.

s/negative-swap/negate and swap/

>+            var dir = document.defaultView.getComputedStyle(this, "").direction;
>+            if (dir == "rtl") {
>+                if (this._direction == "right" || this._direction == "left") {

Seems like you might want to reverse the order of these two tests.

Also, shouldn't you be testing != "top" && != "bottom", so that a marquee without direction gets this treatment too?  Or is _direction somehow initialized in those cases?  (Also, should an rtl marquee without direction scroll the opposite direction from an ltr marquee?

martijn.martijn@gmail.com should review this as well.
Attachment #266844 - Flags: review?(dbaron) → review-
Although, really, the other problem is that bug 192767 and followups added preferences to control which direction scrolling is allowed, so it's probably not the best idea for us to make assumptions about it.

(Does IE ever allow scrolling where the default position is at the right edge?  If so, how does it handle scrollLeft in those cases?)
(In reply to comment #14)
> How do you guarantee that the test hasn't scrolled by the time the image is
> captured?

In the reftest, the scrollamount="0" attribute on the marquee prevents any scrolling from occurring at all.  It seemed like it'd be too tricky to snapshot a moving marquee mid-scroll, so I figured I'd test using a non-moving marquee.

> Seems like you might want to reverse the order of these two tests.

Not sure I agree... I'd imagine it's better to have the (dir == "rtl") check first, because that will fail most of the time (and skip the other condition altogether), whereas the other condition (checking for a horizontal marquee) will almost always pass (since most marquees are horizontal).  Maybe I'm over-thinkinking the performance aspect, though.

> Also, shouldn't you be testing != "top" && != "bottom", so that a marquee
> without direction gets this treatment too?  Or is _direction somehow
> initialized in those cases?  

Oops, good catch.  I think you're right and I'm not handling the no-direction case -- I'll fix that.

> (Also, should an rtl marquee without direction
> scroll the opposite direction from an ltr marquee?

Seems reasonable.  Konqueror and Sarari use opposite directions for RTL and LTR marquees, as you suggest, but IE and Opera use the same default direction for RTL and LTR.  

I think the opposite-direction approach makes more sense, because someone reading RTL text wants to see the beginning of their text first, not the end of it.  I'll post a separate patch for this.
Attached patch Updated patchSplinter Review
Only change from previous patch is the s/negative-swap/negate and swap/.

RE dbaron's comment about checking this._direction == left || this._direction == right: it turns out the _direction field *is* automatically initialized, after all, and it has input validation (in the _set_direction method) to make sure the value is one of left/right/up/down.  (I tried a testcase with no specified direction and a testcase with a gibberish specified direction, and both of those had their _direction fields set to "left.", so this initialization/input validation seems to work.)

I think I addressed all of dbaron's other comments in my previous post.

I'll post a separate patch for reversing the default Marquee direction in RTL regions.
Attachment #266844 - Attachment is obsolete: true
Attachment #266945 - Flags: review?(dbaron)
Attachment #266945 - Flags: review?(martijn.martijn)
(In reply to comment #15)
> (Does IE ever allow scrolling where the default position is at the right edge? 
> If so, how does it handle scrollLeft in those cases?)

For Martijn's "testcase2" (https://bugzilla.mozilla.org/attachment.cgi?id=266846), IE initializes the RTL region with the scrollbar all the way to the right, and with scrollLeft = 1530.  You can reduce scrollLeft to 0 by scrolling all the way in the other direction.

Firefox trunk also initializes the RTL region with the scrollbar all the way to the right, and with scrollLeft = 0.  You can reduce scrollLeft to -527 by scrolling all the way in the other direction.
This patch makes marquees scroll to the right by default in RTL regions.  This is the opposite of marquee default behavior in LTR regions, where they scroll to the left.
Attachment #266949 - Flags: review?(dbaron)
Attachment #266949 - Flags: review?(martijn.martijn)
Comment on attachment 266949 [details] [diff] [review]
patch to make RTL marquees scroll to right by default

Why do you want this?
This would make Mozilla incompatible with IE with the first testcase in this bug.
Attachment #266949 - Flags: review?(martijn.martijn) → review-
Ah, I see you discussed this in comment 16.
I'm afraid I don't agree with you.
I think it's better to be compatible with IE here (and in general). It seems to me that what Safari/Konqueror is doing is a bug.
Also, I think it's weird that when you've set direction="left" on a marquee, it scrolls to the right in an rtl environment. direction="left" means to me that it should scroll to the left, not?
I agree, it would potentially be more readable for rtl readers, but I think the webmaster could take care of that, by using direction="right". And if he's really concerned about readability, he should not use marquee at all.
Depends on: 383026
Comment on attachment 266945 [details] [diff] [review]
Updated patch

I've filed bug 383026 for the .scrollLeft issue now.
I guess this can be used as a workaround if bug 383026 doesn't get fixed before 1.9.
Thanks for the reftest, maybe a separate 'marquee' directory for reftests would be useful?
Attachment #266945 - Flags: review?(martijn.martijn) → review+
(In reply to comment #21)
> I think it's better to be compatible with IE here (and in general).

That's fine by me.  Given that the web developer can control it with by explicitly setting direction=right/left, I don't think this is a huge deal.

> Also, I think it's weird that when you've set direction="left" on a marquee, it
> scrolls to the right in an rtl environment.

I don't think this statement is correct. (But maybe I'm misunderstanding you?) If the marquee explicitly has direction="left" set, it will *always* scroll to the left, regardless of RTL vs LTR.

For example: after applying my first patch, try these test cases.  The marquee scrolls to the left in both cases.
LTR, dir=left: http://people.mozilla.com/~dholbert/tests/336736/ltr_l.html
RTL, dir=left: http://people.mozilla.com/~dholbert/tests/336736/rtl_l.html

Neither of my patches affect the fact that direction="left" makes it scroll to the left.  My second patch just changes the **default** direction, if no "direction=right/left" is supplied.
Thanks for the info. Ok, that argument doesn't count then.
However, the compatibility with IE still stands.
Comment on attachment 266945 [details] [diff] [review]
Updated patch

r+sr=dbaron, but could you get a bug filed on making scrollLeft be compatible with IE (and backing this patch out)?
Attachment #266945 - Flags: superreview+
Attachment #266945 - Flags: review?(dbaron)
Attachment #266945 - Flags: review+
Comment on attachment 266949 [details] [diff] [review]
patch to make RTL marquees scroll to right by default

Given that marquee is there only for compatibility with IE, and we don't particularly want people to use it, we're probably better off leaving this as-is, as Martijn suggests.  If we actually wanted people to use it, I'd probably support this, though.
Attachment #266949 - Flags: review?(dbaron)
(In reply to comment #25)
> r+sr=dbaron, but could you get a bug filed on making scrollLeft be compatible
> with IE (and backing this patch out)?

See bug 383026.

I landed attachment 266945 [details] [diff] [review] on the trunk, but the reftest failed on Mac (other platforms haven't cycled yet) due to a sub-pixel difference in horizontal position (which shows up on Mac).  So leaving bug open until that's addressed.
Status: NEW → ASSIGNED
Whiteboard: [dbaron-1.9:RwCo]
Attached patch Fix for reftestSplinter Review
(In reply to comment #28)
> ... but the reftest failed on Mac (other
> platforms haven't cycled yet) due to a sub-pixel difference in horizontal
> position (which shows up on Mac).  So leaving bug open until that's addressed.

I finally looked into this -- in the old reftest on Mac OS, the 'a' character's frame had a width that was in between exact pixel values, and the resulting pixel-rounding results in a 1-pixel difference between RTL and LTR contexts.

This patch replaces that reftest with a better one.  The new reftest uses an exact-pixel-width green div instead of the 'a' character, so we shouldn't have any pixel-rounding issues.

I've verified that this new reftest...
 - Fails pre-patch
 - Passes post-patch on both Mac and Linux (and I see no reason why it wouldn't pass on Windows.)
Attachment #285508 - Flags: review?(dbaron)
Attachment #285508 - Flags: review?(dbaron)
Fix for reftest checked in.  Resolving.
---------------------------

Checking in 336736-1-ref.html;
/cvsroot/mozilla/layout/reftests/bugs/336736-1-ref.html,v  <--  336736-1-ref.html
new revision: 1.2; previous revision: 1.1
done
Removing 336736-1.html;
/cvsroot/mozilla/layout/reftests/bugs/336736-1.html,v  <--  336736-1.html
new revision: delete; previous revision: 1.1
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/336736-1a.html,v
done
Checking in 336736-1a.html;
/cvsroot/mozilla/layout/reftests/bugs/336736-1a.html,v  <--  336736-1a.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/336736-1b.html,v
done
Checking in 336736-1b.html;
/cvsroot/mozilla/layout/reftests/bugs/336736-1b.html,v  <--  336736-1b.html
initial revision: 1.1
done
Checking in reftest.list;
/cvsroot/mozilla/layout/reftests/bugs/reftest.list,v  <--  reftest.list
new revision: 1.179; previous revision: 1.178
done
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Flags: in-testsuite+
The Mac failing on the reftest came up again in bug 404553, where I think I made a similar testcase (which then failed on the Mac). Is that a known bug on the Mac or an issue with the testcase?
(In reply to comment #31)
> The Mac failing on the reftest came up again in bug 404553, where I think I
> made a similar testcase (which then failed on the Mac). Is that a known bug on
> the Mac or an issue with the testcase?

(see comment 29 for background)
I'm not sure that counts as a bug... -- I think it was basically, to preserve symmetry, we end up rounding one way in LTR mode and the other way in RTL mode.

Regardless of whether that's a bug or not, if you use an exact-pixel-width object (e.g. a div, as in my updated reftest on this bug), your testcase for 404533 should work.
Mass-assigning the new rtl keyword to RTL-related (see bug 349193).
Keywords: rtl
Blocks: 1168747
You need to log in before you can comment on or make changes to this bug.