Closed Bug 159704 Opened 18 years ago Closed 17 years ago

MARQUEE attribute values must be lower-case to work.

Categories

(SeaMonkey :: General, defect, P1)

x86
All

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.0.1

People

(Reporter: dcl441-bugs, Assigned: doronr)

References

()

Details

(Keywords: testcase, Whiteboard: [adt2 RTM] [ETA 08/01])

Attachments

(4 files, 4 obsolete files)

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.1b) Gecko/20020727
BuildID:    20020727

Hi.

   The MARQUEE tag has a parameter DIRECTION which can be set to LEFT or RIGHT,
but also to UP or DOWN in order to do a vertical scroll (just like a film's
credits), that is not implentented yet in Mozilla.

   You can check this at my simple webpage
http://www.danielclemente.com/html/index.htm#scroll
   or writing the HTML code: <MARQUEE DIRECTION="UP">this is a test</MARQUEE>
  
   Thanks,
          Daniel Clemente


Reproducible: Always
Steps to Reproduce:
Open an .HTM that contains this code:

<MARQUEE DIRECTION="UP">this is a test</MARQUEE>


Actual Results:  The text is quiet.

Expected Results:  The text should move from bottom to top.

<MARQUEE DIRECTION="UP">this is a test</MARQUEE>
<MARQUEE DIRECTION="DOWN">this is a test</MARQUEE>

(LEFT and RIGHT work ok, but UP and DOWN do not)
<MARQUEE DIRECTION="LEFT">this is a test</MARQUEE>
<MARQUEE DIRECTION="RIGHT">this is a test</MARQUEE>
Adding another sample - which seems to work fine:

<marquee behavior=scroll direction=up width=130 height=80 scrollamount=1
scrolldelay=0>
This works pretty well
Are you sure your code was valid?
</marquee>
nothing moves with attached testcase 
win98 2002072508
Ok, I've got it:

   I'm sure, 
<MARQUEE DIRECTION="UP">this is a test</MARQUEE>
   doesn't work.

   But this:
<MARQUEE DIRECTION="up">this is a test</MARQUEE>
   works perfectly.

   So, the problem is the case.
From bug 80269: "Will <Marquee> tag be supported?"

> <marquee> is a Microsoft-proprietary tag and is not in any HTML
> standard, thus it is unlikely that Mozilla will support it.

==> VERIF/WONTFIX


Status: UNCONFIRMED → RESOLVED
Closed: 18 years ago
Depends on: 80269
Resolution: --- → WONTFIX
Whiteboard: verifyme
FYI: A XBL emulation of the MARQUEE tag was checked in yesterday (bug 156979)
So.. umm.. it's kinda supported now.
removing bogus dependency
No longer depends on: 80269
Reopening. This is no longer a wontfix, it's a valid bug in a new element.
Status: RESOLVED → UNCONFIRMED
Resolution: WONTFIX → ---
So this bug should be resummarized to "marquee attribute values must be
lower-case to work", and we need to verify that case-sensitivity is a bug (HTML
is case insensitive, so I expect it is).  I'll leave it to Daniel to resummarize.

/be
Status: UNCONFIRMED → NEW
Ever confirmed: true
   Probably it's a bug, since HTML is case-insensitive. In addition, if LEFT and
RIGHT values work regardless to the case, UP and DOWN should do the same.

   Let me put this few examples of what works and don't work (build 2002072704)

<MARQUEE DIRECTION="left">this works</MARQUEE>
<MARQUEE DIRECTION="right">this works</MARQUEE>
<MARQUEE DIRECTION="up">this works</MARQUEE>
<MARQUEE DIRECTION="down">this works</MARQUEE>

<MARQUEE DIRECTION="LEFT">this works</MARQUEE>
<MARQUEE DIRECTION="RIGHT">this works</MARQUEE>
<MARQUEE DIRECTION="UP">this DOESN'T work</MARQUEE>          <-----  FIX THIS ----->
<MARQUEE DIRECTION="DOWN">this DOESN'T work</MARQUEE>    <-----  FIX THIS ----->
Summary: Vertical marquee doesn't work → MARQUEE attribute values must be lower-case to work.
In response to comment 5: would it make sense to go back and reopen all
<marquee> related bugs in the database, since we do support it now? Otherwise,
we'll have more confusion duping some bugs to old WONTFIXes and some other ones
to this.

Hmm. I have 2002072608, and it doesn't support it yet. I'm just one day behind
the events, and I'm not current enough already! Talk about bugreports from 2
month old builds. =^.^=
Whiteboard: verifyme
Attention: I spotted another bug with <MARQUEE DIRECTION="LEFT">, which moves to
the right instead. See attachment.
Attached file testcase
Keywords: testcase
Ok, so this is related to Bug 156979.  
Will submit a patch in a few.
Attached patch patch (obsolete) — Splinter Review
Please don't pull my tail too hard, it's my first try. =^.^=
Keywords: patch, review
Comment on attachment 93046 [details] [diff] [review]
patch

Index: xbl-marquee.xml
===================================================================
RCS file:
/cvsroot/mozilla/layout/html/document/src/xbl-marquee/resources/content/xbl-mar
quee.xml,v
retrieving revision 1.2
diff -u -r1.2 xbl-marquee.xml
--- xbl-marquee.xml	Sat Jul 26 21:07:03 2002	1.2
+++ xbl-marquee.xml	Sat Jul 27 21:06:00 2002
@@ -135,7 +135,7 @@
	   {
	     this.scrollStarted = 1; //we only want this to run once

-	     switch (this.direction)
+	     switch (this.direction.toLowerCase())
	     {
	       case "up":
		 this.dirsign = 1;
@@ -151,18 +151,18 @@
		 this.stopAt  = 0;
	       break;

-	       case "left":
-		 this.dirsign = 1;
-		 this.startAt = this.innerDiv.offsetLeft -
this.outerDiv.offsetWidth;
-		 this.stopAt  = this.outerDiv.offsetWidth +
this.innerDiv.offsetWidth + this.startAt;
-	       break;
-
	       case "right":
-	       default:
		 this.dirsign = -1;
		 this.stopAt  = this.innerDiv.offsetLeft -
this.outerDiv.offsetWidth;
		 this.startAt = this.outerDiv.offsetWidth +
this.innerDiv.offsetWidth + this.stopAt;	      
	       break;
+
+	       case "left":
+	       default:
+		 this.dirsign = 1;
+		 this.startAt = this.innerDiv.offsetLeft -
this.outerDiv.offsetWidth;
+		 this.stopAt  = this.outerDiv.offsetWidth +
this.innerDiv.offsetWidth + this.startAt;
+	       break;
	     }
	     this.newPosition = this.startAt;
	   } //end if
@@ -185,7 +185,7 @@
	     }
	   }

-	   switch(this.direction)
+	   switch(this.direction.toLowerCase())
	   {
	     case "up":
	     case "down":
@@ -223,7 +223,7 @@
	 <![CDATA[
	   var height;

-	   if ((this.direction == "up") || (this.direction == "down")) {
+	   if ((this.direction.toLowerCase() == "up") ||
(this.direction.toLowerCase() == "down")) {
	     height = "200px";
	     this.outerDiv.style.height = height;
	   }
@@ -238,7 +238,7 @@

	   this.outerDiv.style.width = this.width;

-	   if ((this.direction == "up") || (this.direction == "down")) {
+	   if ((this.direction.toLowerCase() == "up") ||
(this.direction.toLowerCase() == "down")) {
	     this.innerDiv.style.padding = height + " 0";
	   }
Attached patch patch (proper one) (obsolete) — Splinter Review
Eep. My apologies, first one was a mess. Silly, silly me! Try rhis.
Attachment #93046 - Attachment is obsolete: true
Wouldn't it be simpler to change the getter for the direction property to do
toLowerCase()?  (You'd still need to do the left/default combination, but not
the other changes.)
Most definitely, but I'm afraid to mess something up. ;)
OK, I'll try and do some serious revamping now.
Could anybody please explain me, is there any specific reason in using this:

      <method name="start">
          if (this.scrollStarted == 0)
          {
            this.scrollStarted = 1; //we only want this to run once
            ...code...
          }
 
instead of putting  ...code...  into init() method?

Assignee: Matti → wesha
Attached patch Updated patch (obsolete) — Splinter Review
* Added toLowerCase() to prop retrievers
* Moved a piece of code that was intended to execute only once to init()
* Optimized switch()
Attachment #93047 - Attachment is obsolete: true
going to steal this, I have some other issues that need to be cleaned up.
OS: Linux → All
note that the reason the code was in start is that in theory, you can stop the
marquee and change its direction. I just never implemented it fully, which is
the bug I want to fix.
stealing bug
Assignee: wesha → doron
QA Contact: asa → nobody
Doron, I doubt JS engine has builtin optimizer, so please also remove any
"case"s in "switch" statement that are grouped with "default".
> cleaning up marquee xbl - adding dynamic direction changing, bgcolor
> and lowercasing

Should we update the summary accordingly?

Are any of these problems critical to the Chinese top sites?

Don't we want to have this in RTM, if so, please nominate...
Keywords: adt1.0.1
   Ok, please tell me if I -the reporter- must do anything (it's my first bug).

   I'd like to explain another possible 'bug' of the MARQUEE tag: when we set
BEHAVIOR parameter to ALTERNATE the text should bounce, but shouldn't hide
itself at the borders.

   Check the attachment.
I am fully aware of that difference, and don't think its that important for
1.0.1.  Lets try to support it as minimally as possible.
And let's keep this bug focused on the case sensitivity problem, please file new
bugs for feature requests.
anyone want to review this? We'll only get qa probably once its reviewed.
this looks completely straight forward
r=bstell@ix.netcom.com
Attachment #93217 - Flags: review+
Attachment #93110 - Attachment is obsolete: true
Comment on attachment 93217 [details] [diff] [review]
branch patch - uper case working, bgcolor, cleaned up a not needed case:

+	  // change the scroll position
+	   if ((this.direction == "up") || (this.direction == "down"))
...

Minor style nit, fix the indentation of the comment.

sr=jst
Attachment #93217 - Flags: superreview+
In the start(),

              case "left":
                this.dirsign = 1;
                this.startAt = this.innerDiv.offsetLeft - this.outerDiv.offsetWidth;
                this.stopAt  = this.outerDiv.offsetWidth +
this.innerDiv.offsetWidth +
this.startAt;
              break;

              case "right":
              default:
                this.dirsign = -1;
                this.stopAt  = this.innerDiv.offsetLeft - this.outerDiv.offsetWidth;
                this.startAt = this.outerDiv.offsetWidth +
this.innerDiv.offsetWidth +
this.stopAt;              
              break;

1) default should be "left", not "right".
2) If "case" group contains "default" case, everything but "default" label is
senseless.

Therefore,

              case "right":
                this.dirsign = -1;
                this.stopAt  = this.innerDiv.offsetLeft - this.outerDiv.offsetWidth;
                this.startAt = this.outerDiv.offsetWidth +
this.innerDiv.offsetWidth +
this.stopAt;              
              break;

              default:
                this.dirsign = 1;
                this.startAt = this.innerDiv.offsetLeft - this.outerDiv.offsetWidth;
                this.stopAt  = this.outerDiv.offsetWidth +
this.innerDiv.offsetWidth +
this.startAt;
              break;



Attention, please DO NOT get "setting default to left in marquee.direction to
"left"" and  this "default"!!! The first one happens if marquee.direction is
empty, while the second happens when it's not empty but isn't one of the keywords.
I am unclear how this is related to making the marquee code case insensitive. 
This sounds like a separate issue. Would you please open a separate bug?
It's more a matter of code optimizaton than a real bug. Besides, only a couple
trivial changes needed to fix it, so why starting the process from the very
beginning when it's possible to add a couple lines to current patch and resolve
it once and forever?
In general the idea of getting multiple things in at once is good but time is
very tight right now and getting an approval is harder the larger the patch is.
Blocks: 143047
Severity: normal → major
Priority: -- → P1
Whiteboard: [adt2 RTM] [ETA 08/01]
Target Milestone: --- → mozilla1.0.1
Keywords: approval
Comment on attachment 93217 [details] [diff] [review]
branch patch - uper case working, bgcolor, cleaned up a not needed case:

a=chofmann for 1.0.1 on this update to the patch for the branch.
Attachment #93217 - Flags: approval+
checked into the branch
posthumus adt1.0.1+. 

brian: pls do not check in to the 1.0 branch, without ADT approval. ADT approval
is expressed with a "adt1.0.1+" in Keywords, and a comment from the ADT that the
patch is approved for checking into the branch. thanks!
Keywords: adt1.0.1adt1.0.1+
Sorry, I thought a=chofmann was approval.
brian: a=chofmann is drivers' aproval for the branch. netscape employees
checking into the branch are also required to have ADT approval before checking
into the 1.0 branch. additionally, once you have checked something into the 1.0
branch, you should add the keyword "fixed1.0.1", so that QA is notified to
verify the fix. thanks!

ji: xianlan, can you pls verify this as fixed on the branch, then replace
"fixed1.0.1" with "verified1.0.1"

doron: will you or brian be checking this into the trunk?
Keywords: mozilla1.0.1fixed1.0.1
QA Contact: nobody → ji
jaime: it's quite ironic that you've called brian on this point given that the
original marquee for 1.0.1 landed a=jaime w/o any driver approval, especially
not for 1_0 which is supposed to get features *after* trunk.
I'll be glad to check it into the trunk once I know it has approval.
ji - can you verify this bug fix in branch?  When verified, pls replace
fixed1.0.1 keyword with verified1.0.1.  Thanks.
2002082304/WinXP - testcase still doesnt work.
> ji - can you verify this bug fix in branch?  When verified, pls replace
> fixed1.0.1 keyword with verified1.0.1.  Thanks.

ji is on vacation until 8/27.
Carine can you help?  or Andreas is there someone else that can?
verified on 1.0.1 branch
Status: NEW → ASSIGNED
   Should someone check-in the patch into the trunk, or first the bug must be
marked as RESOLVED ?
   With Linux 2002100604, the bug is present yet.
This bug is still present in Mozilla 1.4.

I also note that some HTML examples sites (such as
http://www.htmlcodetutorial.com/_MARQUEE_DIRECTION.html) don't use any lower
case for their HTML examples.
I concur with the analysis.

In addition, the "alternate" behavior NEVER works (it should bounce from side to
side; instead it slides across like a normal marquee).

<marquee width=100% behavior=alternate>test</marquee>
That code bounces on the right side, but slides out on the left before returning
to the right. Kinda weird, if you ask me.

Mozilla 1.5, Win32, build 20030925.
Blocks: 203856
lower-case is now fixed
Status: ASSIGNED → RESOLVED
Closed: 18 years ago17 years ago
Resolution: --- → FIXED
Fixed by what? 

->WORKSFORME
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → RESOLVED
Closed: 17 years ago17 years ago
Resolution: --- → WORKSFORME
Doh!  My apologies.  The patch in this very bug.  (Slapping myself on the face
and resetting.)
Resolution: WORKSFORME → FIXED
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.